Embed DuckDB extensions in the Rill binary #4919
Conversation
runtime/drivers/duckdb/extensions.go
Outdated
|
|
||
| // Define source and destination paths | ||
| embedPath := fmt.Sprintf("embed/extensions/%s/%s", duckdbVersion, platformName) | ||
| duckdbExtensionsPath := filepath.Join(os.Getenv("HOME"), ".duckdb", "extensions", duckdbVersion, platformName) |
There was a problem hiding this comment.
Could we use os.UserHomeDir() instead of Getenv?
runtime/drivers/duckdb/extensions.go
Outdated
| defer output.Close() | ||
|
|
||
| for { | ||
| _, err = io.CopyN(output, gzipReader, 1024) // CopyN is used to prevent a warning (G110: Potential DoS vulnerability...) |
There was a problem hiding this comment.
If you prefer io.Copy, adding a // nolint is also okay IMO, since we trust the source of the data
runtime/drivers/duckdb/extensions.go
Outdated
| // Since DuckDB is called from multiple packages, the extensions are installed in the init function | ||
| func init() { | ||
| err := installExtensions() | ||
| if err != nil { | ||
| // If extensions cannot be installed, log the error and continue as the extensions can be downloaded | ||
| // Should it be fatal in order to notice the issue prior to a release? | ||
| log.Printf("Error preparing DuckDB extensions: %v", err) | ||
| } | ||
| } |
There was a problem hiding this comment.
I think this might delay process start. Can we move this to lazy evaluation, e.g. using a sync.Once global that is called when the first DuckDB handle is opened?
There was a problem hiding this comment.
Do you expect something similar to this?
var once sync.Once
func (c *connection) prepareExtensions() error {
var err error
once.Do(func() {
err = installExtensions()
})
return err
}
However DuckDB is also used in duckdbsql package (query function). This function is called on app start by emitStartEvent.
There was a problem hiding this comment.
Yeah something like that. You could make it a public function, like duckdb.PrepareExtensions, and call it from the duckdbsql package before loading an extension (and other packages that load extensions).
I understand in practice it gets loaded fast for rill start, but there are other CLI commands like rill user add etc. that don't need DuckDB, and also some tests that don't need it but might still load the DuckDB driver.
There was a problem hiding this comment.
Moved to separate package to resolve a circular dependency and also I decided to ignore the case if no extensions are embedded at all as this might be noisy in dev runs and tests
runtime/drivers/duckdb/extensions.go
Outdated
| } | ||
| } | ||
|
|
||
| //go:embed embed/extensions/* |
There was a problem hiding this comment.
To avoid accidental commits, can we add the extension files here to gitignore? Might need a nested pattern like runtime/drivers/duckdb/embed/extensions/*/* or something like that to still maintain the .gitkeep file.
begelundmuller
added
the
blocker
Closes #4574