diff --git a/catalog/ducklake.go b/catalog/ducklake.go index ea074b4..f06136e 100644 --- a/catalog/ducklake.go +++ b/catalog/ducklake.go @@ -1841,9 +1841,15 @@ func (rt *duckLakeRuntime) attachLocked(ctx context.Context, key any, execer dri } // Both values have already passed configuration validation. SQL-literal // quoting is still required because service paths can contain apostrophes. - attach := "ATTACH IF NOT EXISTS " + duckDBStringLiteral("ducklake:"+metadata) + - " AS " + QuoteIdentifierANSI(DuckLakeCatalogName) + - " (DATA_PATH " + duckDBStringLiteral(dataPath) + ", DATA_INLINING_ROW_LIMIT 0, CREATE_IF_NOT_EXISTS true)" + // CREATE_IF_NOT_EXISTS is only for first attach when the catalog file is + // missing. An existing catalog ATTACHes with or without that option. + // Restart failed because attachCatalogs opened the metadata file as a + // regular DuckDB database before the ducklake: ATTACH. + missing, err := duckLakeCatalogMissing(metadata) + if err != nil { + return newDuckLakeInitError(duckLakeStageAttach, "", err) + } + attach := duckLakeAttachSQL(metadata, dataPath, missing) if _, err := execer.ExecContext(ctx, attach, nil); err != nil { return newDuckLakeInitError(duckLakeStageAttach, "", err) } @@ -1886,6 +1892,31 @@ func localDuckLakeCatalogPath() (string, error) { return filepath.Join(dir, "ducklake-catalog.duckdb"), nil } +// duckLakeStat is os.Stat in production. Tests replace it to inject Stat +// failures that chmod 000 cannot produce when the process is root. +var duckLakeStat = os.Stat + +func duckLakeCatalogMissing(path string) (bool, error) { + _, err := duckLakeStat(path) + if err == nil { + return false, nil + } + if os.IsNotExist(err) { + return true, nil + } + return false, err +} + +func duckLakeAttachSQL(metadata, dataPath string, catalogMissing bool) string { + opts := "DATA_PATH " + duckDBStringLiteral(dataPath) + ", DATA_INLINING_ROW_LIMIT 0" + if catalogMissing { + opts += ", CREATE_IF_NOT_EXISTS true" + } + return "ATTACH IF NOT EXISTS " + duckDBStringLiteral("ducklake:"+metadata) + + " AS " + QuoteIdentifierANSI(DuckLakeCatalogName) + + " (" + opts + ")" +} + func duckDBStringLiteral(value string) string { // Ordinary DuckDB string literals preserve backslashes; only a single quote // terminates the literal and therefore needs SQL-standard doubling. diff --git a/catalog/provider.go b/catalog/provider.go index b494861..f002d84 100644 --- a/catalog/provider.go +++ b/catalog/provider.go @@ -525,6 +525,17 @@ func (prov *DatabaseProvider) HasCatalog(name string) bool { } // attachCatalogs attaches all the databases in the data directory +func (prov *DatabaseProvider) duckLakeMetadataFile(name string) bool { + if prov == nil || prov.duckLake == nil { + return false + } + meta := strings.TrimSpace(prov.duckLake.config.MetadataPath) + if meta == "" { + return false + } + return filepath.Clean(filepath.Join(prov.dataDir, name)) == filepath.Clean(meta) +} + func (prov *DatabaseProvider) attachCatalogs() error { files, err := os.ReadDir(prov.dataDir) if err != nil { @@ -555,6 +566,12 @@ func (prov *DatabaseProvider) AttachCatalog(file interface { } return fmt.Errorf("file %s is not a database file", file.Name()) } + // The DuckLake catalog file lives in dataDir but must not be ATTACHed as a + // regular DuckDB database. Doing so occupies the file and makes the later + // ducklake: ATTACH fail after restart. + if prov.duckLakeMetadataFile(file.Name()) { + return nil + } name := strings.TrimSuffix(file.Name(), ".db") quoted := QuoteIdentifierANSI(name) if _, err := prov.storage.ExecContext(context.Background(), "ATTACH IF NOT EXISTS '"+filepath.Join(prov.dataDir, file.Name())+"' AS "+quoted); err != nil { diff --git a/catalog/provider_ducklake_test.go b/catalog/provider_ducklake_test.go index a8f3aad..ad118c8 100644 --- a/catalog/provider_ducklake_test.go +++ b/catalog/provider_ducklake_test.go @@ -4,6 +4,8 @@ import ( "context" stdsql "database/sql" "database/sql/driver" + "os" + "path/filepath" "testing" "time" @@ -132,6 +134,56 @@ func TestDuckLakeAttachUsesServicePaths(t *testing.T) { }, execer.queries) } +func TestDuckLakeAttachOmitsCreateIfCatalogExists(t *testing.T) { + dir := t.TempDir() + metadata := filepath.Join(dir, "catalog.ducklake") + require.NoError(t, os.WriteFile(metadata, []byte("existing"), 0o600)) + runtime := &duckLakeRuntime{config: configuration.DuckLakeConfig{ + MetadataPath: metadata, + DataPath: "s3://test-bucket/data", + }} + execer := &recordingDuckLakeExecer{} + + require.NoError(t, runtime.attachLocked(context.Background(), nil, execer)) + require.Equal(t, []string{ + "ATTACH IF NOT EXISTS 'ducklake:" + metadata + "' AS \"__myduck_ducklake\" (DATA_PATH 's3://test-bucket/data', DATA_INLINING_ROW_LIMIT 0)", + }, execer.queries) + require.NotContains(t, execer.queries[0], "CREATE_IF_NOT_EXISTS") +} + +func TestAttachCatalogSkipsDuckLakeMetadataFile(t *testing.T) { + dir := t.TempDir() + meta := filepath.Join(dir, "ducklake.db") + require.NoError(t, os.WriteFile(meta, []byte("existing"), 0o600)) + prov := &DatabaseProvider{ + dataDir: dir, + duckLake: &duckLakeRuntime{config: configuration.DuckLakeConfig{ + MetadataPath: meta, + }}, + } + info, err := os.Stat(meta) + require.NoError(t, err) + require.NoError(t, prov.AttachCatalog(info, false)) +} + +func TestDuckLakeAttachStatNonExistErrorDoesNotCreate(t *testing.T) { + orig := duckLakeStat + t.Cleanup(func() { duckLakeStat = orig }) + duckLakeStat = func(string) (os.FileInfo, error) { + return nil, os.ErrPermission + } + runtime := &duckLakeRuntime{config: configuration.DuckLakeConfig{ + MetadataPath: "/injected/catalog.ducklake", + DataPath: "s3://test-bucket/data", + }} + execer := &recordingDuckLakeExecer{} + + err := runtime.attachLocked(context.Background(), nil, execer) + require.Error(t, err) + require.Contains(t, err.Error(), "reason=permission_denied") + require.Empty(t, execer.queries) +} + func TestDuckLakeAttachRejectsRemoteCatalogURI(t *testing.T) { runtime := &duckLakeRuntime{config: configuration.DuckLakeConfig{ MetadataPath: "s3://test-bucket/catalog.ducklake", diff --git a/docker/Dockerfile b/docker/Dockerfile index ebd8b88..08820be 100644 --- a/docker/Dockerfile +++ b/docker/Dockerfile @@ -116,6 +116,9 @@ RUN chmod 755 /usr/local/lib/myduckserver /usr/local/lib/myduckserver/duckdb-ext USER admin WORKDIR /home/admin +# Empty Docker named volumes copy this directory's ownership. Without it the +# volume is root:root and admin cannot create myduck.db / ducklake.db. +RUN mkdir -p /home/admin/data /home/admin/log # Copy application files COPY --from=builder /myduckserver /usr/local/bin/myduckserver diff --git a/docs/object-storage.md b/docs/object-storage.md index 0a82493..a1e77dc 100644 --- a/docs/object-storage.md +++ b/docs/object-storage.md @@ -14,6 +14,13 @@ must not include diagnostic inject switches. catalog file (`MYDUCK_DUCKLAKE_METADATA_PATH`). The object bucket stores table data only. Losing the catalog cannot be recovered from the bucket alone. +4. **Object tables do not support PRIMARY KEY.** `CREATE TABLE ... ENGINE=DUCKLAKE` + with a primary key returns 1105. The table is not created and no object + prefix is written. +5. **Data directory ownership.** The image user is `admin` (uid 1000). The + image ships `/home/admin/data` owned by admin so an empty Docker named + volume mounted there inherits that ownership. A bind-mounted host directory + that is `root:root` still needs `chown 1000:1000` before first start. ## Scope