You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
pgschema's temporary comparison schema during plan generation only sets search_path to "<temp>", public, so a bare reference to an extension-owned type installed in a non-public schema (e.g. pgvector's vector in a domain schema) fails with type ... does not exist when the desired-state SQL is applied there. This is a deeper, apply-time failure mode that #544's pre-flight schema-consistency check doesn't address even when the plan and target databases agree on the extension's schema.
Three related fixes
All rooted in the same cause — pgschema's per-object code only knows about the target schema, not about extension schemas:
external.go: extend the temp schema's search_path to include every schema hosting an installed extension on the plan database.
inspector.go: stripSameSchemaPrefix also strips a prefix matching any known extension schema, not just the routine's own schema — needed because a function's routineSchemais the temp schema name when introspecting the "new" side of a diff, so an extension-owned type's qualifier would otherwise survive stripped on one side and not the other, causing a spurious drop+recreate.
inspector.go: buildPrivileges applies the same extension-schema-aware stripping (via the existing StripSchemaQualifiers tokenizer already used for aggregate signatures) to FUNCTION/PROCEDURE object_name, which bypasses stripSameSchemaPrefix entirely by being rendered via pg_get_function_identity_arguments() directly in SQL.
Testing
Verified locally end-to-end against a real pgvector-backed schema: the unfixed binary fails with type vector does not exist on plan; the fixed binary plans/applies cleanly, and a subsequent plan returns No changes detected.
go build ./..., go vet ./..., and go test ./... all pass, no regressions.
…mp schema
pgschema's temporary comparison schema during plan generation only sets
search_path to "<temp>, public", so a bare reference to an extension-owned
type installed in a non-public schema (e.g. pgvector's "vector" in a
"domain" schema) fails with "type ... does not exist" when the desired-state
SQL is applied there. This is issue pgplex#518's deeper, apply-time failure mode,
which PR pgplex#544's pre-flight schema-consistency check does not address even
when the plan and target databases agree on the extension's schema.
Three related fixes, all rooted in the same cause (pgschema's per-object
code only knows about the target schema, not about extension schemas):
- external.go: extend the temp schema's search_path to include every schema
hosting an installed extension on the plan database.
- inspector.go: stripSameSchemaPrefix also strips a prefix matching any
known extension schema, not just the routine's own schema - needed
because a function's routineSchema *is* the temp schema name when
introspecting the "new" side of a diff, so an extension-owned type's
qualifier would otherwise survive stripped on one side and not the other,
causing a spurious drop+recreate.
- inspector.go: buildPrivileges applies the same extension-schema-aware
stripping (via the existing StripSchemaQualifiers tokenizer already used
for aggregate signatures) to FUNCTION/PROCEDURE object_name, which
bypasses stripSameSchemaPrefix entirely by being rendered via
pg_get_function_identity_arguments() directly in SQL.
This PR is not safe to merge until extension-schema stripping is limited so ordinary cross-schema routine and privilege signatures retain resolvable type identities.
This PR allows desired-state SQL in an external plan database to resolve types hosted by extensions outside public, then normalizes extension-qualified routine and privilege signatures across the desired and target IRs.
Adds installed extension schemas to the external temporary schema’s search path.
Loads extension-schema metadata during IR construction.
Strips extension-schema qualifiers from routine parameters and privilege identities.
Adds focused unit coverage for temporary-schema parameter normalization.
The normalization currently also removes necessary qualifiers from ordinary cross-schema routines and should be narrowed before merge.
Diagram
sequenceDiagram
participant DB as PostgreSQL
participant Inspector
participant IR
participant Renderer as Dump / Migration Renderer
participant Apply as Target Apply Session
DB->>Inspector: app.fn(domain.vector)
Inspector->>DB: Load installed extension schemas
DB-->>Inspector: domain
Inspector->>IR: Strip domain. and store fn(vector)
IR->>Renderer: Render normalized parameter
Renderer-->>Apply: CREATE/DROP/GRANT ... fn(vector)
Note over Apply: search_path = app, public
Apply-->>Apply: vector may be unresolved or resolve incorrectly
The reason will be displayed to describe this comment to others. Learn more.
🟡 Changes recommended
Current schema-wide normalization and search-path handling can resolve or emit incorrect type identities.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Extends external plan generation to resolve extension-owned types from non-public schemas and normalize routine identities.
Changes:
Adds extension schemas to temporary database resolution.
Normalizes routine parameters and privileges.
Adds focused normalization tests.
File summaries
File
Description
ir/inspector.go
Collects extension schemas and normalizes routine identities.
ir/inspector_extension_schema_test.go
Tests extension-schema parameter normalization.
internal/postgres/external.go
Extends temporary-schema search_path.
Review details
Suppressed comments (3)
internal/postgres/external.go:177
getExtensionSchemas normally includes plpgsql in pg_catalog. Explicitly placing pg_catalog after the temporary and public schemas disables PostgreSQL's implicit catalog-first lookup, allowing objects in those schemas to shadow built-ins during desired-state execution. Skip pg_catalog here so it remains implicitly first.
An extension is attached to individual catalog objects, not every object in its installation schema. If exts hosts an extension and also a user-defined exts.status, this strips a cross-schema function parameter to status; generated CREATE/DROP SQL then loses the required qualifier, and overload keys can collide. Track actual extension-member types via pg_depend and preserve qualification for non-members.
for extSchema := range i.extensionSchemas {
if stripped, ok := stripSchemaPrefixIfMatches(typeName, extSchema); ok {
return stripped
ir/inspector.go:2378
This repeats the schema-wide ownership assumption for privilege identities. A privilege on f(exts.status) is rewritten to f(status) whenever any extension is installed in exts, so generated GRANT/REVOKE SQL can target no function (or the wrong overload) when exts is not on the target search path. Strip only catalog-confirmed extension-member types, and retain qualifiers for ordinary cross-schema types.
for extSchema := range i.extensionSchemas {
objectName = StripSchemaQualifiers(objectName, extSchema)
Addresses Greptile and Copilot review findings on the original commit:
- external.go: only add the managed schema itself to the temp schema's
search_path (and only if it hosts an installed extension), instead of
every extension schema in the database. Adding unrelated extension
schemas let bare cross-schema types resolve during planning that the
real apply session (search_path "<schema>, public") could never resolve,
and could explicitly relocate pg_catalog out of its default
implicit-first search position when a bundled extension (e.g. plpgsql)
lives there.
- inspector.go: stripSameSchemaPrefix and buildPrivileges now only strip
an extension schema qualifier when routineSchema/targetSchema is
pgschema's own temp comparison schema (pgschema_tmp_*) - never for a
routine's real, permanent schema, where an extension-owned type from a
different schema is a genuine cross-schema reference that must stay
qualified in generated DDL.
- inspector.go: added extensionOwnedTypes, populated via pg_depend
(deptype='e'), so stripping only applies to catalog-verified extension
member types, not to every type that happens to live in a schema an
extension also occupies (a schema can host both).
- inspector.go: function return types (direct, SETOF, and TABLE(...)) now
get the same extension-aware stripping as parameters, via a new
stripSameSchemaPrefixFromReturnType - previously only parameters were
covered, so a function returning an extension-owned type directly would
still spuriously diff.
Extended unit tests to cover all of the above, including the specific
schema-hosts-extension-but-type-isn't-a-member and
genuine-cross-schema-must-stay-qualified regressions the reviews caught.
go build/vet/test ./... all pass. Re-verified end-to-end against the real
ddms model files: plan/apply/re-plan still comes back clean after these
changes.
Thanks for the reviews — both raised valid points, verified each against the actual code (not just at face value) before fixing. Pushed a follow-up commit addressing all of them:
Greptile: cross-schema qualifiers lost
Confirmed by tracing Function.Parameters into generateFunctionSQL in internal/diff/function.go — the same field the stripping mutated is used directly to render CREATE/DROP/GRANT DDL, not just for comparison. Fixed by gating the extension-schema fallback in stripSameSchemaPrefix/buildPrivileges on routineSchema/targetSchema starting with pgschema_tmp_ (the deterministic temp-schema naming pattern already used for this exact purpose elsewhere in the codebase, e.g. ir/normalize.go, internal/diff/table.go). Verified structurally that plan.go always introspects the desired state from exactly one such temp schema on both the embedded and external plan-DB paths, so this gate is sound rather than a naming-convention hack.
Agreed — appending every extension schema in the database let bare cross-schema types resolve during planning that the real apply session ("<schema>, public" only) could never resolve, and since plpgsql lives in pg_catalog by default on nearly every Postgres database, this also explicitly relocated pg_catalog out of its implicit-first search position on nearly every run. Fixed by only adding the managed schema itself to search_path, and only if it hosts an installed extension — matches the actual use case (extension lives in the schema being planned) and sidesteps both problems.
Copilot: schema-level check treats co-located objects as extension members
Also agreed — a schema can host both extension members and unrelated user-defined objects (e.g. exts.vector from pgvector next to a hand-written exts.status), and the original fix stripped both indiscriminately. Added Inspector.extensionOwnedTypes, populated via pg_depend (deptype='e'), so stripping now requires catalog-confirmed extension membership, not just schema co-location. Applied both to stripSameSchemaPrefix (single-type case) and a new stripExtensionMemberTypeQualifiers helper for buildPrivileges (whole-signature-string case).
Copilot: return types not covered
Correct — the parameter-focused fix never touched return types at all; normalizeFunction in ir/normalize.go only strips a return type against the routine's own schema, with no extension-schema awareness, so a function directly RETURNS vector would still spuriously diff. Added stripSameSchemaPrefixFromReturnType, decomposing SETOF/TABLE(...) forms the same way stripSchemaFromReturnType does, applying the (now correctly gated) stripSameSchemaPrefix to each contained type.
Testing
Extended ir/inspector_extension_schema_test.go with cases for every point above (genuine cross-schema preserved, non-member type in an extension schema preserved, array-of-member-type, SETOF/TABLE(...) return types, privilege-signature equivalent). go build/go vet/go test ./... all green. Re-ran the full local end-to-end validation against the real pgvector-backed schema this was built against — plan/apply/re-plan still comes back clean after these changes.
Addresses a fresh round of Copilot review feedback on the previous fix:
- external.go: the managed schema was appended after "public" in the temp
schema's search_path ("<temp>", public, "<managed>"), but the real apply
session uses "<managed>, public" - managed schema first. If public had
an object with the same bare name as the managed schema's extension
type, the plan side would resolve it against public while apply
resolves it against the managed schema. Reordered to
"<temp>", "<managed>", public to match apply's priority. Extracted the
construction logic into buildDesiredStateSearchPath, a pure function
now directly unit-tested without needing a live DB connection.
- ir/inspector.go: extensionOwnedTypes is keyed by the raw, unquoted
pg_type.typname, but the membership check in stripSameSchemaPrefix's
extension loop compared it against the still-quoted stripped value, so
a quoted mixed-case extension member type never matched and stayed
qualified on the temp side - the same spurious-recreate class of bug
this fix exists to prevent. stripExtensionMemberTypeQualifiers had the
same root cause, plus its regex never matched quoted identifiers at
all. Both now unquote before the membership check while still
returning the properly-quoted identifier as the strip result.
Added regression tests for both (quoted mixed-case member types in both
helpers, all 4 search_path ordering cases). go build/vet clean, full
go test ./... green, no regressions. Re-verified end-to-end against the
real ddms model files (both the raw pgschema CLI and the actual deploydb
orchestrator pipeline) - still comes back "No changes detected." after
these changes.
…p-schema introspection
Addresses a fresh round of review feedback:
- ir/inspector.go (HIGH): gating the extension-schema fallback on
routineSchema looking like a temp schema wasn't precise enough. Every
function's routineSchema during temp-schema introspection is the same
temp-schema literal regardless of which real schema it represents, so a
function genuinely declared in a managed "app" schema that takes a
parameter from a different schema's extension (e.g. "domain.vector",
where pgvector lives in "domain" not "app") would also get stripped -
a real cross-schema reference losing its qualifier.
Fixed by threading the logical managed schema through the Inspector
(new SetManagedSchema/managedSchema field, defaulting to targetSchema
when unset) and scoping stripSameSchemaPrefix/buildPrivileges'
extension handling to managedSchema specifically, never any other
known extension schema and never based on routineSchema's shape. This
is simpler than the previous temp-schema-prefix gate and subsumes it:
on the real target side routineSchema always equals managedSchema by
construction (BuildIR only returns objects within targetSchema), so
the primary same-schema check already handles it; the fallback now
only ever considers the one schema that's actually correct.
cmd/util/connection.go's GetIRFromDatabase gained a managedSchema
parameter (empty string defaults to the schema being introspected);
only cmd/plan/plan.go's desired-state IR call passes a real value
(config.Schema, distinct from the temp schema it actually connects to).
- internal/postgres/external.go (MEDIUM): getExtensionSchemas only sees
the plan database, and validateExtensionSchemas explicitly permits an
extension present on only one side. Without cross-checking against
what's confirmed on the target too, an extension installed on the plan
database alone would let a bare type reference resolve during planning
that the real target could never resolve at apply time - plan
succeeds, apply fails. Fixed by storing ExternalDatabaseConfig's
existing TargetExtensions on ExternalDatabase and filtering
getExtensionSchemas' result through it (new filterConfirmedExtensionSchemas)
before building the search path.
Extended unit tests for both: a function within a differently-managed
schema keeps a genuine cross-schema qualifier through stripSameSchemaPrefix,
stripSameSchemaPrefixFromReturnType, and stripExtensionMemberTypeQualifiers
alike; filterConfirmedExtensionSchemas covers plan-only, target-only, and
both-sides-confirmed cases. go build/vet clean, full go test ./... green.
Re-verified end-to-end against the real ddms model files (raw pgschema CLI
and the actual deploydb orchestrator pipeline) - still "No changes detected."
after these changes.
Adding the whole managed schema to search_path exposes every object in that schema, not just the confirmed extension members. For example, if the plan database has a stale domain.status type but the target does not, desired SQL using bare status now validates against the plan-only type and produces a plan that fails on the target. This also breaks the temporary-schema isolation described by this provider. Please either qualify only catalog-verified extension objects or reject dependencies from the temp schema to non-extension objects in the managed schema.
Normalize extension types in aggregate signatures
ir/inspector.go:1362
The extension-aware normalization is not applied to aggregate argument lists. buildAggregates still sends AggregateIdentityArgs and AggregateSignature through stripSameSchemaPrefixFromList, which only knows the temporary routine schema. Thus an aggregate over domain.vector is keyed as vector on the real side but domain.vector on the temp side and is spuriously dropped/recreated. Extend the token-aware list normalizer to apply the same managed-schema extension-membership rule.
This issue also appears on line 2468 of the same file.
…arch_path limitation
Addresses a fifth round of review feedback:
- ir/inspector.go (MEDIUM): stripExtensionMemberTypeQualifiers used a raw
regex, which can't distinguish a genuine "<schema>.<type>" qualifier
from the same bytes appearing inside an unrelated quoted identifier
(e.g. a parameter literally named "domain.vector" - dots are valid
inside quotes), and never matched a quoted schema ("Domain".vector) at
all since it only checked for the unquoted schema name as literal text.
Replaced with a single-pass, quoted-identifier-aware tokenizer (same
scanning rules as the existing StripSchemaQualifiers) with one token of
lookahead to check extension membership before stripping, rather than
matching raw text.
- ir/inspector.go (MEDIUM, "previously missed"): stripSameSchemaPrefixFromList
(used for aggregate identity args and signatures via buildAggregates)
only applied the basic same-schema strip, never the extension-membership
fallback - so an aggregate over an extension-owned type keyed as "vector"
on the real side but stayed "domain.vector" on the temp side, spuriously
dropped/recreated. Now also applies stripExtensionMemberTypeQualifiers,
matching the pattern already used for privileges. (AggregateReturnType
already went through stripSameSchemaPrefix directly, which already
covers this via the managedSchema fix - only the list-based helper was
missing it.)
- internal/postgres/external.go ("previously missed"): documented, rather
than further "fixed", the fact that adding the managed schema to
search_path exposes its whole namespace, not just the confirmed
extension member. Considered and rejected two alternatives: rewriting
desired-state SQL text to explicitly qualify bare extension-type
references (reintroduces the exact type-vs-column-name ambiguity issue
pgplex#354's design already avoided - a regex/text approach can't reliably
tell a type reference from an identically-named column apart, so it
risks silently qualifying the wrong token instead of just failing
loudly), and rejecting any temp-schema dependency on a non-extension
object in the managed schema (breaks the legitimate case this fix
targets - ddms's domain schema intentionally co-locates the vector
extension with ordinary tables/functions). The practical mitigation is
operational: keep the plan database's copy of an extension-hosting
schema free of objects absent from the real target.
Added tests for the two exact SQL-parsing edge cases (quoted identifier
containing dotted text; quoted schema qualifier) and for the aggregate
list helper. go build/vet clean, full go test ./... green. Re-verified
end-to-end against the real ddms model files (raw pgschema CLI and the
actual deploydb orchestrator pipeline) - still "No changes detected."
Normalize managed-schema casts in parameter default expressions
ir/inspector.go:1371
The managed-schema normalization is not applied to parameter default expressions. For a routine in domain such as f(v vector DEFAULT '[]'::vector), pg_get_function_arguments() deparses the cast as domain.vector on both databases; current-state normalization strips it using the real routine schema, while the temp-side routine schema is the temporary schema, so the desired default remains qualified. parametersEqual then reports a change on every plan. Normalize extension-member type casts in parameter defaults using the logical managed schema as well.
This issue also appears on line 2518 of the same file.
Qualify regclass literals in extension-membership queries
ir/inspector.go:1488
Qualify these regclass literals as the other extension-membership queries do (for example, ir/queries/queries.sql:2121-2127). The inspector inherits the role's search path, so if pg_catalog is explicitly ordered after a user schema containing a relation named pg_type or pg_extension, these unqualified literals resolve to the wrong OIDs and extension-member detection silently fails.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
tianzhou
left a comment
Fixes #607
pgschema's temporary comparison schema during plan generation only sets
search_pathto"<temp>", public, so a bare reference to an extension-owned type installed in a non-public schema (e.g. pgvector'svectorin adomainschema) fails withtype ... does not existwhen the desired-state SQL is applied there. This is a deeper, apply-time failure mode that #544's pre-flight schema-consistency check doesn't address even when the plan and target databases agree on the extension's schema.Three related fixes
All rooted in the same cause — pgschema's per-object code only knows about the target schema, not about extension schemas:
external.go: extend the temp schema'ssearch_pathto include every schema hosting an installed extension on the plan database.inspector.go:stripSameSchemaPrefixalso strips a prefix matching any known extension schema, not just the routine's own schema — needed because a function'sroutineSchemais the temp schema name when introspecting the "new" side of a diff, so an extension-owned type's qualifier would otherwise survive stripped on one side and not the other, causing a spurious drop+recreate.inspector.go:buildPrivilegesapplies the same extension-schema-aware stripping (via the existingStripSchemaQualifierstokenizer already used for aggregate signatures) to FUNCTION/PROCEDUREobject_name, which bypassesstripSameSchemaPrefixentirely by being rendered viapg_get_function_identity_arguments()directly in SQL.Testing
Verified locally end-to-end against a real pgvector-backed schema: the unfixed binary fails with
type vector does not existonplan; the fixed binary plans/applies cleanly, and a subsequentplanreturnsNo changes detected.go build ./...,go vet ./..., andgo test ./...all pass, no regressions.