From 2bfeea2e105d96786ba154add8ea50227b566773 Mon Sep 17 00:00:00 2001 From: James Greenhill Date: Fri, 11 Sep 2026 21:34:42 +0000 Subject: [PATCH 1/6] feat: accept an org's Trino catalog name as a logical catalog alias MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A Duckgres session may now connect with `database=org_` — the same catalog name the org has on Trino — and get the physical DuckLake catalog under that name. current_database(), pg_database, information_schema, three-part references, and `USE` all answer to it. The point is SQLMesh: it sees ONE catalog name on both the Duckgres and Trino engines, so moving a project between engines needs no state rewrite. PR #651's invariant is preserved. The startup `database` is still never used to find, select, or route to an org. Identity stays SNI-only, and the alias is validated AGAINST the org SNI has already resolved — it is compared to that org's own catalog name, never used as a lookup key. A sibling tenant's catalog name is just another unrecognized string and fails closed, exactly as today. Sessions connecting with "ducklake" or nothing are unchanged; the alias is opt-in per connection. The name derivation moves to configstore (untagged) so every build has it; provisioner.TrinoCatalogName now delegates to that one definition. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01WCY5Jf2BQPCVKJTZU1TpEe --- controlplane/configstore/store.go | 53 ++++++++--- controlplane/configstore/store_test.go | 95 +++++++++++++++++++ controlplane/configstore/trinoname.go | 48 ++++++++++ controlplane/control.go | 48 +++++++--- controlplane/provisioner/trino_provisioner.go | 35 ++----- controlplane/session_search_path.go | 16 ++++ controlplane/session_search_path_test.go | 27 ++++++ server/conn_query_exec.go | 26 ++++- server/direct_query_rewrite_test.go | 48 ++++++++++ server/logical_catalog_alias_test.go | 38 ++++++++ server/session_database_metadata_test.go | 82 ++++++++++++++++ 11 files changed, 461 insertions(+), 55 deletions(-) create mode 100644 controlplane/configstore/trinoname.go create mode 100644 server/logical_catalog_alias_test.go diff --git a/controlplane/configstore/store.go b/controlplane/configstore/store.go index ddde24cc4..e9d0d5c33 100644 --- a/controlplane/configstore/store.go +++ b/controlplane/configstore/store.go @@ -110,9 +110,10 @@ type Snapshot struct { OrgUserAccess map[OrgUserKey]OrgUserAccessConfig } -// Selectable catalog names. The startup `database` param now names the catalog -// a session defaults to rather than identifying the org — these are the only -// non-empty values a client may request. +// The physical catalog name. The startup `database` param now names the +// catalog a session defaults to rather than identifying the org; a client may +// request this, "", or its own org's logical alias (TrinoCatalogName of the +// org's database_name) and nothing else. const ( catalogDuckLake = "ducklake" ) @@ -133,11 +134,21 @@ type PostgresConnectionResolution struct { SNIAliasUsed bool // SNIResolved is true when the managed hostname resolved to a known org. SNIResolved bool - // EffectiveCatalog is the catalog the session should default to, selected by - // the startup `database` param: "" (use the attached default) or "ducklake". + // EffectiveCatalog is the REAL catalog the session should default to, + // selected by the startup `database` param: "" (use the attached default) + // or "ducklake". A logical alias still resolves to "ducklake" here — + // execution always targets the physical catalog. EffectiveCatalog string + // LogicalCatalog is the client-visible name for that same catalog, set only + // when the startup `database` matched the SNI-resolved org's own Trino + // catalog name (`org_`). Empty for "" and "ducklake", which + // report the physical name. It renames the catalog on the PG wire + // (current_database(), pg_database, information_schema) and in three-part + // references; it never changes what the session executes against. + LogicalCatalog string // CatalogValid is false when the requested `database` is not a selectable - // catalog name (anything other than "" or "ducklake"). + // catalog name: anything other than "", "ducklake", or the SNI-resolved + // org's own catalog name. CatalogValid bool // Valid is true when (OrgID, username, password) authenticated. Valid bool @@ -565,10 +576,12 @@ func (cs *ConfigStore) ResolvePostgresConnection(startupDatabase, sniPrefix stri result := PostgresConnectionResolution{} // The startup `database` param is now pure catalog selection, not identity. - // Valid values: "" (use the attached default) or "ducklake". Anything else - // fails closed — there is no logical-name masking, so an arbitrary name no - // longer routes anywhere. - switch strings.ToLower(strings.TrimSpace(startupDatabase)) { + // Valid values: "" (use the attached default), "ducklake", or — resolved + // further down, once SNI has named an org — that org's own Trino catalog + // name. Anything else fails closed: there is no logical-name masking, so an + // arbitrary name no longer routes anywhere. + requestedCatalog := strings.ToLower(strings.TrimSpace(startupDatabase)) + switch requestedCatalog { case "": result.CatalogValid = true case catalogDuckLake: @@ -588,7 +601,7 @@ func (cs *ConfigStore) ResolvePostgresConnection(startupDatabase, sniPrefix stri if !useManagedSNI { return result } - orgID, _, aliasUsed := resolveSNIPrefixFromSnapshot(cs.snapshot, sniPrefix) + orgID, databaseName, aliasUsed := resolveSNIPrefixFromSnapshot(cs.snapshot, sniPrefix) if orgID == "" { return result } @@ -597,6 +610,24 @@ func (cs *ConfigStore) ResolvePostgresConnection(startupDatabase, sniPrefix stri result.SNIOrgID = orgID result.OrgID = orgID + // Logical catalog alias. A session may also name the catalog THIS org + // already has on Trino (`org_`) and get the same physical + // DuckLake catalog under that name, so one engine-agnostic catalog name + // works on both engines. + // + // Direction matters, and it is the whole of PR #651's invariant: the name + // is compared against the catalog name derived from the org SNI has ALREADY + // resolved. It is never a key into DatabaseOrg, Orgs, or any other map, so + // it can neither discover nor select an org — a sibling tenant's catalog + // name is just another unrecognized string here, and fails closed. Never + // rewrite this as a lookup from name to org. + if !result.CatalogValid && requestedCatalog != "" && databaseName != "" && + requestedCatalog == TrinoCatalogName(databaseName) { + result.EffectiveCatalog = catalogDuckLake + result.LogicalCatalog = requestedCatalog + result.CatalogValid = true + } + // Authenticate the user within the resolved org. Minted service // credentials (svc_-prefixed usernames) resolve against the grants // snapshot map ONLY — the service plane shares no storage with diff --git a/controlplane/configstore/store_test.go b/controlplane/configstore/store_test.go index da126c718..ad21706c0 100644 --- a/controlplane/configstore/store_test.go +++ b/controlplane/configstore/store_test.go @@ -830,3 +830,98 @@ func TestWithSnapshotHoldsPublicationReadLock(t *testing.T) { t.Fatal("WithSnapshot did not invoke callback") } } + +// TestResolvePostgresConnectionLogicalCatalog covers the logical catalog alias: +// a session may name its org's Trino catalog (`org_`) as the +// startup `database` and get the SAME physical DuckLake catalog under that +// name. The alias is validated AGAINST the org the managed hostname already +// resolved — it is never a lookup key, so it cannot route anywhere (PR #651). +func TestResolvePostgresConnectionLogicalCatalog(t *testing.T) { + cs := &ConfigStore{ + snapshot: &Snapshot{ + Orgs: map[string]*OrgConfig{ + "acme": {Name: "acme", DatabaseName: "acme-analytics"}, + "billing": {Name: "billing", DatabaseName: "billing_db"}, + }, + DatabaseOrg: map[string]string{ + "acme-analytics": "acme", + "billing_db": "billing", + }, + OrgUserPassword: map[OrgUserKey]string{ + {OrgID: "acme", Username: "root"}: mustHash(t, "secret"), + {OrgID: "billing", Username: "root"}: mustHash(t, "secret"), + }, + }, + } + + t.Run("org catalog name selects the physical catalog under the logical name", func(t *testing.T) { + got := cs.ResolvePostgresConnection("org_acme_analytics", "acme-analytics", true, "root", "secret") + if !got.CatalogValid { + t.Fatalf("org catalog name must be selectable: %+v", got) + } + if got.EffectiveCatalog != "ducklake" { + t.Fatalf("EffectiveCatalog = %q, want ducklake (execution stays physical): %+v", got.EffectiveCatalog, got) + } + if got.LogicalCatalog != "org_acme_analytics" { + t.Fatalf("LogicalCatalog = %q, want org_acme_analytics: %+v", got.LogicalCatalog, got) + } + if !got.Valid || got.OrgID != "acme" { + t.Fatalf("unexpected auth result: %+v", got) + } + }) + + t.Run("mixed case and surrounding space normalize to the canonical name", func(t *testing.T) { + got := cs.ResolvePostgresConnection(" ORG_Acme_Analytics ", "acme-analytics", true, "root", "secret") + if !got.CatalogValid || got.LogicalCatalog != "org_acme_analytics" { + t.Fatalf("catalog = (valid=%v, logical=%q), want the canonical lowercase name: %+v", + got.CatalogValid, got.LogicalCatalog, got) + } + }) + + t.Run("ducklake and empty carry no logical name", func(t *testing.T) { + for _, db := range []string{"", "ducklake"} { + got := cs.ResolvePostgresConnection(db, "acme-analytics", true, "root", "secret") + if !got.CatalogValid || got.LogicalCatalog != "" { + t.Fatalf("database %q: catalog = (valid=%v, logical=%q), want valid with no logical name: %+v", + db, got.CatalogValid, got.LogicalCatalog, got) + } + } + }) + + t.Run("an arbitrary name still fails closed", func(t *testing.T) { + for _, db := range []string{"postgres", "org_", "org_nope", "acme-analytics", "acme"} { + got := cs.ResolvePostgresConnection(db, "acme-analytics", true, "root", "secret") + if got.CatalogValid || got.LogicalCatalog != "" { + t.Fatalf("database %q must fail closed: %+v", db, got) + } + } + }) + + t.Run("another org's catalog name is refused", func(t *testing.T) { + // The security case: SNI authenticates acme, so billing's catalog name + // must not be selectable — not even with acme's valid credentials. + got := cs.ResolvePostgresConnection("org_billing_db", "acme-analytics", true, "root", "secret") + if got.CatalogValid || got.LogicalCatalog != "" { + t.Fatalf("a sibling org's catalog name must fail closed: %+v", got) + } + if got.OrgID != "acme" { + t.Fatalf("OrgID = %q, want acme — identity still comes from SNI alone", got.OrgID) + } + }) + + t.Run("without managed SNI there is no org to validate against", func(t *testing.T) { + // No SNI-resolved org means no catalog name to compare to, so the alias + // cannot be accepted. The startup database must never resolve an org. + got := cs.ResolvePostgresConnection("org_acme_analytics", "acme-analytics", false, "root", "secret") + if got.CatalogValid || got.SNIResolved { + t.Fatalf("logical alias must not be accepted without managed SNI: %+v", got) + } + }) + + t.Run("unknown managed hostname refuses the alias", func(t *testing.T) { + got := cs.ResolvePostgresConnection("org_acme_analytics", "ghostorg", true, "root", "secret") + if got.CatalogValid || got.SNIResolved || got.OrgID != "" { + t.Fatalf("unknown SNI must not admit a logical alias: %+v", got) + } + }) +} diff --git a/controlplane/configstore/trinoname.go b/controlplane/configstore/trinoname.go new file mode 100644 index 000000000..198e76aaa --- /dev/null +++ b/controlplane/configstore/trinoname.go @@ -0,0 +1,48 @@ +package configstore + +import ( + "regexp" + "strings" +) + +// trinoCatalogIdentifier is the Trino catalog identifier grammar +// ([a-z0-9_]+). Anything outside this set in the principal is replaced with +// `_` before forming the catalog name. +var trinoCatalogIdentifier = regexp.MustCompile(`[^a-z0-9_]`) + +// TrinoSanitize lowercases and replaces non-[a-z0-9_] runs with `_`. +// Pure function so callers can recover the sanitized name without holding a +// provisioner. +func TrinoSanitize(principal string) string { + return trinoCatalogIdentifier.ReplaceAllString(strings.ToLower(principal), "_") +} + +// TrinoCatalogName returns the catalog identifier for an org. +// Format: org_. The sanitization maps the org's TrinoPrincipal +// (its database_name) to Trino identifier rules ([a-z0-9_]); any other +// characters collapse to underscores. +// +// For principals that satisfy ValidateDatabaseName the mapping is injective +// — that grammar allows only lowercase alphanumerics and hyphens, so the +// hyphen is the only character rewritten and no valid principal contains the +// underscore it becomes — which, with database_name's global unique index, +// makes distinct orgs' catalog names distinct by construction. Grandfathered +// rows predate the validation and can still converge; the Trino provisioner's +// rejectPrincipalCollisions holds those orgs back rather than letting one read +// the other's catalog. +// +// The name carried an `_iceberg` suffix while the backing table format was +// Iceberg behind Lakekeeper. Warehouses are DuckLake now (migration 000014 +// dropped every iceberg_* column), so the suffix went with it. The shape is +// pinned from three sides — this function, opa.ManagedCatalogPattern, and +// the regex literal inside policy.rego — and the pair of tests named in +// ManagedCatalogPattern's doc comment fails if any one of them moves alone. +// +// It lives here, not in the (kubernetes-tagged) Trino provisioner, because +// ResolvePostgresConnection needs it in every build: the same name is the +// logical catalog alias a pgwire session may connect with, so SQLMesh and +// friends see ONE catalog name across the Duckgres and Trino engines. +// provisioner.TrinoCatalogName delegates here. +func TrinoCatalogName(principal string) string { + return "org_" + TrinoSanitize(principal) +} diff --git a/controlplane/control.go b/controlplane/control.go index 36043177f..20d819507 100644 --- a/controlplane/control.go +++ b/controlplane/control.go @@ -1042,11 +1042,15 @@ func (cp *ControlPlane) handleConnection(conn net.Conn) { // In multi-tenant mode the org is resolved solely from the managed hostname // (SNI); the user is authenticated within that org. The startup `database` // param no longer identifies the org — it selects which attached catalog - // (ducklake) the session defaults to. + // (ducklake) the session defaults to, optionally under the org's own Trino + // catalog name as a logical alias. var ( - orgID string - passthroughUser bool - requestedCatalog string // "" | "ducklake" (validated below) + orgID string + passthroughUser bool + requestedCatalog string // "" | "ducklake" (validated below) + // logicalCatalog is the client-visible name for that same catalog when + // the connection selected its org's Trino catalog name; "" otherwise. + logicalCatalog string queryAccessPolicy *server.QueryAccessPolicy ) if cp.configStore != nil { @@ -1082,8 +1086,10 @@ func (cp *ControlPlane) handleConnection(conn net.Conn) { return } if !resolution.CatalogValid { - // The startup `database` is now a catalog selector; only - // "ducklake"/empty are valid. No logical-name masking. + // The startup `database` is now a catalog selector: "ducklake", + // empty, or this org's own Trino catalog name. No org lookup — an + // unrecognized name (a sibling tenant's catalog included) is refused + // here, never routed. clog.Warn("Postgres connection rejected: requested database is not a selectable catalog.", "database", database, "org", resolution.OrgID) _ = server.WriteErrorResponse(writer, "FATAL", "3D000", @@ -1115,6 +1121,7 @@ func (cp *ControlPlane) handleConnection(conn net.Conn) { clog = clog.With("org", orgID) passthroughUser = resolution.Passthrough requestedCatalog = resolution.EffectiveCatalog + logicalCatalog = resolution.LogicalCatalog if resolution.QueryAccess != nil { queryAccessPolicy = &server.QueryAccessPolicy{ ReadOnly: resolution.QueryAccess.ReadOnly, @@ -1408,6 +1415,7 @@ func (cp *ControlPlane) handleConnection(conn net.Conn) { sessionMeta := sessionMetadataInput{ database: database, requestedCatalog: requestedCatalog, + logicalCatalog: logicalCatalog, passthroughUser: passthroughUser, clientSearchPath: clientSearchPath, queryAccessPolicy: queryAccessPolicy, @@ -1443,7 +1451,7 @@ func (cp *ControlPlane) handleConnection(conn net.Conn) { return } duckLakeAttached = true - database = effectiveCatalog + database = visibleCatalogName(logicalCatalog, effectiveCatalog) defer destroySessionOnExit() // No slow pre-ready acquisition to watch: ReadyForQuery follows the // initial parameters immediately, so the disconnect watcher would only @@ -1508,10 +1516,12 @@ func (cp *ControlPlane) handleConnection(conn net.Conn) { } duckLakeAttached = meta.duckLakeAttached effectiveCatalog = meta.effectiveCatalog - // `database` now reflects the real catalog the session defaults to — this is - // what drives the current_database() macro/pg_database view and what logs and + // `database` now reflects the name the session's catalog answers to — the + // logical alias when one was selected, else the real attached catalog. + // This is what drives the current_database() macro/pg_database view, what + // the transpiler rewrites three-part references from, and what logs and // observability surface. - database = effectiveCatalog + database = meta.visibleCatalog // Register the TCP connection so OnWorkerCrash can close it to unblock // the message loop if the backing worker dies. @@ -1745,7 +1755,7 @@ func (cp *ControlPlane) handleConnection(conn net.Conn) { // being silently ignored. server.SetConnectionPhysicalCatalog(cc, res.meta.effectiveCatalog) server.SetCatalogUseRewrite(cc, res.meta.duckLakeAttached && !passthroughUser) - server.SetConnectionDatabase(cc, res.meta.effectiveCatalog) + server.SetConnectionDatabase(cc, res.meta.visibleCatalog) if pinned { // Off the tier without a worker switch: the connection is // already ON the escalation target, so escalating would destroy @@ -1924,6 +1934,10 @@ type sessionMetadataInput struct { // requestedCatalog is the multitenant user resolution's effective catalog // ("" = the connection's default). requestedCatalog string + // logicalCatalog is the client-visible alias the connection selected for + // that catalog (its org's Trino catalog name); "" when it connected with + // "ducklake" or nothing. Renames the catalog on the PG wire only. + logicalCatalog string // passthroughUser skips pg_catalog init + catalog USE rewriting. passthroughUser bool // clientSearchPath is the connect-time `-c search_path=...` (already @@ -1945,6 +1959,9 @@ type sessionMetadataResult struct { duckLakeAttached bool // effectiveCatalog is the real catalog the session defaults to. effectiveCatalog string + // visibleCatalog is the name that catalog answers to on the PG wire — the + // logical alias when the connection selected one, else effectiveCatalog. + visibleCatalog string } // sessionInitError is a session-metadata init failure. It has ALREADY been @@ -2033,6 +2050,7 @@ func (cp *ControlPlane) initSessionMetadata( } } res.effectiveCatalog = effectiveCatalog + res.visibleCatalog = visibleCatalogName(in.logicalCatalog, effectiveCatalog) // Passthrough users skip pg_catalog initialization and the catalog USE // rewriting — they bypass the PG compatibility layer entirely. They still @@ -2048,7 +2066,11 @@ func (cp *ControlPlane) initSessionMetadata( AllowedRelations: in.queryAccessPolicy.AllowedRelations, } } - if err := sessionmeta.InitSessionDatabaseMetadataWithAccess(initCtx, exec, effectiveCatalog, metadataAccess); err != nil { + // The metadata surfaces report the VISIBLE name — the logical alias when + // the connection selected one — so current_database() and the + // information_schema views agree with the name the client connected + // with. Everything that executes below still uses effectiveCatalog. + if err := sessionmeta.InitSessionDatabaseMetadataWithAccess(initCtx, exec, res.visibleCatalog, metadataAccess); err != nil { initContextErr := initCtx.Err() initCancel() outcome, reason := controlPlaneSessionStartOperationResult( @@ -2057,7 +2079,7 @@ func (cp *ControlPlane) initSessionMetadata( cp.isDraining(), observe.SessionStartReasonMetadataStore, ) - clog.Error("Failed to initialize session database metadata.", "database", effectiveCatalog, "error", err) + clog.Error("Failed to initialize session database metadata.", "database", res.visibleCatalog, "error", err) return res, &sessionInitError{ outcome: outcome, reason: reason, code: "XX000", message: "failed to initialize session database metadata", err: err, diff --git a/controlplane/provisioner/trino_provisioner.go b/controlplane/provisioner/trino_provisioner.go index cb4384bb9..0bb54ac62 100644 --- a/controlplane/provisioner/trino_provisioner.go +++ b/controlplane/provisioner/trino_provisioner.go @@ -181,40 +181,19 @@ var secretDataKeyPattern = regexp.MustCompile(`^[-._a-zA-Z0-9]+$`) // admin authority from drifting apart. var managedCatalogRe = regexp.MustCompile(opa.ManagedCatalogPattern) -// trinoCatalogIdentifier is the Trino catalog identifier grammar -// ([a-z0-9_]+). Anything outside this set in Org.Name is replaced with -// `_` before forming the catalog name. -var trinoCatalogIdentifier = regexp.MustCompile(`[^a-z0-9_]`) - // trinoSanitize lowercases and replaces non-[a-z0-9_] runs with `_`. -// Pure function so callers can recover the sanitized name without -// holding the provisioner. +// Thin alias for the canonical definition: the same sanitization backs the +// pgwire logical catalog alias, which every build needs, so it lives in +// configstore (untagged) rather than in this kubernetes-tagged file. func trinoSanitize(orgName string) string { - lower := strings.ToLower(orgName) - return trinoCatalogIdentifier.ReplaceAllString(lower, "_") + return configstore.TrinoSanitize(orgName) } // TrinoCatalogName returns the catalog identifier for an org. -// Format: org_. The sanitization maps Org.Name to Trino -// identifier rules ([a-z0-9_]); any other characters collapse to -// underscores. -// -// For principals that satisfy ValidateDatabaseName the mapping is injective -// — that grammar allows only lowercase alphanumerics and hyphens, so the -// hyphen is the only character rewritten and no valid principal contains the -// underscore it becomes — which, with database_name's global unique index, -// makes distinct orgs' catalog names distinct by construction. Grandfathered -// rows predate the validation and can still converge; rejectPrincipalCollisions -// holds those orgs back rather than letting one read the other's catalog. -// -// The name carried an `_iceberg` suffix while the backing table format was -// Iceberg behind Lakekeeper. Warehouses are DuckLake now (migration 000014 -// dropped every iceberg_* column), so the suffix went with it. The shape is -// pinned from three sides — this function, opa.ManagedCatalogPattern, and -// the regex literal inside policy.rego — and the pair of tests named in -// ManagedCatalogPattern's doc comment fails if any one of them moves alone. +// Format: org_. See configstore.TrinoCatalogName for the full +// contract — this is the Trino-side spelling of that one definition. func TrinoCatalogName(principal string) string { - return "org_" + trinoSanitize(principal) + return configstore.TrinoCatalogName(principal) } // TrinoGroupName returns the file-group-provider group label for an org, diff --git a/controlplane/session_search_path.go b/controlplane/session_search_path.go index cdf6ba899..0f38484ab 100644 --- a/controlplane/session_search_path.go +++ b/controlplane/session_search_path.go @@ -65,6 +65,22 @@ func resolveEffectiveCatalog(requested string, duckLakeAttached bool) (string, b return "", false } +// visibleCatalogName returns the name the session reports for its catalog on +// the PG wire — current_database(), pg_database, information_schema, the logs, +// and the catalog half of a three-part reference. +// +// It is the logical alias when the connection selected one (its org's Trino +// catalog name, validated against the SNI-resolved org in +// ResolvePostgresConnection), else the real attached catalog. Only the NAME +// differs: every statement still executes against effectiveCatalog, and the +// transpiler rewrites the alias back to it. +func visibleCatalogName(logicalCatalog, effectiveCatalog string) string { + if logicalCatalog != "" { + return logicalCatalog + } + return effectiveCatalog +} + func ensureMemoryMainInSearchPath(searchPath string) string { if strings.Contains(strings.ToLower(searchPath), "memory.main") { return searchPath diff --git a/controlplane/session_search_path_test.go b/controlplane/session_search_path_test.go index 021faaff8..745d6b98a 100644 --- a/controlplane/session_search_path_test.go +++ b/controlplane/session_search_path_test.go @@ -80,3 +80,30 @@ func TestResolveEffectiveCatalog(t *testing.T) { }) } } + +// TestVisibleCatalogName pins the split between the name a session reports and +// the catalog it executes against: a logical alias renames, it never redirects. +func TestVisibleCatalogName(t *testing.T) { + tests := []struct { + name string + logical string + effective string + want string + }{ + {name: "no alias reports the physical catalog", logical: "", effective: "ducklake", want: "ducklake"}, + {name: "alias renames the physical catalog", logical: "org_acme", effective: "ducklake", want: "org_acme"}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := visibleCatalogName(tt.logical, tt.effective); got != tt.want { + t.Fatalf("visibleCatalogName(%q, %q) = %q, want %q", tt.logical, tt.effective, got, tt.want) + } + }) + } + + // The alias is a NAME. Execution still resolves to the attached catalog, + // so resolveEffectiveCatalog must be unaffected by it. + if got, ok := resolveEffectiveCatalog("ducklake", true); got != "ducklake" || !ok { + t.Fatalf("resolveEffectiveCatalog under an alias = (%q, %v), want (ducklake, true)", got, ok) + } +} diff --git a/server/conn_query_exec.go b/server/conn_query_exec.go index 81723a865..8245ae27d 100644 --- a/server/conn_query_exec.go +++ b/server/conn_query_exec.go @@ -104,8 +104,15 @@ func (c *clientConn) executeQueryDirect(query, cmdType string) error { // `catalog.schema` target. This is NOT logical-name masking — the catalog name // is real; the rewrite only works around DuckDB's bare-catalog `USE` // resolution (a bare `USE ducklake` issued while the session is in another -// catalog resolves `ducklake` as a *schema* within that catalog). Any other -// `USE ` and all other statements are passed through unchanged. +// catalog resolves `ducklake` as a *schema* within that catalog). +// +// A session connected under its org's logical catalog alias (c.database, the +// org's Trino catalog name) may name the catalog either way: the alias is the +// only catalog name such a client ever sees, so `USE ` has to land where +// `USE ducklake` does. That alias was validated against the session's own org +// at connect time, so it can only ever name this session's own catalog. +// +// Any other `USE ` and all other statements are passed through unchanged. func (c *clientConn) rewriteDirectQuery(query string) string { if c == nil || c.server == nil || c.passthrough || !c.catalogUseRewrite { return query @@ -133,7 +140,7 @@ func (c *clientConn) rewriteDirectQuery(query string) string { unquoted = strings.ReplaceAll(target[1:len(target)-1], `""`, `"`) } - if !strings.EqualFold(unquoted, physicalDuckLakeCatalog) { + if !strings.EqualFold(unquoted, physicalDuckLakeCatalog) && !c.namesDuckLakeCatalog(unquoted) { return query } // `USE ducklake` -> ducklake.main (DuckLake's real schema is `main`). @@ -146,6 +153,19 @@ func (c *clientConn) rewriteDirectQuery(query string) string { return rewritten } +// namesDuckLakeCatalog reports whether name is this session's logical alias for +// the physical DuckLake catalog. True only when the session actually executes +// against DuckLake and its PG-visible database name differs from the physical +// one — i.e. the control plane accepted an org catalog name at connect. Every +// other session (standalone, plain "ducklake", memory) has no alias, so this is +// false and nothing new is rewritten. +func (c *clientConn) namesDuckLakeCatalog(name string) bool { + return c.physicalCatalog == physicalDuckLakeCatalog && + c.database != "" && + !strings.EqualFold(c.database, physicalDuckLakeCatalog) && + strings.EqualFold(c.database, name) +} + // physicalDuckLakeCatalog is the physical catalog name DuckLake is attached as. const physicalDuckLakeCatalog = "ducklake" diff --git a/server/direct_query_rewrite_test.go b/server/direct_query_rewrite_test.go index b8dce2322..6a6e6b898 100644 --- a/server/direct_query_rewrite_test.go +++ b/server/direct_query_rewrite_test.go @@ -103,3 +103,51 @@ func TestRewriteDirectQueryPreservesUseWithoutCatalogRewrite(t *testing.T) { t.Fatalf("rewriteDirectQuery(USE ducklake) = %q, want %q", got, want) } } + +// TestRewriteDirectQueryLogicalCatalogAlias covers a session connected under +// its org's Trino catalog name: `USE ` must reach the same physical +// catalog as `USE ducklake`, so a client that sees one catalog name can switch +// to it by that name. Any OTHER name still passes through untouched. +func TestRewriteDirectQueryLogicalCatalogAlias(t *testing.T) { + c := &clientConn{ + server: &Server{}, + database: "org_acme_analytics", + physicalCatalog: physicalDuckLakeCatalog, + catalogUseRewrite: true, + } + + tests := []struct { + name string + query string + want string + }{ + { + name: "rewrites the logical alias to two-part ducklake.main", + query: "USE org_acme_analytics", + want: "USE ducklake.main", + }, + { + name: "rewrites the quoted logical alias", + query: `USE "org_acme_analytics";`, + want: "USE ducklake.main;", + }, + { + name: "still rewrites the physical name", + query: "USE ducklake", + want: "USE ducklake.main", + }, + { + name: "preserves another org's catalog name", + query: "USE org_billing_db", + want: "USE org_billing_db", + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + if got := c.rewriteDirectQuery(tc.query); got != tc.want { + t.Fatalf("rewriteDirectQuery(%q) = %q, want %q", tc.query, got, tc.want) + } + }) + } +} diff --git a/server/logical_catalog_alias_test.go b/server/logical_catalog_alias_test.go new file mode 100644 index 000000000..5025e1cf3 --- /dev/null +++ b/server/logical_catalog_alias_test.go @@ -0,0 +1,38 @@ +package server + +import ( + "strings" + "testing" +) + +// TestNewTranspilerRewritesLogicalCatalogAlias pins the connection-level wiring +// that makes the logical catalog alias usable in SQL: the session's PG-visible +// database name becomes the transpiler's logical catalog, so a three-part +// reference written against the org's Trino catalog name reaches the physical +// DuckLake catalog. Without this, a client that sees only the alias could not +// write a fully-qualified reference at all. +func TestNewTranspilerRewritesLogicalCatalogAlias(t *testing.T) { + c := &clientConn{ + server: &Server{}, + database: "org_acme_analytics", + physicalCatalog: physicalDuckLakeCatalog, + } + + got, err := c.newTranspiler(false).Transpile("SELECT id FROM org_acme_analytics.public.events") + if err != nil { + t.Fatalf("transpile: %v", err) + } + if !strings.Contains(got.SQL, "ducklake.main.events") { + t.Fatalf("transpiled = %q, want a rewrite to ducklake.main.events", got.SQL) + } + + // A sibling org's catalog name is not this session's alias, so it is left + // alone and fails on the worker rather than resolving to this tenant's data. + got, err = c.newTranspiler(false).Transpile("SELECT id FROM org_billing_db.public.events") + if err != nil { + t.Fatalf("transpile foreign catalog: %v", err) + } + if strings.Contains(got.SQL, "ducklake.") { + t.Fatalf("transpiled = %q, a foreign catalog name must not be rewritten onto ducklake", got.SQL) + } +} diff --git a/server/session_database_metadata_test.go b/server/session_database_metadata_test.go index 19ef9661c..99edacd7d 100644 --- a/server/session_database_metadata_test.go +++ b/server/session_database_metadata_test.go @@ -712,3 +712,85 @@ func TestProjectMetadataViewsHideRelationsOutsideAccessPolicy(t *testing.T) { FROM memory.main.information_schema_sequences_compat `, "team_42.owned_seq") } + +// TestInitSessionDatabaseMetadataReportsLogicalCatalogAlias covers a session +// connected under its org's Trino catalog name: every pg-visible catalog +// surface reports the LOGICAL name, while the session still executes against +// the physical `ducklake` catalog. That split is the whole point of the alias — +// SQLMesh sees one catalog name on the Duckgres and Trino engines, and nothing +// about execution moves. +func TestInitSessionDatabaseMetadataReportsLogicalCatalogAlias(t *testing.T) { + const logicalCatalog = "org_acme_analytics" + + db, err := sql.Open("duckdb", ":memory:") + if err != nil { + t.Fatalf("open duckdb: %v", err) + } + db.SetMaxOpenConns(1) + defer func() { _ = db.Close() }() + + if _, err := db.Exec(`ATTACH ':memory:' AS ducklake`); err != nil { + t.Fatalf("attach ducklake: %v", err) + } + if err := initInformationSchema(db, true); err != nil { + t.Fatalf("init information_schema: %v", err) + } + if _, err := db.Exec("USE ducklake"); err != nil { + t.Fatalf("use ducklake: %v", err) + } + if _, err := db.Exec("CREATE TABLE main.events(id INTEGER)"); err != nil { + t.Fatalf("create ducklake table: %v", err) + } + + executor := NewLocalExecutor(db) + if err := sessionmeta.InitSessionDatabaseMetadata(context.Background(), executor, logicalCatalog); err != nil { + t.Fatalf("init session database metadata: %v", err) + } + + assertSingleValue := func(label, query, want string) { + t.Helper() + var got string + if err := db.QueryRow(query).Scan(&got); err != nil { + t.Fatalf("%s query: %v", label, err) + } + if got != want { + t.Fatalf("%s = %q, want %q", label, got, want) + } + } + + assertSingleValue("current_database", "SELECT current_database()", logicalCatalog) + assertSingleValue("pg_database", + "SELECT datname FROM memory.main.pg_database WHERE datname = current_database()", logicalCatalog) + assertSingleValue("information_schema.tables catalog", ` + SELECT DISTINCT table_catalog + FROM memory.main.information_schema_tables_compat + WHERE table_name = 'events' + `, logicalCatalog) + assertSingleValue("information_schema.schemata catalog", ` + SELECT DISTINCT catalog_name + FROM memory.main.information_schema_schemata_compat + WHERE schema_name = 'public' + `, logicalCatalog) + assertSingleValue("information_schema.columns catalog", ` + SELECT DISTINCT table_catalog + FROM memory.main.information_schema_columns_compat + WHERE table_name = 'events' + `, logicalCatalog) + + // The alias renames; it never redirects. Unqualified DDL after init must + // still land in the PHYSICAL catalog — there is no catalog called + // org_acme_analytics for it to land in. + if _, err := db.Exec("CREATE TABLE alias_probe(id INTEGER)"); err != nil { + t.Fatalf("create table under the alias: %v", err) + } + var probes int + if err := db.QueryRow(` + SELECT COUNT(*) FROM duckdb_tables() + WHERE database_name = 'ducklake' AND table_name = 'alias_probe' + `).Scan(&probes); err != nil { + t.Fatalf("probe query: %v", err) + } + if probes != 1 { + t.Fatalf("alias_probe rows in the ducklake catalog = %d, want 1 — execution must stay physical", probes) + } +} From a89b1eb4ac5d044b2c9cce065bfb18e2f2dd9357 Mon Sep 17 00:00:00 2001 From: James Greenhill Date: Fri, 11 Sep 2026 21:37:07 +0000 Subject: [PATCH 2/6] test(e2e): assert the logical catalog alias against mw-dev MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit logical_catalog_alias connects the cnpg tenant with its own Trino catalog name and asserts the alias renames without redirecting: current_database() and pg_database report it, a three-part reference and `USE ` reach the real catalog, and a session connected the ordinary way sees the same row. It also asserts the security half — a sibling tenant's catalog name and an arbitrary name both get 3D000. Docs: CLAUDE.md gains the alias contract and the PR #651 invariant that governs it; the harness path references are corrected to the directory that exists (tests/mw-dev/e2e/, not tests/e2e-mw-dev/). Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01WCY5Jf2BQPCVKJTZU1TpEe --- CLAUDE.md | 39 ++++++++++++++++++++-- cmd/cache-proxy/README.md | 2 +- controlplane/admin/README.md | 2 +- tests/mw-dev/README.md | 6 ++-- tests/mw-dev/e2e/harness.sh | 65 ++++++++++++++++++++++++++++++++++++ 5 files changed, 107 insertions(+), 7 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index d1e223b9f..251eafc25 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -180,7 +180,7 @@ The project uses [just](https://github.com/casey/just) as a command runner. Run **Every feature, behavior change, bugfix, AND refactor that affects runtime or cluster behavior MUST ship with a solid end-to-end test case in -`tests/e2e-mw-dev/` (`harness.sh`).** This is not just for new features — any +`tests/mw-dev/e2e/` (`harness.sh`).** This is not just for new features — any change to how the system behaves at runtime (new capability, changed semantics, a fixed bug, a new config knob, an activation/routing/teardown tweak) extends or adds a harness assertion in the same PR. Refactors count too: when you move or @@ -202,11 +202,11 @@ Three test lanes worth knowing about, in increasing order of blast radius: - **Unit / package tests** (`go test ./...`): in-process, no external deps. Where most coverage lives. Includes `tests/manifests/` (static-manifest artifact asserts for `k8s/rbac.yaml` + `k8s/networkpolicy.yaml`). - **`tests/integration/`** (`just test-integration`): spins up the standalone server binary against a real MinIO + Postgres metadata store via docker compose. Covers wire protocol, DuckLake on real S3-compatible storage, transpilation against a live server. -- **`tests/e2e-mw-dev/`** (per-PR GitHub workflow `e2e-mw-dev.yml`): the full multi-tenant activation pipeline against the **real posthog-mw-dev EKS cluster** — real Cilium, real Crossplane ducklings, real cnpg-shard + external-RDS metadata, real AWS S3. A shell harness (`harness.sh`) runs as an in-cluster Job per PR; `run.sh` orchestrates deploy/test/teardown/e2e-cleanup. **Replaces the retired kind suite** (`tests/k8s/`) — that suite's `k8s-integration-tests` CI job and its Go tests are gone; the supporting `k8s/` scripts/manifests + Dockerfiles are kept for now. See `tests/e2e-mw-dev/README.md`. +- **`tests/mw-dev/e2e/`** (per-PR GitHub workflow `e2e-mw-dev.yml`): the full multi-tenant activation pipeline against the **real posthog-mw-dev EKS cluster** — real Cilium, real Crossplane ducklings, real cnpg-shard + external-RDS metadata, real AWS S3. A shell harness (`harness.sh`) runs as an in-cluster Job per PR; `run.sh` orchestrates deploy/test/teardown/e2e-cleanup. **Replaces the retired kind suite** (`tests/k8s/`) — that suite's `k8s-integration-tests` CI job and its Go tests are gone; the supporting `k8s/` scripts/manifests + Dockerfiles are kept for now. See `tests/mw-dev/README.md`. ### When code changes obligate test changes -`tests/e2e-mw-dev/` is the only place we exercise the full activation pipeline (control plane → STS broker → worker pod → DuckDB → ATTACH against real cloud storage). If your change touches any of the following, treat updating the harness as part of the change, not a follow-up: +`tests/mw-dev/e2e/` is the only place we exercise the full activation pipeline (control plane → STS broker → worker pod → DuckDB → ATTACH against real cloud storage). If your change touches any of the following, treat updating the harness as part of the change, not a follow-up: - `controlplane/shared_worker_activator.go`, `controlplane/sts_broker.go`, anything in the activation payload shape (`TenantActivationPayload`, `server.DuckLakeConfig`) - `server/server.go::AttachDeltaCatalog`, `server.attachDuckLake*`, `server.refresh*Secret` @@ -1716,6 +1716,39 @@ password/tenant/catalog changes never propagate. the `ui/src/lib/trino.test.ts` derivations and `tests/mw-dev/e2e/trino.sh`. +## Logical Catalog Alias (`org_` as the startup `database`) + +A pgwire session may select its catalog by the name the org has on Trino +(`configstore.TrinoCatalogName`, `org_`) instead of +`ducklake`. The catalog is the same one either way: the alias only renames it +on the wire. This exists so SQLMesh sees ONE catalog name across the Duckgres +and Trino engines and migrating between them needs no state rewrite. + +- **What the startup `database` may be**: `""`, `ducklake`, or the + SNI-resolved org's own catalog name. Everything else is 3D000, as before. +- **Identity is still SNI-only, and this must stay true.** The alias is + compared against the catalog name of the org the managed hostname ALREADY + resolved — it is never a key into `DatabaseOrg`, `Orgs`, or any other map, + so it can neither discover nor select an org. A sibling tenant's catalog + name is just an unrecognized string and fails closed. If a change here ever + looks up an org BY the database name, it has reintroduced exactly what PR + #651 removed. See the comment in `ResolvePostgresConnection`. +- **Physical vs. visible**: `EffectiveCatalog` stays `ducklake` and is what + every statement executes against; `LogicalCatalog` (and + `sessionMetadataResult.visibleCatalog`) is the name reported by + `current_database()`, `pg_database`, `information_schema`, and the logs. + `visibleCatalogName` is the one place that chooses between them. +- **SQL written against the alias**: the transpiler's `LogicalCatalogTransform` + rewrites `.public.t` → `ducklake.main.t` (fed by `clientConn.database` + in `newTranspiler`), and `rewriteDirectQuery` expands `USE ` to + `ducklake.main`. +- Opt-in per connection: a session that connects with `ducklake` or nothing is + byte-for-byte unaffected. +- Touching any of this → update `controlplane/configstore/store_test.go`, + `controlplane/session_search_path_test.go`, `server/direct_query_rewrite_test.go`, + `server/logical_catalog_alias_test.go`, `server/session_database_metadata_test.go`, + and `logical_catalog_alias` in `tests/mw-dev/e2e/harness.sh`. + ## TODO Reference `TODO.md` is a lightweight backlog for ideas that do not yet have a better diff --git a/cmd/cache-proxy/README.md b/cmd/cache-proxy/README.md index a7294f13b..32ea8830e 100644 --- a/cmd/cache-proxy/README.md +++ b/cmd/cache-proxy/README.md @@ -278,7 +278,7 @@ observed peer outcome (`present`, `in_flight`, `negative`, `timeout`, span for every probe. `org_id` is intentionally absent — the proxy has no per-request tenant identity. -> The cache proxy is not deployed in the `tests/e2e-mw-dev` environment +> The cache proxy is not deployed in the `tests/mw-dev/e2e` environment > (`DUCKGRES_CACHE_ENABLED` is off there). Unit tests in > `cmd/cache-proxy/tracing_test.go` cover propagation behavior; validate the > complete trace in a cache-enabled dev deployment. diff --git a/controlplane/admin/README.md b/controlplane/admin/README.md index 69e1d1a68..9d87e8ecf 100644 --- a/controlplane/admin/README.md +++ b/controlplane/admin/README.md @@ -337,7 +337,7 @@ region. `dashboard_test.go` (TokenSet / break-glass login / cookie), `api_test.go` + `api_postgres_test.go` (CRUD), `models_api_test.go` (redaction). e2e: the `admin_*` / `impersonation_*` / `models_explorer_api` assertions in -`tests/e2e-mw-dev/harness.sh`. +`tests/mw-dev/e2e/harness.sh`. **Frontend** (`ui/`, Vitest + Testing Library — `just ui-test`, CI job `ui-tests`): the dashboard's data-derivation logic has shipped wrong more than diff --git a/tests/mw-dev/README.md b/tests/mw-dev/README.md index fb670c4fb..f858282d7 100644 --- a/tests/mw-dev/README.md +++ b/tests/mw-dev/README.md @@ -651,8 +651,10 @@ got through, in order — each was a real fix: `DUCKGRES_MANAGED_HOSTNAME_SUFFIXES=.ci.duckgres.local` + `DUCKGRES_SNI_ROUTING_MODE=passthrough`, and connecting with libpq `host=.` (SNI) + `hostaddr=` (TCP). -5. ✅ catalog selection — `dbname` must be `ducklake`, not the org - (PR #651: *database = catalog selection*). harness.sh now does this. +5. ✅ catalog selection — `dbname` must be `ducklake` or the org's own Trino + catalog name (`org_`, a logical alias for the same catalog), + never an arbitrary name (PR #651: *database = catalog selection*). + harness.sh covers both, in `logical_catalog_alias`. 6. ✅ **activation (cnpg DuckLake)** — the control plane resolves the metadata password from the Secret referenced by Duckling status. Activation failed at diff --git a/tests/mw-dev/e2e/harness.sh b/tests/mw-dev/e2e/harness.sh index ea6429aab..a1914e0be 100755 --- a/tests/mw-dev/e2e/harness.sh +++ b/tests/mw-dev/e2e/harness.sh @@ -3952,6 +3952,68 @@ tenant_isolation() { # orgA pwA orgB pwB pg "$1" "$2" ducklake "DROP TABLE $t;" } +# ---- logical catalog alias (org_ as the dbname) ------------- +# An org's Trino catalog name is a second, LOGICAL name for the same physical +# DuckLake catalog. SQLMesh must see one catalog name on both engines, so a +# session that connects with it has to behave exactly like a `ducklake` one +# while REPORTING the logical name everywhere a client can observe a catalog. +# +# Also the security half of PR #651: the dbname is catalog selection, never +# identity. The alias is validated against the org the SNI hostname already +# resolved, so a sibling tenant's catalog name must be refused — the same 3D000 +# any other unknown name gets, and no routing to the sibling. +trino_catalog_name() { # org -> org_ + printf 'org_%s' "$(printf %s "$1" | tr 'A-Z' 'a-z' | tr -c 'a-z0-9_' '_')" +} + +logical_catalog_alias() { # org password sibling_org + alias_db="$(trino_catalog_name "$1")" + sibling_db="$(trino_catalog_name "$3")" + log "logical catalog alias: $1 connects as $alias_db" + + # Every pg-visible catalog surface reports the LOGICAL name. + got="$(pg "$1" "$2" "$alias_db" 'SELECT current_database()')" + [ "$got" = "$alias_db" ] \ + || fail "logical alias: current_database() = '$got', want '$alias_db'" + got="$(pg "$1" "$2" "$alias_db" 'SELECT datname FROM pg_database WHERE datname = current_database()')" + [ "$got" = "$alias_db" ] \ + || fail "logical alias: pg_database datname = '$got', want '$alias_db'" + + # A three-part reference written against the logical name reaches the real + # catalog, and so does `USE `. This is what SQLMesh emits. + t="alias_$(printf %s "$1" | tr -c 'a-z0-9' _)" + pg "$1" "$2" "$alias_db" \ + "DROP TABLE IF EXISTS $alias_db.public.$t; CREATE TABLE $alias_db.public.$t AS SELECT 42 AS v;" + got="$(pg "$1" "$2" "$alias_db" "SELECT v FROM $alias_db.public.$t")" + [ "$got" = "42" ] || fail "logical alias: three-part read returned '$got', want 42" + got="$(pg "$1" "$2" "$alias_db" "USE $alias_db; SELECT v FROM $t;" | tail -1)" + [ "$got" = "42" ] || fail "logical alias: USE $alias_db then unqualified read returned '$got', want 42" + + # The alias renames; it does not fork storage. The same row is there for a + # session connected the ordinary way. + got="$(pg "$1" "$2" ducklake "SELECT v FROM main.$t")" + [ "$got" = "42" ] || fail "logical alias: ducklake session read returned '$got', want 42 (same catalog)" + pg "$1" "$2" ducklake "DROP TABLE main.$t;" + + # A sibling tenant's catalog name is not selectable, even with valid creds. + if out="$(pg_try "$1" "$2" "$sibling_db" 'SELECT 1')"; then + fail "logical alias: $1 connected with $3's catalog name $sibling_db (got '$out') — isolation breach" + fi + case "$out" in + *'does not exist'*) ;; + *) fail "logical alias: sibling-catalog rejection was not the 3D000 does-not-exist: $out" ;; + esac + + # An arbitrary name still fails closed. + if out="$(pg_try "$1" "$2" org_not_a_tenant 'SELECT 1')"; then + fail "logical alias: an arbitrary dbname connected (got '$out')" + fi + case "$out" in + *'does not exist'*) ;; + *) fail "logical alias: arbitrary-dbname rejection was wrong: $out" ;; + esac +} + # ---- lifecycle: deprovision → warehouse deleted → Duckling CR fully gone ---- # Proves the teardown path works end to end: warehouse marked deleted, the # Crossplane Duckling CR removed, and its finalizer cascade (which drops the @@ -4657,6 +4719,9 @@ engine_main() { # ---- cross-tenant isolation between independent CNPG-backed orgs ---- tenant_isolation "$CNPG" "$cnpg_pw" "$RES1" "$res1_pw" + # ---- logical catalog alias: org_ selects the same catalog -- + logical_catalog_alias "$CNPG" "$cnpg_pw" "$RES1" + # NOTE: the version-mismatch worker reaper is not exercised in-Job (it needs a # mid-run image bump); it stays covered by the controlplane/ unit tests. log "SKIP version-reaper (needs an in-run image bump; see README)" From dcb043de726ad97de13a010850249fb6244c7268 Mon Sep 17 00:00:00 2001 From: James Greenhill Date: Fri, 11 Sep 2026 21:41:12 +0000 Subject: [PATCH 3/6] refactor: drop two unreachable guards in the alias checks MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `requestedCatalog != ""` cannot be false where it sits — an empty startup database already set CatalogValid — and `c.database != ""` is subsumed by the EqualFold against a non-empty USE target. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01WCY5Jf2BQPCVKJTZU1TpEe --- controlplane/configstore/store.go | 2 +- server/conn_query_exec.go | 1 - 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/controlplane/configstore/store.go b/controlplane/configstore/store.go index e9d0d5c33..cd1d077fd 100644 --- a/controlplane/configstore/store.go +++ b/controlplane/configstore/store.go @@ -621,7 +621,7 @@ func (cs *ConfigStore) ResolvePostgresConnection(startupDatabase, sniPrefix stri // it can neither discover nor select an org — a sibling tenant's catalog // name is just another unrecognized string here, and fails closed. Never // rewrite this as a lookup from name to org. - if !result.CatalogValid && requestedCatalog != "" && databaseName != "" && + if !result.CatalogValid && databaseName != "" && requestedCatalog == TrinoCatalogName(databaseName) { result.EffectiveCatalog = catalogDuckLake result.LogicalCatalog = requestedCatalog diff --git a/server/conn_query_exec.go b/server/conn_query_exec.go index 8245ae27d..05f312649 100644 --- a/server/conn_query_exec.go +++ b/server/conn_query_exec.go @@ -161,7 +161,6 @@ func (c *clientConn) rewriteDirectQuery(query string) string { // false and nothing new is rewritten. func (c *clientConn) namesDuckLakeCatalog(name string) bool { return c.physicalCatalog == physicalDuckLakeCatalog && - c.database != "" && !strings.EqualFold(c.database, physicalDuckLakeCatalog) && strings.EqualFold(c.database, name) } From 243d9fe34a719e4ab189208f821266d461739af7 Mon Sep 17 00:00:00 2001 From: James Greenhill Date: Fri, 11 Sep 2026 21:46:47 +0000 Subject: [PATCH 4/6] fix: restore the empty-database guard in namesDuckLakeCatalog MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit dcb043de dropped `c.database != ""` on the claim that it was "subsumed by the EqualFold against a non-empty USE target". That claim was wrong: the USE target is only checked for emptiness BEFORE quote-stripping, so `USE ""` passes that check as the two-character `""` and then unquotes to "". A session whose database is unset then matched its own empty name, and invalid SQL was silently rewritten into `USE ducklake.main` instead of passing through to DuckDB and erroring. No cross-tenant exposure — it resolves to the catalog the session already executes against — but an invalid statement must not silently succeed. The other guard that commit dropped (`requestedCatalog != ""` in store.go) is genuinely unreachable and stays dropped. TestRewriteDirectQueryEmptyQuotedUseIsNotAnAlias pins it at the rewrite entry point, plus an `USE ""` case in the alias table. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01WCY5Jf2BQPCVKJTZU1TpEe --- server/conn_query_exec.go | 7 +++++++ server/direct_query_rewrite_test.go | 29 +++++++++++++++++++++++++++++ 2 files changed, 36 insertions(+) diff --git a/server/conn_query_exec.go b/server/conn_query_exec.go index 05f312649..220a9f86f 100644 --- a/server/conn_query_exec.go +++ b/server/conn_query_exec.go @@ -159,8 +159,15 @@ func (c *clientConn) rewriteDirectQuery(query string) string { // one — i.e. the control plane accepted an org catalog name at connect. Every // other session (standalone, plain "ducklake", memory) has no alias, so this is // false and nothing new is rewritten. +// +// The empty-database guard is load-bearing, not belt-and-braces. `USE ""` +// arrives here as an empty name — rewriteDirectQuery's empty-target check runs +// BEFORE quote-stripping, so `""` passes it and unquotes to "". Without the +// guard, a session whose database is unset matches its own empty name and +// invalid SQL is silently rewritten into `USE ducklake.main`. func (c *clientConn) namesDuckLakeCatalog(name string) bool { return c.physicalCatalog == physicalDuckLakeCatalog && + c.database != "" && !strings.EqualFold(c.database, physicalDuckLakeCatalog) && strings.EqualFold(c.database, name) } diff --git a/server/direct_query_rewrite_test.go b/server/direct_query_rewrite_test.go index 6a6e6b898..12a5b10f6 100644 --- a/server/direct_query_rewrite_test.go +++ b/server/direct_query_rewrite_test.go @@ -141,6 +141,11 @@ func TestRewriteDirectQueryLogicalCatalogAlias(t *testing.T) { query: "USE org_billing_db", want: "USE org_billing_db", }, + { + name: "preserves an empty quoted identifier", + query: `USE ""`, + want: `USE ""`, + }, } for _, tc := range tests { @@ -151,3 +156,27 @@ func TestRewriteDirectQueryLogicalCatalogAlias(t *testing.T) { }) } } + +// TestRewriteDirectQueryEmptyQuotedUseIsNotAnAlias is the regression net for the +// alias predicate. `USE ""` reaches the alias check as an EMPTY name: the +// empty-target check in rewriteDirectQuery runs before quote-stripping, so `""` +// gets past it and then unquotes to "". A session whose database is unset must +// not match that empty name — `USE ""` is invalid SQL and has to pass through to +// DuckDB and error there, never be silently rewritten into `USE ducklake.main`. +func TestRewriteDirectQueryEmptyQuotedUseIsNotAnAlias(t *testing.T) { + unnamed := &clientConn{ + server: &Server{}, + database: "", + physicalCatalog: physicalDuckLakeCatalog, + catalogUseRewrite: true, + } + + if got, want := unnamed.rewriteDirectQuery(`USE ""`), `USE ""`; got != want { + t.Fatalf("rewriteDirectQuery(%q) on an unnamed session = %q, want %q", `USE ""`, got, want) + } + // The same session still rewrites the physical name, so the guard narrows + // nothing it should not. + if got, want := unnamed.rewriteDirectQuery("USE ducklake"), "USE ducklake.main"; got != want { + t.Fatalf("rewriteDirectQuery(USE ducklake) = %q, want %q", got, want) + } +} From b3ae617a44331f3e2a32720273e0d4c0803e5ae7 Mon Sep 17 00:00:00 2001 From: James Greenhill Date: Fri, 11 Sep 2026 23:53:53 +0000 Subject: [PATCH 5/6] fix(e2e): send USE as its own message in the logical-alias assertion MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The e2e assertion batched `USE ; SELECT ...` into one psql -c, which duckgres cannot split. handleQuery splits a multi-statement simple query only when pg_query parses it (conn.go: `parseErr == nil && len(tree.Stmts) > 1`), and `USE` is not PostgreSQL syntax — so the batch reached rewriteDirectQuery whole, its USE target was `; SELECT ...` (matching no catalog name, so correctly left alone), and DuckDB split it and failed the bare USE. The product code was right; the assertion was written in a shape the simple query protocol does not support here. `USE ducklake; SELECT ...` fails the same way on main, so this is pre-existing and not alias-specific. pg_script feeds a script on stdin, where psql sends each statement as its own simple-query message on ONE session. Verified against a local standalone server: state set by the first statement is visible to the third, on one pid. Tests: TestLogicalCatalogAliasThroughConnectionSetup drives the alias through the REAL setup path — NewClientConn plus the exported setters control.go calls, in order, including the post-worker-switch replay — for both the simple and the extended query composition. That closes the gap that let this reach CI: the old tests hand-built a clientConn and never exercised how the session fields get populated. TestUseStatementIsNeverSplitOutOfASimpleQueryBatch pins the batching limitation so the next harness author does not rediscover it, and fails loudly if pg_query ever learns to parse USE. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01WCY5Jf2BQPCVKJTZU1TpEe --- server/logical_catalog_alias_test.go | 97 ++++++++++++++++++++++++++++ tests/mw-dev/e2e/harness.sh | 38 ++++++++++- 2 files changed, 134 insertions(+), 1 deletion(-) diff --git a/server/logical_catalog_alias_test.go b/server/logical_catalog_alias_test.go index 5025e1cf3..390e321e5 100644 --- a/server/logical_catalog_alias_test.go +++ b/server/logical_catalog_alias_test.go @@ -3,6 +3,8 @@ package server import ( "strings" "testing" + + pg_query "github.com/pganalyze/pg_query_go/v6" ) // TestNewTranspilerRewritesLogicalCatalogAlias pins the connection-level wiring @@ -36,3 +38,98 @@ func TestNewTranspilerRewritesLogicalCatalogAlias(t *testing.T) { t.Fatalf("transpiled = %q, a foreign catalog name must not be rewritten onto ducklake", got.SQL) } } + +// aliasSessionConn builds a connection the way controlplane.handleConnection +// actually builds one for a session that selected its org's catalog name: +// NewClientConn takes the VISIBLE catalog as the PG-visible database, and the +// physical catalog + USE rewriting are stamped through the exported setters +// afterwards. Going through the real constructor and the real setters is the +// point — hand-building a clientConn is what let a broken session field pass +// unit tests and fail in the e2e lane. +func aliasSessionConn(t *testing.T, visibleCatalog string) *clientConn { + t.Helper() + cc := NewClientConn(&Server{}, nil, nil, nil, + "root", "acme", visibleCatalog, "psql", nil, 1, 2, 0, "") + // control.go, in this order: physical catalog, then USE rewriting. + SetConnectionPhysicalCatalog(cc, physicalDuckLakeCatalog) + SetCatalogUseRewrite(cc, true) + return cc +} + +// TestLogicalCatalogAliasThroughConnectionSetup exercises the alias through the +// real connection-setup sequence, for both query protocols. The simple-query +// path rewrites the raw statement; the extended-query path rewrites the +// TRANSPILED statement (conn_extended_query.go stores +// rewriteDirectQuery(result.SQL) as the prepared statement's converted query), +// so both compositions are asserted here. +func TestLogicalCatalogAliasThroughConnectionSetup(t *testing.T) { + const alias = "org_acme_analytics" + cc := aliasSessionConn(t, alias) + + if got, want := cc.rewriteDirectQuery("USE "+alias), "USE ducklake.main"; got != want { + t.Fatalf("simple-query USE = %q, want %q", got, want) + } + + // Extended query: Parse transpiles first, then rewrites the result. `USE` + // is not PostgreSQL, so it falls back to the raw statement and the rewrite + // still has to fire on it. + res, err := cc.newTranspiler(true).Transpile("USE " + alias) + if err != nil { + t.Fatalf("transpile USE: %v", err) + } + if got, want := cc.rewriteDirectQuery(res.SQL), "USE ducklake.main"; got != want { + t.Fatalf("extended-query USE = %q, want %q", got, want) + } + + // A three-part reference on the same connection. + res, err = cc.newTranspiler(false).Transpile("SELECT id FROM " + alias + ".public.events") + if err != nil { + t.Fatalf("transpile three-part: %v", err) + } + if !strings.Contains(res.SQL, "ducklake.main.events") { + t.Fatalf("three-part reference = %q, want a rewrite to ducklake.main.events", res.SQL) + } + + // After a worker switch the control plane re-stamps the same three fields + // (control.go's activation path). The alias must survive that replay. + SetConnectionPhysicalCatalog(cc, physicalDuckLakeCatalog) + SetCatalogUseRewrite(cc, true) + SetConnectionDatabase(cc, alias) + if got, want := cc.rewriteDirectQuery("USE "+alias), "USE ducklake.main"; got != want { + t.Fatalf("USE after a worker switch = %q, want %q", got, want) + } +} + +// TestUseStatementIsNeverSplitOutOfASimpleQueryBatch pins the pre-existing +// limitation that broke the first version of the e2e assertion for this +// feature, so nobody re-learns it from a CI failure. +// +// handleQuery splits a multi-statement simple query only when pg_query can +// parse it (conn.go: `parseErr == nil && len(tree.Stmts) > 1`). `USE` is not +// PostgreSQL syntax, so a USE-led batch never splits: the whole string reaches +// rewriteDirectQuery as one statement, whose USE target is then everything +// after `USE` — semicolon, following statements and all — which matches no +// catalog name and is correctly left alone. DuckDB then splits the batch +// itself and fails the bare `USE`. +// +// This is NOT specific to the alias: `USE ducklake; SELECT ...` behaves +// identically. Send `USE` as its own statement. +func TestUseStatementIsNeverSplitOutOfASimpleQueryBatch(t *testing.T) { + const alias = "org_acme_analytics" + cc := aliasSessionConn(t, alias) + + for _, batch := range []string{ + "USE " + alias + "; SELECT v FROM events;", + "USE ducklake; SELECT v FROM events;", + } { + if got := cc.rewriteDirectQuery(batch); got != batch { + t.Fatalf("rewriteDirectQuery(%q) = %q; a USE-led batch must be left alone, not partially rewritten", batch, got) + } + } + + // The reason it is left alone: the batch never splits upstream. + if _, err := pg_query.Parse("USE " + alias + "; SELECT v FROM events;"); err == nil { + t.Fatal("pg_query now parses a USE-led batch; handleQuery would split it and this limitation is gone — " + + "re-check the e2e assertion and delete this test") + } +} diff --git a/tests/mw-dev/e2e/harness.sh b/tests/mw-dev/e2e/harness.sh index a1914e0be..ce9625346 100755 --- a/tests/mw-dev/e2e/harness.sh +++ b/tests/mw-dev/e2e/harness.sh @@ -308,6 +308,38 @@ pg_try() { # org password dbname sql [user=root] printf %s "$out"; return 1 } +# Runs a MULTI-statement script on ONE session, feeding it on stdin so psql +# sends each statement as its OWN simple-query message. `psql -c "a; b"` does +# not do this: it sends the whole string as a single message, and duckgres only +# splits such a batch when pg_query can parse it (conn.go: `parseErr == nil && +# len(tree.Stmts) > 1`). `USE` is not PostgreSQL syntax, so a USE-led batch +# never splits — it reaches the worker whole and DuckDB fails the bare `USE`. +# So any assertion that needs session state from an earlier statement AND a +# `USE` has to come through here, not through pg/pg_try. +# +# Same positive/abort contract and transient-retry set as _pg_exec. +# TODO: the retry case list is now spelled three times (_pg_exec, pg_try, here); +# fold them into one classifier next time this file is open for real work. +pg_script() { # org password dbname sql_script [user=root] -> prints output; rc 0 ok / 1 real error + a=0 out="" + while [ "$a" -lt 12 ]; do + if out="$(printf '%s\n' "$4" | PGPASSWORD="$2" psql \ + "sslmode=require host=$1$SNI_SUFFIX hostaddr=$CP_IP port=5432 user=${5:-root} dbname=$3" \ + -v ON_ERROR_STOP=1 -tA 2>&1)"; then + printf %s "$out"; return 0 + fi + case "$out" in + *"capacity exhausted"*|*"no Duckgres worker"*|\ + *"still provisioning"*|*"failed to initialize session"*|\ + *"timed out waiting for an available worker"*|*"failed to start"*|*"spawn sized worker"*|\ + *"failed to detect attached catalogs"*) + sleep 10; a=$((a + 1)); continue ;; + *) printf %s "$out" >&2; return 1 ;; + esac + done + printf %s "$out" >&2; return 1 +} + # Connect preflight: a worker isn't ready the instant a warehouse goes ready — # there is no warm pool, so the first connection for an org cold-spawns a worker # (and a burst can momentarily hit the org/global cap). The CP returns a @@ -3986,7 +4018,11 @@ logical_catalog_alias() { # org password sibling_org "DROP TABLE IF EXISTS $alias_db.public.$t; CREATE TABLE $alias_db.public.$t AS SELECT 42 AS v;" got="$(pg "$1" "$2" "$alias_db" "SELECT v FROM $alias_db.public.$t")" [ "$got" = "42" ] || fail "logical alias: three-part read returned '$got', want 42" - got="$(pg "$1" "$2" "$alias_db" "USE $alias_db; SELECT v FROM $t;" | tail -1)" + # `USE` must be its own simple-query message — see pg_script. The read that + # follows shares the session, so it proves the USE actually moved the session + # into the catalog rather than just returning without an error. + got="$(pg_script "$1" "$2" "$alias_db" "USE $alias_db; +SELECT v FROM $t;" | tail -1)" [ "$got" = "42" ] || fail "logical alias: USE $alias_db then unqualified read returned '$got', want 42" # The alias renames; it does not fork storage. The same row is there for a From 3c2eb9e0e85591d98a10789a52bc48da1859b959 Mon Sep 17 00:00:00 2001 From: James Greenhill Date: Fri, 11 Sep 2026 23:58:38 +0000 Subject: [PATCH 6/6] docs: record the catalog-qualified SET search_path gap `SET search_path = '.main'` is not rewritten and fails on the worker, while the physical `'ducklake.main'` works. The catalog name sits in a string literal rather than a RangeVar, so LogicalCatalogTransform has nothing to match and the USE pass does not look at SET. Deliberately left alone: SQLMesh selects a catalog with `USE ` as its own statement, and touches search_path only in dbt code it marks unsupported. Also record in the harness that project_reader_isolation already issues its USE as a separate message (psql -c/-c) and is not a latent failure, and that the retry case list now has four copies worth folding together later. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01WCY5Jf2BQPCVKJTZU1TpEe --- server/conn_query_exec.go | 9 +++++++++ tests/mw-dev/e2e/harness.sh | 11 +++++++++-- 2 files changed, 18 insertions(+), 2 deletions(-) diff --git a/server/conn_query_exec.go b/server/conn_query_exec.go index 220a9f86f..5d148b0a4 100644 --- a/server/conn_query_exec.go +++ b/server/conn_query_exec.go @@ -165,6 +165,15 @@ func (c *clientConn) rewriteDirectQuery(query string) string { // BEFORE quote-stripping, so `""` passes it and unquotes to "". Without the // guard, a session whose database is unset matches its own empty name and // invalid SQL is silently rewritten into `USE ducklake.main`. +// +// TODO: a catalog-qualified `SET search_path = '.main'` is NOT rewritten +// and fails on the worker, while the physical `'ducklake.main'` works. Neither +// this function nor the transpiler's LogicalCatalogTransform sees it: the +// catalog name sits inside a string literal, not a RangeVar, so the AST pass +// has nothing to match on and this pass only inspects `USE`. Left alone +// deliberately — SQLMesh selects a catalog with `USE ` as its own +// statement (the path above), and uses search_path only in dbt code it marks +// unsupported. Fix it here if a client ever needs the qualified form. func (c *clientConn) namesDuckLakeCatalog(name string) bool { return c.physicalCatalog == physicalDuckLakeCatalog && c.database != "" && diff --git a/tests/mw-dev/e2e/harness.sh b/tests/mw-dev/e2e/harness.sh index ce9625346..9f5cd10a7 100755 --- a/tests/mw-dev/e2e/harness.sh +++ b/tests/mw-dev/e2e/harness.sh @@ -318,8 +318,15 @@ pg_try() { # org password dbname sql [user=root] # `USE` has to come through here, not through pg/pg_try. # # Same positive/abort contract and transient-retry set as _pg_exec. -# TODO: the retry case list is now spelled three times (_pg_exec, pg_try, here); -# fold them into one classifier next time this file is open for real work. +# +# project_reader_isolation predates this helper and does the same thing inline +# with `psql -c -c ` (also one message per statement, one session) +# plus its own copy of the retry loop. Both forms are correct; those two are the +# only places in this file that issue a `USE`. +# TODO: the retry case list is now spelled four times (_pg_exec, pg_try, +# project_reader_isolation, here). Fold them into one classifier, and move +# project_reader_isolation onto this helper, next time this file is open for +# real work. pg_script() { # org password dbname sql_script [user=root] -> prints output; rc 0 ok / 1 real error a=0 out="" while [ "$a" -lt 12 ]; do