diff --git a/cmd/apply/apply.go b/cmd/apply/apply.go index cc04bd2a..936178dc 100644 --- a/cmd/apply/apply.go +++ b/cmd/apply/apply.go @@ -457,7 +457,7 @@ func RunApply(cmd *cobra.Command, args []string) error { func validateSchemaFingerprint(migrationPlan *plan.Plan, host string, port int, db, user, password, sslmode, schema, applicationName string, ignoreConfig *ir.IgnoreConfig) error { // Get current state from target database with ignore config // This ensures ignored objects are excluded from fingerprint calculation - currentStateIR, err := util.GetIRFromDatabase(host, port, db, user, password, sslmode, schema, applicationName, ignoreConfig) + currentStateIR, err := util.GetIRFromDatabase(host, port, db, user, password, sslmode, schema, applicationName, ignoreConfig, "") if err != nil { return fmt.Errorf("failed to get current database state for fingerprint validation: %w", err) } diff --git a/cmd/dump/dump.go b/cmd/dump/dump.go index f497a41d..cadc22bc 100644 --- a/cmd/dump/dump.go +++ b/cmd/dump/dump.go @@ -79,7 +79,7 @@ func ExecuteDump(config *DumpConfig) (string, error) { } // Get IR from database using the shared utility - schemaIR, err := util.GetIRFromDatabase(config.Host, config.Port, config.DB, config.User, config.Password, config.SSLMode, config.Schema, "pgschema", ignoreConfig) + schemaIR, err := util.GetIRFromDatabase(config.Host, config.Port, config.DB, config.User, config.Password, config.SSLMode, config.Schema, "pgschema", ignoreConfig, "") if err != nil { return "", fmt.Errorf("failed to get database schema: %w", err) } diff --git a/cmd/plan/plan.go b/cmd/plan/plan.go index b6fa5606..5e1f8b7a 100644 --- a/cmd/plan/plan.go +++ b/cmd/plan/plan.go @@ -287,7 +287,7 @@ func GeneratePlan(config *PlanConfig, provider postgres.DesiredStateProvider) (* } // Get current state from target database - currentStateIR, err := util.GetIRFromDatabase(config.Host, config.Port, config.DB, config.User, config.Password, config.SSLMode, config.Schema, config.ApplicationName, ignoreConfig) + currentStateIR, err := util.GetIRFromDatabase(config.Host, config.Port, config.DB, config.User, config.Password, config.SSLMode, config.Schema, config.ApplicationName, ignoreConfig, "") if err != nil { return nil, fmt.Errorf("failed to get current state from database: %w", err) } @@ -357,7 +357,13 @@ func GeneratePlan(config *PlanConfig, provider postgres.DesiredStateProvider) (* providerSSLMode = "prefer" } } - desiredStateIR, err := util.GetIRFromDatabase(providerHost, providerPort, providerDB, providerUsername, providerPassword, providerSSLMode, schemaToInspect, config.ApplicationName, ignoreConfig) + // managedSchema is config.Schema (not schemaToInspect): when the provider + // uses a temporary comparison schema, schemaToInspect is that temp + // schema's literal name, but config.Schema is the real schema it stands + // in for - the Inspector needs both to correctly scope extension-owned + // type handling to the real schema, not the temp one (PR #608 review + // feedback; see Inspector.SetManagedSchema). + desiredStateIR, err := util.GetIRFromDatabase(providerHost, providerPort, providerDB, providerUsername, providerPassword, providerSSLMode, schemaToInspect, config.ApplicationName, ignoreConfig, config.Schema) if err != nil { return nil, fmt.Errorf("failed to get desired state: %w", err) } diff --git a/cmd/util/connection.go b/cmd/util/connection.go index 9eba0422..6496fdea 100644 --- a/cmd/util/connection.go +++ b/cmd/util/connection.go @@ -94,8 +94,12 @@ func ValidateSSLMode(mode string) error { } } -// GetIRFromDatabase gets the IR from a database with ignore configuration -func GetIRFromDatabase(host string, port int, db, user, password, sslmode, schemaName, applicationName string, ignoreConfig *ir.IgnoreConfig) (*ir.IR, error) { +// GetIRFromDatabase gets the IR from a database with ignore configuration. +// managedSchema is the logical schema this IR represents when it differs +// from schemaName - the schema actually being connected to and introspected +// (see Inspector.SetManagedSchema). Pass "" when they're the same, which is +// every caller except the desired-state/temp-schema comparison path. +func GetIRFromDatabase(host string, port int, db, user, password, sslmode, schemaName, applicationName string, ignoreConfig *ir.IgnoreConfig, managedSchema string) (*ir.IR, error) { if sslmode == "" { sslmode = "prefer" } @@ -121,6 +125,9 @@ func GetIRFromDatabase(host string, port int, db, user, password, sslmode, schem // Build IR using the IR system with ignore config inspector := ir.NewInspector(conn, ignoreConfig) + if managedSchema != "" { + inspector.SetManagedSchema(managedSchema) + } // Default to public schema if none specified targetSchema := schemaName diff --git a/internal/postgres/external.go b/internal/postgres/external.go index 5012fc79..9c558a46 100644 --- a/internal/postgres/external.go +++ b/internal/postgres/external.go @@ -24,6 +24,14 @@ type ExternalDatabase struct { tempSchema string // Temporary schema name with timestamp suffix targetMajorVersion int // Expected major version (from target database) stubRoles []string // Roles created for ALTER DEFAULT PRIVILEGES (issue #553) + // targetExtensions is ExternalDatabaseConfig.TargetExtensions, kept for + // ApplySchema: getExtensionSchemas(ed.db) only sees what's installed on + // the plan database, so without cross-checking against what's actually + // on the target, an extension installed only on the plan side (e.g. for + // local testing convenience) would let a bare type reference resolve + // during planning that the real target could never resolve at apply + // time - plan succeeds, apply fails (PR #608 review feedback). + targetExtensions map[string]string } // ExternalDatabaseConfig holds configuration for connecting to an external database @@ -106,6 +114,7 @@ func NewExternalDatabase(config *ExternalDatabaseConfig) (*ExternalDatabase, err password: config.Password, tempSchema: tempSchema, targetMajorVersion: config.TargetMajorVersion, + targetExtensions: config.TargetExtensions, }, nil } @@ -142,8 +151,78 @@ func (ed *ExternalDatabase) ApplySchema(ctx context.Context, schema string, sql } // Set search_path to the temporary schema, with public as fallback - // for resolving extension types installed in public schema (issue #197) - setSearchPathSQL := fmt.Sprintf("SET search_path TO \"%s\", public", ed.tempSchema) + // for resolving extension types installed in public schema (issue #197). + // + // Also append the managed schema itself, if (and only if) it hosts an + // installed extension (e.g. pgvector's "vector" living in a non-public + // schema like "domain"). Desired-state SQL for an extension-owned column + // is written unqualified, same as any other same-schema reference (e.g. + // "embedding vector(384)"), and stripSchemaQualifications only strips - + // it never adds - a schema prefix. Without the extension's real schema in + // this search_path, such a bare reference cannot resolve inside the temp + // schema at all, since the temp schema has no copy of the type and + // "public" may not either (issue #518's apply-time failure mode, not + // addressed by #544's pre-flight schema-consistency check alone). This is + // safe: NewExternalDatabase already validated that every extension shared + // between the plan and target databases lives in the same schema on + // both, so resolving a bare extension reference against the plan + // database's copy here is exactly equivalent to how it resolves on the + // real target. + // + // Deliberately NOT every extension schema in the database: the real + // apply session against the target only ever uses ", public" + // (see cmd/apply/apply.go), so adding an unrelated extension's schema + // here would let a cross-schema type resolve during planning that the + // real apply could never resolve, and would explicitly relocate + // pg_catalog out of its default implicit-first search position for the + // common case of a bundled extension (e.g. plpgsql) living there (PR + // #608 review feedback). + // + // Inserted before "public" (not after): the real apply session's + // search_path is ", public", so the managed schema takes + // priority over public there. Appending it after public here would flip + // that priority for the plan-side lookup, letting a same-named object in + // public shadow the managed schema's extension type on the plan side + // while the real target resolves it the other way around (PR #608 + // review feedback). + extraSchemas, err := getExtensionSchemas(ed.db) + if err != nil { + return fmt.Errorf("failed to query extension schemas: %w", err) + } + // getExtensionSchemas only sees the plan database. validateExtensionSchemas + // (in NewExternalDatabase) explicitly permits an extension present on only + // one side, so an extension installed on the plan database alone (e.g. for + // local testing convenience) is not itself an error - but it would be + // wrong to add its schema to search_path here: a bare type reference would + // then resolve during planning that the real target, lacking the + // extension entirely, could never resolve at apply time. Only trust an + // extension confirmed present on both sides (PR #608 review feedback). + // + // Known limitation, deliberately not "fixed" further (PR #608 review + // feedback): adding the managed schema to search_path exposes every bare + // reference to that schema's whole namespace, not just the confirmed + // extension member - if the plan database's copy of that schema also had + // some other object the target lacks, a bare reference could resolve on + // the plan side and then fail to apply on the real target. There is no + // narrower alternative that's actually safer: + // - Postgres's search_path has no per-object granularity - it's + // schema-wide or nothing. + // - Rewriting the desired-state SQL text to explicitly qualify bare + // extension-type references instead of expanding search_path would + // reintroduce exactly the ambiguity issue #354's design already + // rejected: text can't reliably distinguish a type reference from an + // identically-named column/parameter, so a rewrite risks silently + // qualifying the wrong token rather than just failing loudly. + // - Rejecting any dependency from the temp schema on a non-extension + // object in the managed schema isn't viable either: this whole + // scenario (ddms's domain schema) legitimately co-locates the + // extension with ordinary user tables/functions in the same schema, + // which is exactly the case #518 needs to keep working. + // The practical mitigation is operational, not code: keep the plan + // database's copy of an extension-hosting schema free of objects that + // don't also exist on the real target. + confirmedSchemas := filterConfirmedExtensionSchemas(extraSchemas, ed.targetExtensions) + setSearchPathSQL := fmt.Sprintf("SET search_path TO %s", buildDesiredStateSearchPath(ed.tempSchema, schema, confirmedSchemas)) if _, err := util.ExecContextWithLogging(ctx, conn, setSearchPathSQL, "set search_path for desired state"); err != nil { return fmt.Errorf("failed to set search_path: %w", err) } @@ -314,6 +393,47 @@ func quoteIdent(name string) string { return `"` + strings.ReplaceAll(name, `"`, `""`) + `"` } +// buildDesiredStateSearchPath builds the search_path used when applying +// desired-state SQL to the temporary comparison schema: the temp schema +// first, then the managed schema itself if it hosts an installed extension, +// then public - in that order, to mirror the real apply session's +// ", public" priority (see cmd/apply/apply.go). extensionSchemas is +// the extname -> schema map from getExtensionSchemas. Not every extension +// schema in the database is included, only the managed one, or a same-named +// object in public could shadow the extension type on the plan side while +// resolving the other way on the real target (PR #608 review feedback). +func buildDesiredStateSearchPath(tempSchema, schema string, extensionSchemas map[string]string) string { + parts := []string{quoteIdent(tempSchema)} + if schema != "public" { + for _, extSchema := range extensionSchemas { + if extSchema == schema { + parts = append(parts, quoteIdent(schema)) + break + } + } + } + parts = append(parts, "public") + return strings.Join(parts, ", ") +} + +// filterConfirmedExtensionSchemas keeps only the entries of planSchemas +// (extname -> schema, from getExtensionSchemas on the plan database) whose +// extension name also appears in targetExtensions (extname -> schema, from +// the real target). getExtensionSchemas only sees the plan database, and +// validateExtensionSchemas explicitly permits an extension present on only +// one side - so without this filter, a plan-only extension would let a bare +// type reference resolve during planning that the real target could never +// resolve at apply time (PR #608 review feedback). +func filterConfirmedExtensionSchemas(planSchemas, targetExtensions map[string]string) map[string]string { + confirmed := make(map[string]string, len(planSchemas)) + for extName, extSchema := range planSchemas { + if _, onTarget := targetExtensions[extName]; onTarget { + confirmed[extName] = extSchema + } + } + return confirmed +} + // detectMajorVersion queries the database to determine its PostgreSQL major version func detectMajorVersion(db *sql.DB) (int, error) { ctx := context.Background() diff --git a/internal/postgres/external_test.go b/internal/postgres/external_test.go new file mode 100644 index 00000000..4936f55f --- /dev/null +++ b/internal/postgres/external_test.go @@ -0,0 +1,110 @@ +package postgres + +import "testing" + +// buildDesiredStateSearchPath must order the managed schema before "public" +// (not after), so its priority matches the real apply session's +// ", public" search_path (see cmd/apply/apply.go). Putting it after +// public would let a same-named object in public shadow the managed +// schema's extension type during planning while the real target resolves +// it the other way around (PR #608 review feedback). +func TestBuildDesiredStateSearchPath(t *testing.T) { + tests := []struct { + name string + tempSchema string + schema string + extensionSchemas map[string]string + want string + }{ + { + name: "managed schema hosts an extension - inserted before public", + tempSchema: "pgschema_tmp_xxx", + schema: "domain", + extensionSchemas: map[string]string{"vector": "domain"}, + want: `"pgschema_tmp_xxx", "domain", public`, + }, + { + name: "managed schema does not host an extension", + tempSchema: "pgschema_tmp_xxx", + schema: "domain", + extensionSchemas: map[string]string{"vector": "other_schema"}, + want: `"pgschema_tmp_xxx", public`, + }, + { + name: "no extensions installed at all", + tempSchema: "pgschema_tmp_xxx", + schema: "domain", + extensionSchemas: map[string]string{}, + want: `"pgschema_tmp_xxx", public`, + }, + { + name: "managed schema is public - never duplicated", + tempSchema: "pgschema_tmp_xxx", + schema: "public", + extensionSchemas: map[string]string{"vector": "public"}, + want: `"pgschema_tmp_xxx", public`, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := buildDesiredStateSearchPath(tt.tempSchema, tt.schema, tt.extensionSchemas); got != tt.want { + t.Errorf("buildDesiredStateSearchPath(%q, %q, %v) = %q, want %q", tt.tempSchema, tt.schema, tt.extensionSchemas, got, tt.want) + } + }) + } +} + +// filterConfirmedExtensionSchemas must keep only extensions present in both +// maps. getExtensionSchemas only sees the plan database, and +// validateExtensionSchemas explicitly permits an extension present on only +// one side, so without this filter a plan-only extension would let a bare +// type reference resolve during planning that the real target could never +// resolve at apply time (PR #608 review feedback). +func TestFilterConfirmedExtensionSchemas(t *testing.T) { + tests := []struct { + name string + planSchemas map[string]string + targetExtensions map[string]string + want map[string]string + }{ + { + name: "extension present on both sides is kept", + planSchemas: map[string]string{"vector": "domain"}, + targetExtensions: map[string]string{"vector": "domain"}, + want: map[string]string{"vector": "domain"}, + }, + { + name: "plan-only extension is dropped", + planSchemas: map[string]string{"vector": "domain"}, + targetExtensions: map[string]string{}, + want: map[string]string{}, + }, + { + name: "target-only extension is irrelevant and absent from the result", + planSchemas: map[string]string{}, + targetExtensions: map[string]string{"vector": "domain"}, + want: map[string]string{}, + }, + { + name: "mixed - only the confirmed one survives", + planSchemas: map[string]string{"vector": "domain", "hstore": "exts"}, + targetExtensions: map[string]string{"vector": "domain"}, + want: map[string]string{"vector": "domain"}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := filterConfirmedExtensionSchemas(tt.planSchemas, tt.targetExtensions) + if len(got) != len(tt.want) { + t.Fatalf("filterConfirmedExtensionSchemas(%v, %v) = %v, want %v", tt.planSchemas, tt.targetExtensions, got, tt.want) + } + for k, v := range tt.want { + if got[k] != v { + t.Errorf("filterConfirmedExtensionSchemas(%v, %v) = %v, want %v", tt.planSchemas, tt.targetExtensions, got, tt.want) + } + } + }) + } +} diff --git a/ir/inspector.go b/ir/inspector.go index 0d018b02..68f89858 100644 --- a/ir/inspector.go +++ b/ir/inspector.go @@ -38,6 +38,38 @@ type Inspector struct { db *sql.DB queries *queries.Queries ignoreConfig *IgnoreConfig + // managedSchema is the logical schema pgschema is planning/applying, + // which can differ from targetSchema (the schema this particular + // BuildIR call actually introspects): when introspecting pgschema's own + // temporary comparison schema, targetSchema is that temp schema's + // literal name, but managedSchema is the real schema it stands in for + // (e.g. "domain"). Set via SetManagedSchema before calling BuildIR; + // defaults to targetSchema if never set, which is correct for every + // caller that introspects the schema it means to manage directly (dump, + // apply's current-state read, etc.) - only the desired-state/temp-schema + // path needs to set it explicitly. + managedSchema string + // extensionSchemas is the set of schema names hosting at least one + // installed extension on this connection, populated once per BuildIR call + // (see populateExtensionSchemas). Only ever consulted for managedSchema + // itself, never any other schema - see stripSameSchemaPrefix. + extensionSchemas map[string]bool + // extensionOwnedTypes is the set of "schema.typename" pairs that are + // confirmed, catalog-verified members of an installed extension (via + // pg_depend, deptype='e'; see populateExtensionSchemas). Consulted by + // stripSameSchemaPrefix and buildPrivileges, but only for a type + // qualified with managedSchema specifically - never an arbitrary schema + // that happens to host some extension elsewhere in the database, and + // never for a routine's real, permanent schema when it differs from + // managedSchema, where an extension-owned type from a different schema + // is a genuine cross-schema reference that must stay qualified (issue + // #518, PR #608 review feedback: this must be scoped to managedSchema, + // not "any known extension schema" or "any temp-schema introspection", + // or a genuinely cross-schema reference to another schema's extension + // type - e.g. a function in a managed "app" schema taking a + // "domain.vector" parameter - would also get incorrectly stripped while + // introspecting the temp schema). + extensionOwnedTypes map[string]bool } // NewInspector creates a new schema inspector with optional ignore configuration @@ -49,8 +81,19 @@ func NewInspector(db *sql.DB, ignoreConfig *IgnoreConfig) *Inspector { } } +// SetManagedSchema records the logical schema this Inspector's BuildIR call +// represents, when it differs from the schema literally being introspected +// (see the managedSchema field comment). Must be called before BuildIR. +func (i *Inspector) SetManagedSchema(schema string) { + i.managedSchema = schema +} + // BuildIR builds the schema IR from the database for a specific schema func (i *Inspector) BuildIR(ctx context.Context, targetSchema string) (*IR, error) { + if i.managedSchema == "" { + i.managedSchema = targetSchema + } + schema := NewIR() // Sequential prerequisites @@ -66,6 +109,10 @@ func (i *Inspector) BuildIR(ctx context.Context, targetSchema string) (*IR, erro return nil, fmt.Errorf("failed to build schemas: %w", err) } + if err := i.populateExtensionSchemas(ctx); err != nil { + return nil, fmt.Errorf("failed to query extension schemas: %w", err) + } + if err := i.buildTables(ctx, schema, targetSchema); err != nil { return nil, fmt.Errorf("failed to build tables: %w", err) } @@ -1066,7 +1113,7 @@ func (i *Inspector) buildFunctions(ctx context.Context, schema *IR, targetSchema Schema: schemaName, Name: functionName, Definition: definition, - ReturnType: i.safeInterfaceToString(fn.DataType), + ReturnType: i.stripSameSchemaPrefixFromReturnType(i.safeInterfaceToString(fn.DataType), schemaName), Language: i.safeInterfaceToString(fn.ExternalLanguage), Comment: comment, Parameters: parameters, @@ -1273,40 +1320,287 @@ func (i *Inspector) parseParametersFromSignature(signature string, routineSchema // This ensures consistent comparison between database inspection (which may return qualified // names) and source SQL (which typically uses unqualified names for same-schema types). func (i *Inspector) stripSameSchemaPrefix(typeName, routineSchema string) string { - if typeName == "" || routineSchema == "" { + if typeName == "" { return typeName } + if routineSchema != "" { + if stripped, ok := stripSchemaPrefixIfMatches(typeName, routineSchema); ok { + return stripped + } + } + + // Also strip a prefix matching managedSchema specifically - never any + // other schema, even one that hosts an installed extension - and only + // when the specific type is a catalog-verified extension member (see + // populateExtensionSchemas). + // + // Scoping to managedSchema (rather than "any known extension schema", or + // gating on routineSchema looking like a temp schema) matters because + // when introspecting pgschema's own temporary comparison schema, every + // object's routineSchema is that temp schema's literal name regardless + // of which real schema it represents - so a routine genuinely declared + // in some other, unmanaged schema (e.g. "app") that merely takes a + // parameter of managedSchema's extension type (e.g. "domain.vector") + // would also have routineSchema equal to the temp schema string during + // that introspection. Checking only against managedSchema itself (never + // looping every known extension schema, and never keying off + // routineSchema's shape) is what correctly leaves that genuine + // cross-schema reference qualified on both the temp and real-target + // sides, while still stripping a type that's genuinely in managedSchema + // wherever it's introspected from (issue #518, PR #608 review + // feedback). + // + // The extension-membership check matters because a schema can host both + // extension-owned and ordinary user-defined objects (e.g. an "exts" + // schema with both pgvector's "vector" and an unrelated user-defined + // "exts.status" type) - only the former is safe to treat as + // automatically resolvable without qualification (PR #608 review + // feedback). + if i.managedSchema != "" && i.extensionSchemas[i.managedSchema] { + if stripped, ok := stripSchemaPrefixIfMatches(typeName, i.managedSchema); ok { + // extensionOwnedTypes is keyed by the raw, unquoted pg_type.typname + // (see populateExtensionSchemas), so a quoted mixed-case type (e.g. + // stripped == "\"Vector\"") must be unquoted before the membership + // lookup - comparing the quoted form would never match and silently + // leave every quoted extension-member type qualified (PR #608 review + // feedback). stripped itself (still quoted, if it was quoted) is what + // gets returned, so a valid identifier is preserved either way. + baseType := unquoteIdentifier(strings.TrimSuffix(stripped, "[]")) + if i.extensionOwnedTypes[i.managedSchema+"."+baseType] { + return stripped + } + } + } + + // No matching prefix - return as-is (could be cross-schema type or already unqualified) + return typeName +} + +// stripSameSchemaPrefixFromReturnType applies stripSameSchemaPrefix to a +// function return type, decomposing SETOF and TABLE(...) forms the same way +// stripSchemaFromReturnType (ir/normalize.go) does, so an extension-owned +// type appearing directly, under SETOF, or as a TABLE(...) column type is +// normalized consistently with parameter types. Without this, a function +// declared as e.g. "RETURNS vector" would still compare as +// "RETURNS domain.vector" on the temp-schema side vs "RETURNS vector" on the +// real target side, causing the same spurious drop+recreate that motivated +// stripSameSchemaPrefix itself (PR #608 review feedback: return types were +// previously only normalized against the routine's own schema, later in +// normalizeFunction, which has no extension-schema knowledge). +func (i *Inspector) stripSameSchemaPrefixFromReturnType(returnType, routineSchema string) string { + if returnType == "" { + return returnType + } + + if len(returnType) > 6 && strings.EqualFold(returnType[:6], "SETOF ") { + rest := strings.TrimSpace(returnType[6:]) + stripped := i.stripSameSchemaPrefix(rest, routineSchema) + if stripped != rest { + return returnType[:6] + stripped + } + return returnType + } + + if strings.HasPrefix(returnType, "TABLE(") && strings.HasSuffix(returnType, ")") { + inner := returnType[6 : len(returnType)-1] + parts := splitTableColumns(inner) + var newParts []string + for _, part := range parts { + part = strings.TrimSpace(part) + if part == "" { + continue + } + name, typePart := splitColumnNameAndType(part) + if typePart != "" { + newParts = append(newParts, name+" "+i.stripSameSchemaPrefix(typePart, routineSchema)) + } else { + newParts = append(newParts, part) + } + } + return "TABLE(" + strings.Join(newParts, ", ") + ")" + } + + return i.stripSameSchemaPrefix(returnType, routineSchema) +} + +// stripSchemaPrefixIfMatches strips a "schema." or "\"schema\"." prefix from +// typeName if it matches schemaName, reporting whether a strip happened. +func stripSchemaPrefixIfMatches(typeName, schemaName string) (string, bool) { // Remove quotes from schema name for comparison - unquotedSchema := routineSchema - if strings.HasPrefix(routineSchema, `"`) && strings.HasSuffix(routineSchema, `"`) { - unquotedSchema = routineSchema[1 : len(routineSchema)-1] + unquotedSchema := schemaName + if strings.HasPrefix(schemaName, `"`) && strings.HasSuffix(schemaName, `"`) { + unquotedSchema = schemaName[1 : len(schemaName)-1] } // Handle quoted schema prefix: "schema".typename quotedPrefix := fmt.Sprintf(`"%s".`, unquotedSchema) if strings.HasPrefix(typeName, quotedPrefix) { - return typeName[len(quotedPrefix):] + return typeName[len(quotedPrefix):], true } // Handle unquoted schema prefix: schema.typename unquotedPrefix := unquotedSchema + "." if strings.HasPrefix(typeName, unquotedPrefix) { - return typeName[len(unquotedPrefix):] + return typeName[len(unquotedPrefix):], true } - // No matching prefix - return as-is (could be cross-schema type or already unqualified) - return typeName + return typeName, false +} + +// populateExtensionSchemas records which schemas host at least one installed +// extension on this connection, and which specific types are catalog-verified +// members of an extension (e.g. pgvector's "vector"), for use by +// stripSameSchemaPrefix and buildPrivileges. Extensions are database-wide, so +// this runs once per BuildIR call rather than per schema/target. +func (i *Inspector) populateExtensionSchemas(ctx context.Context) error { + schemaRows, err := i.db.QueryContext(ctx, + `SELECT DISTINCT n.nspname + FROM pg_catalog.pg_extension e + JOIN pg_catalog.pg_namespace n ON n.oid = e.extnamespace`) + if err != nil { + return err + } + defer schemaRows.Close() + + extensionSchemas := make(map[string]bool) + for schemaRows.Next() { + var schemaName string + if err := schemaRows.Scan(&schemaName); err != nil { + return err + } + extensionSchemas[schemaName] = true + } + if err := schemaRows.Err(); err != nil { + return err + } + + // pg_depend with deptype='e' identifies objects that are genuine members + // of an extension (as opposed to merely living in a schema an extension + // happens to also occupy - a schema can hold both). + typeRows, err := i.db.QueryContext(ctx, + `SELECT n.nspname, t.typname + FROM pg_catalog.pg_depend d + JOIN pg_catalog.pg_type t ON t.oid = d.objid + JOIN pg_catalog.pg_namespace n ON n.oid = t.typnamespace + WHERE d.deptype = 'e' + AND d.classid = 'pg_type'::regclass + AND d.refclassid = 'pg_extension'::regclass`) + if err != nil { + return err + } + defer typeRows.Close() + + extensionOwnedTypes := make(map[string]bool) + for typeRows.Next() { + var schemaName, typeName string + if err := typeRows.Scan(&schemaName, &typeName); err != nil { + return err + } + extensionOwnedTypes[schemaName+"."+typeName] = true + } + if err := typeRows.Err(); err != nil { + return err + } + + i.extensionSchemas = extensionSchemas + i.extensionOwnedTypes = extensionOwnedTypes + return nil } // stripSameSchemaPrefixFromList is stripSameSchemaPrefix for a comma-separated -// argument list such as pg_get_function_identity_arguments output. It strips a +// argument list such as pg_get_function_identity_arguments output (used for +// aggregate identity args/signatures - see buildAggregates). It strips a // schema qualifier only where an identifier token equal to the schema (bare or // quote_ident form) directly precedes a dot, so a schema that needs quoting // ("MySchema".v) normalizes like public.v while quoted names that merely -// contain the schema text ("public.foo") are left intact. +// contain the schema text ("public.foo") are left intact. Also applies +// managedSchema's extension-membership-aware stripping (same as +// stripExtensionMemberTypeQualifiers uses for privileges) - without this, an +// aggregate over an extension-owned type (e.g. "domain.vector") would key as +// "vector" on the real side but stay "domain.vector" on the temp side and be +// spuriously dropped/recreated (PR #608 review feedback). func (i *Inspector) stripSameSchemaPrefixFromList(list, schema string) string { - return StripSchemaQualifiers(list, schema) + return i.stripExtensionMemberTypeQualifiers(StripSchemaQualifiers(list, schema)) +} + +// stripExtensionMemberTypeQualifiers drops a "." qualifier +// from s only where the identifier immediately following it is a +// catalog-verified extension member of managedSchema specifically (see +// populateExtensionSchemas / extensionOwnedTypes) - never any other schema, +// even one that hosts some other installed extension, and never an ordinary +// object that merely lives in managedSchema without being an extension +// member. Scoping to managedSchema alone (rather than looping every known +// extension schema) is what keeps a genuinely cross-schema reference to +// another schema's extension type qualified, on both the temp-schema and +// real-target sides (PR #608 review feedback - see stripSameSchemaPrefix's +// comment for the full rationale, which applies identically here). +func (i *Inspector) stripExtensionMemberTypeQualifiers(s string) string { + if s == "" || i.managedSchema == "" || !i.extensionSchemas[i.managedSchema] || len(i.extensionOwnedTypes) == 0 { + return s + } + extSchema := i.managedSchema + + // A regex over raw text can't tell a genuine "." qualifier + // from the same bytes appearing inside an unrelated quoted identifier + // (e.g. a parameter literally named "domain.vector"), and can't match a + // quoted schema ("Domain".vector) at all. Use the same single-pass, + // quoted-identifier-aware tokenizer as StripSchemaQualifiers instead, + // with one token of lookahead to check extension membership before + // deciding whether to strip (PR #608 review feedback). + var out strings.Builder + out.Grow(len(s)) + pos := 0 + for pos < len(s) { + token, end := scanIdentToken(s, pos) + if isIdentToken(token) && end < len(s) && s[end] == '.' && unquoteIdentifier(token) == extSchema { + nextTok, nextEnd := scanIdentToken(s, end+1) + if isIdentToken(nextTok) && i.extensionOwnedTypes[extSchema+"."+unquoteIdentifier(nextTok)] { + out.WriteString(nextTok) + pos = nextEnd + continue + } + } + out.WriteString(token) + pos = end + } + return out.String() +} + +// scanIdentToken extracts one token from s starting at pos, using the same +// rules as StripSchemaQualifiers: a quoted identifier (honoring "" escapes), +// a run of identifier characters, or a single other byte. Returns the token +// and the position right after it. +func scanIdentToken(s string, pos int) (token string, end int) { + start := pos + switch { + case s[pos] == '"': + pos++ + for pos < len(s) { + if s[pos] == '"' { + if pos+1 < len(s) && s[pos+1] == '"' { + pos += 2 + continue + } + pos++ + break + } + pos++ + } + case isIdentChar(s[pos]): + for pos < len(s) && isIdentChar(s[pos]) { + pos++ + } + default: + pos++ + } + return s[start:pos], pos +} + +// isIdentToken reports whether token (as returned by scanIdentToken) is an +// identifier - quoted or bare - rather than a single punctuation byte. +func isIdentToken(token string) bool { + return token != "" && (token[0] == '"' || isIdentChar(token[0])) } // oidToTypeName maps PostgreSQL type OIDs to standard SQL type names. @@ -2212,6 +2506,19 @@ func (i *Inspector) buildPrivileges(ctx context.Context, schema *IR, targetSchem objectName := row.ObjectName.String objectType := row.ObjectType.String privilegeType := row.PrivilegeType.String + + // FUNCTION/PROCEDURE object_name is rendered by pg_get_function_identity_arguments + // directly in SQL (see GetPrivilegesForSchema), so it never goes through + // stripSameSchemaPrefix the way parseParametersFromSignature does. Strip it + // here too: routine's own schema always, plus managedSchema's extension-member + // types specifically (stripExtensionMemberTypeQualifiers is itself scoped to + // managedSchema - see its comment and stripSameSchemaPrefix's for why a + // genuinely cross-schema or non-member privilege signature must keep its + // qualifier). + if objectType == "FUNCTION" || objectType == "PROCEDURE" { + objectName = StripSchemaQualifiers(objectName, targetSchema) + objectName = i.stripExtensionMemberTypeQualifiers(objectName) + } owner := row.Owner.String isGrantable := row.IsGrantable.Valid && row.IsGrantable.Bool diff --git a/ir/inspector_extension_schema_test.go b/ir/inspector_extension_schema_test.go new file mode 100644 index 00000000..820d41d6 --- /dev/null +++ b/ir/inspector_extension_schema_test.go @@ -0,0 +1,338 @@ +package ir + +import "testing" + +// stripSameSchemaPrefix must strip a prefix matching either the routine's own +// schema, or (only for the schema this Inspector's managedSchema is set to, +// and only for a catalog-verified extension member of it) an +// extension-owned type (issue #518): when introspecting pgschema's own +// temporary comparison schema, every function's routineSchema is that temp +// schema's literal name regardless of which real schema it represents, so an +// extension-owned type's real schema qualifier (e.g. "domain.vector") never +// matches routineSchema and would otherwise survive unstripped - causing the +// temp-schema side and the real-target side of a diff to compare as +// different function signatures. +// +// Scoping the fallback to managedSchema specifically (never "any known +// extension schema") matters because a function that is itself part of the +// schema being managed can still take a parameter whose type genuinely lives +// in a different schema (e.g. a function declared in a managed "app" schema +// taking a "domain.vector" parameter, where pgvector lives in "domain", not +// "app"). Since every function's routineSchema during temp-schema +// introspection is the same temp-schema literal regardless of which real +// schema it represents, a check keyed off "is routineSchema a temp schema" +// alone can't distinguish that genuine cross-schema reference from a +// same-managed-schema one - only checking against managedSchema itself can +// (a regression caught in PR #608 review, HIGH severity: the original +// temp-schema-prefix-only gate stripped this case incorrectly). +// +// It also must NOT fire for a type that merely lives in managedSchema +// without being an extension member (a schema can host both extension-owned +// and ordinary user-defined objects) - another regression caught in PR #608 +// review, addressed by checking extensionOwnedTypes rather than +// extensionSchemas alone. +func TestStripSameSchemaPrefix_ExtensionSchemaAware(t *testing.T) { + tests := []struct { + name string + typeName string + routineSchema string + managedSchema string + extensionSchemas map[string]bool + extensionOwnedTypes map[string]bool + want string + }{ + { + name: "strips routine's own schema, no extension schemas known", + typeName: "domain.mytype", + routineSchema: "domain", + managedSchema: "domain", + want: "mytype", + }, + { + name: "temp-schema introspection, type is a confirmed member of managedSchema", + typeName: "domain.vector", + routineSchema: "pgschema_tmp_20260101_000000_abcd1234", + managedSchema: "domain", + extensionSchemas: map[string]bool{"domain": true}, + extensionOwnedTypes: map[string]bool{"domain.vector": true}, + want: "vector", + }, + { + name: "quoted extension schema qualifier", + typeName: `"domain".vector`, + routineSchema: "pgschema_tmp_xxx", + managedSchema: "domain", + extensionSchemas: map[string]bool{"domain": true}, + extensionOwnedTypes: map[string]bool{"domain.vector": true}, + want: "vector", + }, + { + name: "array of a confirmed extension member type", + typeName: "domain.vector[]", + routineSchema: "pgschema_tmp_xxx", + managedSchema: "domain", + extensionSchemas: map[string]bool{"domain": true}, + extensionOwnedTypes: map[string]bool{"domain.vector": true}, + want: "vector[]", + }, + { + name: "quoted mixed-case extension member type - membership check must unquote", + typeName: `domain."Vector"`, + routineSchema: "pgschema_tmp_xxx", + managedSchema: "domain", + extensionSchemas: map[string]bool{"domain": true}, + extensionOwnedTypes: map[string]bool{"domain.Vector": true}, + want: `"Vector"`, + }, + { + name: "quoted mixed-case array of a confirmed extension member type", + typeName: `domain."Vector"[]`, + routineSchema: "pgschema_tmp_xxx", + managedSchema: "domain", + extensionSchemas: map[string]bool{"domain": true}, + extensionOwnedTypes: map[string]bool{"domain.Vector": true}, + want: `"Vector"[]`, + }, + { + // A function declared in the managed "app" schema takes a parameter + // whose type genuinely lives in "domain" (a different schema, which + // hosts pgvector) - real-target-side introspection: routineSchema + // equals managedSchema ("app"), which does not itself host any + // extension, so the fallback never even considers "domain". + name: "genuine cross-schema reference within the managed schema's own function - real side", + typeName: "domain.vector", + routineSchema: "app", + managedSchema: "app", + extensionSchemas: map[string]bool{"domain": true}, + extensionOwnedTypes: map[string]bool{"domain.vector": true}, + want: "domain.vector", + }, + { + // Same scenario, but introspected via the temp schema - HIGH + // severity regression from PR #608 review: a gate based only on + // "is routineSchema a temp schema" would strip this incorrectly, + // since every function's routineSchema is the same temp-schema + // literal here regardless of which real schema it belongs to. + // Checking against managedSchema ("app", which hosts no + // extension) rather than looping every known extension schema is + // what keeps this qualified, matching the real side above. + name: "genuine cross-schema reference within the managed schema's own function - temp side", + typeName: "domain.vector", + routineSchema: "pgschema_tmp_xxx", + managedSchema: "app", + extensionSchemas: map[string]bool{"domain": true}, + extensionOwnedTypes: map[string]bool{"domain.vector": true}, + want: "domain.vector", + }, + { + name: "managed schema hosts an extension, but this specific type is not a member", + typeName: "exts.status", + routineSchema: "pgschema_tmp_xxx", + managedSchema: "exts", + extensionSchemas: map[string]bool{"exts": true}, + extensionOwnedTypes: map[string]bool{"exts.vector": true}, + want: "exts.status", + }, + { + name: "cross-schema type unaffected - schema matches neither routine nor managed schema", + typeName: "utils.hstore", + routineSchema: "pgschema_tmp_xxx", + managedSchema: "domain", + extensionSchemas: map[string]bool{"domain": true}, + extensionOwnedTypes: map[string]bool{"domain.vector": true}, + want: "utils.hstore", + }, + { + name: "already-bare type unaffected", + typeName: "vector", + routineSchema: "pgschema_tmp_xxx", + managedSchema: "domain", + extensionSchemas: map[string]bool{"domain": true}, + extensionOwnedTypes: map[string]bool{"domain.vector": true}, + want: "vector", + }, + { + name: "empty type name", + typeName: "", + routineSchema: "domain", + managedSchema: "domain", + extensionSchemas: map[string]bool{"domain": true}, + extensionOwnedTypes: map[string]bool{"domain.vector": true}, + want: "", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + insp := &Inspector{managedSchema: tt.managedSchema, extensionSchemas: tt.extensionSchemas, extensionOwnedTypes: tt.extensionOwnedTypes} + if got := insp.stripSameSchemaPrefix(tt.typeName, tt.routineSchema); got != tt.want { + t.Errorf("stripSameSchemaPrefix(%q, %q) = %q, want %q", tt.typeName, tt.routineSchema, got, tt.want) + } + }) + } +} + +// stripSameSchemaPrefixFromReturnType must decompose SETOF and TABLE(...) +// return types the same way ir/normalize.go's stripSchemaFromReturnType +// does, applying the managedSchema-scoped stripSameSchemaPrefix to each +// contained type rather than a single top-level prefix check. Without this, +// "RETURNS vector" would compare as "domain.vector" (temp side) vs "vector" +// (real target side) and spuriously trigger a drop+recreate (PR #608 review +// feedback). +func TestStripSameSchemaPrefixFromReturnType(t *testing.T) { + insp := &Inspector{ + managedSchema: "domain", + extensionSchemas: map[string]bool{"domain": true}, + extensionOwnedTypes: map[string]bool{"domain.vector": true}, + } + + tests := []struct { + name string + returnType string + routineSchema string + want string + }{ + { + name: "direct extension-owned return type in temp schema", + returnType: "domain.vector", + routineSchema: "pgschema_tmp_xxx", + want: "vector", + }, + { + name: "SETOF extension-owned return type in temp schema", + returnType: "SETOF domain.vector", + routineSchema: "pgschema_tmp_xxx", + want: "SETOF vector", + }, + { + name: "TABLE(...) column referencing an extension-owned type in temp schema", + returnType: "TABLE(id integer, embedding domain.vector)", + routineSchema: "pgschema_tmp_xxx", + want: "TABLE(id integer, embedding vector)", + }, + { + name: "direct extension-owned return type on the managed schema's own real routine", + returnType: "domain.vector", + routineSchema: "domain", + want: "vector", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := insp.stripSameSchemaPrefixFromReturnType(tt.returnType, tt.routineSchema); got != tt.want { + t.Errorf("stripSameSchemaPrefixFromReturnType(%q, %q) = %q, want %q", tt.returnType, tt.routineSchema, got, tt.want) + } + }) + } + + t.Run("genuine cross-schema return type on a differently-managed schema is preserved", func(t *testing.T) { + appInsp := &Inspector{ + managedSchema: "app", + extensionSchemas: map[string]bool{"domain": true}, + extensionOwnedTypes: map[string]bool{"domain.vector": true}, + } + for _, routineSchema := range []string{"app", "pgschema_tmp_xxx"} { + if got := appInsp.stripSameSchemaPrefixFromReturnType("domain.vector", routineSchema); got != "domain.vector" { + t.Errorf("stripSameSchemaPrefixFromReturnType(%q, %q) = %q, want %q", "domain.vector", routineSchema, got, "domain.vector") + } + } + }) +} + +// stripExtensionMemberTypeQualifiers is buildPrivileges' equivalent of +// stripSameSchemaPrefix's extension-membership check, operating on a whole +// function/procedure identity-arguments string instead of a single type, and +// scoped to managedSchema the same way. +func TestStripExtensionMemberTypeQualifiers(t *testing.T) { + insp := &Inspector{ + managedSchema: "domain", + extensionSchemas: map[string]bool{"domain": true}, + extensionOwnedTypes: map[string]bool{"domain.vector": true, "domain.Vector": true}, + } + + tests := []struct { + name string + in string + want string + }{ + { + name: "strips a confirmed extension member type in a signature", + in: "vector_search(query_embedding domain.vector)", + want: "vector_search(query_embedding vector)", + }, + { + name: "preserves a non-member type in the same extension schema", + in: "f(x domain.status)", + want: "f(x domain.status)", + }, + { + name: "leaves an unrelated schema untouched", + in: "g(x utils.hstore)", + want: "g(x utils.hstore)", + }, + { + name: "strips a quoted mixed-case confirmed extension member type", + in: `h(x domain."Vector")`, + want: `h(x "Vector")`, + }, + { + // A parameter literally NAMED the quoted identifier "domain.vector" + // (a valid, if unusual, Postgres identifier - dots are permitted + // inside quotes). A raw-text regex can't tell this apart from a + // genuine schema.type qualifier since it doesn't track quoting + // context; the tokenizer treats the whole quoted string as one + // atomic token and never looks inside it (PR #608 review feedback). + name: "quoted identifier that merely contains dotted text is left untouched", + in: `f("domain.vector" integer)`, + want: `f("domain.vector" integer)`, + }, + { + // A quoted schema qualifier - the old regex only ever matched the + // bare, unquoted schema name literally, so this never stripped at + // all (PR #608 review feedback). + name: "quoted schema qualifier on a confirmed member type", + in: `k(x "domain".vector)`, + want: `k(x vector)`, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := insp.stripExtensionMemberTypeQualifiers(tt.in); got != tt.want { + t.Errorf("stripExtensionMemberTypeQualifiers(%q) = %q, want %q", tt.in, got, tt.want) + } + }) + } + + t.Run("genuine cross-schema privilege signature on a differently-managed schema is preserved", func(t *testing.T) { + appInsp := &Inspector{ + managedSchema: "app", + extensionSchemas: map[string]bool{"domain": true}, + extensionOwnedTypes: map[string]bool{"domain.vector": true}, + } + in := "vector_search(query_embedding domain.vector)" + if got := appInsp.stripExtensionMemberTypeQualifiers(in); got != in { + t.Errorf("stripExtensionMemberTypeQualifiers(%q) = %q, want unchanged %q", in, got, in) + } + }) +} + +// stripSameSchemaPrefixFromList (used for aggregate identity args and +// signatures) must also apply managedSchema's extension-membership-aware +// stripping, not just the basic same-schema strip - otherwise an aggregate +// over an extension-owned type keys as "vector" on the real side but stays +// "domain.vector" on the temp side and is spuriously dropped/recreated (PR +// #608 review feedback). +func TestStripSameSchemaPrefixFromList(t *testing.T) { + insp := &Inspector{ + managedSchema: "domain", + extensionSchemas: map[string]bool{"domain": true}, + extensionOwnedTypes: map[string]bool{"domain.vector": true}, + } + in := "domain.vector" + want := "vector" + if got := insp.stripSameSchemaPrefixFromList(in, "pgschema_tmp_xxx"); got != want { + t.Errorf("stripSameSchemaPrefixFromList(%q, %q) = %q, want %q", in, "pgschema_tmp_xxx", got, want) + } +}