Conversation
…376) When using an external database for plan generation, pg_get_function_identity_arguments() qualifies user-defined argument types with the temporary schema name (e.g., pgschema_tmp_20260326_161823_31f3dbda.my_input[]). This leaked into GRANT/REVOKE statements, causing apply to fail with "schema does not exist". Fix: normalize privilege ObjectName to strip the schema prefix from function/procedure argument types, consistent with how other schema objects are normalized. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Fixes a planning/apply failure where temporary desired-state schema qualifiers (e.g., pgschema_tmp_...) leak into GRANT/REVOKE ... ON FUNCTION/PROCEDURE signatures, producing invalid SQL at apply time.
Changes:
- Normalize
Privilege.ObjectNameandRevokedDefaultPrivilege.ObjectNamefor FUNCTION/PROCEDURE objects by stripping same-schema qualifiers from argument type references during IR normalization. - Add a regression fixture for issue #376 validating the generated plan SQL no longer references the temporary schema.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
ir/normalize.go |
Adds privilege/revoked-default-privilege signature normalization to remove temp schema qualifiers from function/procedure argument types. |
testdata/diff/privilege/issue_376_grant_revoke_temp_schema_leak/plan.txt |
New expected plan output for the regression case. |
testdata/diff/privilege/issue_376_grant_revoke_temp_schema_leak/plan.sql |
New expected DDL showing unqualified argument type usage. |
testdata/diff/privilege/issue_376_grant_revoke_temp_schema_leak/plan.json |
New expected plan JSON with corrected object paths/SQL. |
testdata/diff/privilege/issue_376_grant_revoke_temp_schema_leak/old.sql |
Old-state SQL fixture for the regression test. |
testdata/diff/privilege/issue_376_grant_revoke_temp_schema_leak/new.sql |
New-state SQL fixture adding the GRANT/REVOKE statements that previously failed. |
testdata/diff/privilege/issue_376_grant_revoke_temp_schema_leak/diff.sql |
Expected diff SQL for the regression case. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Greptile SummaryThis PR fixes a bug where temporary schema names (e.g. Confidence Score: 5/5Safe to merge — the fix is narrowly scoped, pointer semantics are correct, and all remaining findings are minor style/test suggestions. The only open findings are P2 style observations (use ir/normalize.go — minor style point around argument-splitting approach and lack of unit test for the new helper. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["normalizeSchema\nschema.Name = pgschema_tmp_xxx"] --> B["Iterate schema.Privileges"]
B --> C{"ObjectType ==\nFUNCTION or PROCEDURE?"}
C -- No --> D["Skip"]
C -- Yes --> E["normalizePrivilegeObjectName(objectName, schemaName)"]
E --> F{"Has parens?"}
F -- No --> G["Return unchanged"]
F -- Yes --> H["Extract funcName + args"]
H --> I{"args empty?"}
I -- Yes --> G
I -- No --> J["Split args by ', '"]
J --> K["For each part: strip schema prefix"]
K --> L{"Any part changed?"}
L -- No --> G
L -- Yes --> M["Reconstruct: funcName + cleaned args"]
M --> N["priv.ObjectName updated\ne.g. f_test(p_items my_input[])"]
A --> O["Iterate schema.RevokedDefaultPrivileges"]
O --> P{"ObjectType ==\nFUNCTION or PROCEDURE?"}
P -- No --> D
P -- Yes --> E
Reviews (1): Last reviewed commit: "fix: strip temp schema prefix from GRANT..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
ir/normalize.go
Outdated
| parts := strings.Split(args, ", ") | ||
| changed := false | ||
| for i, part := range parts { | ||
| if strings.Contains(part, prefix) { | ||
| parts[i] = strings.ReplaceAll(part, prefix, "") | ||
| changed = true | ||
| } | ||
| } |
There was a problem hiding this comment.
splitTableColumns helper
The rest of normalize.go already has a splitTableColumns helper that splits argument lists by top-level commas while respecting nested parentheses (e.g. numeric(10, 2)). This function splits with a plain strings.Split(args, ", ") instead.
In practice the round-trip is safe (split and join with the same ", " delimiter preserves the original string), and pg_get_function_identity_arguments() rarely returns precision-modified types. But it is an inconsistency with the established pattern in this file, and could produce subtly wrong intermediate parts slices if the type list ever contains ", " inside type modifiers—even though the final joined string remains correct.
Consider using splitTableColumns for consistency and robustness:
parts := splitTableColumns(args)Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| // normalizePrivilegeObjectName strips schema qualifiers from argument types | ||
| // in function/procedure signatures used in GRANT/REVOKE statements. | ||
| // e.g., "f_test(p_items pgschema_tmp_20260326_161823_31f3dbda.my_input[])" → "f_test(p_items my_input[])" | ||
| func normalizePrivilegeObjectName(objectName, schemaName string) string { | ||
| if objectName == "" || schemaName == "" { | ||
| return objectName | ||
| } | ||
|
|
||
| // Find the argument list in the signature: name(args) | ||
| parenOpen := strings.Index(objectName, "(") | ||
| parenClose := strings.LastIndex(objectName, ")") | ||
| if parenOpen < 0 || parenClose < 0 || parenClose <= parenOpen { | ||
| return objectName | ||
| } | ||
|
|
||
| funcName := objectName[:parenOpen] | ||
| args := objectName[parenOpen+1 : parenClose] | ||
|
|
||
| if args == "" { | ||
| return objectName | ||
| } | ||
|
|
||
| prefix := schemaName + "." | ||
|
|
||
| // Split args by comma, strip schema prefix from each type reference, rejoin | ||
| parts := strings.Split(args, ", ") | ||
| changed := false | ||
| for i, part := range parts { | ||
| if strings.Contains(part, prefix) { | ||
| parts[i] = strings.ReplaceAll(part, prefix, "") | ||
| changed = true | ||
| } | ||
| } | ||
|
|
||
| if !changed { | ||
| return objectName | ||
| } | ||
|
|
||
| return funcName + "(" + strings.Join(parts, ", ") + ")" | ||
| } |
There was a problem hiding this comment.
normalizePrivilegeObjectName
Every other normalization helper in this package has a corresponding table-driven unit test in normalize_test.go (e.g. TestStripSchemaPrefixFromBody). The new normalizePrivilegeObjectName function is exercised only through the integration test data file, which makes it harder to catch regressions and edge-case inputs (empty args, no-arg function, array types, multi-arg, prefix not present) without spinning up the full diff pipeline.
Consider adding a TestNormalizePrivilegeObjectName table-driven test alongside the existing helpers in normalize_test.go.
…legeObjectName Address review feedback: - Use splitTableColumns helper for arg splitting to correctly handle nested parentheses in type modifiers (e.g., numeric(10, 2)) - Add table-driven unit tests covering edge cases: empty inputs, no-arg functions, array types, multi-arg, temp schema prefix, precision types Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Summary
pg_get_function_identity_arguments()qualifies user-defined argument types with the temporary schema name (e.g.,pgschema_tmp_20260326_161823_31f3dbda.my_input[])ERROR: schema "pgschema_tmp_..." does not existPrivilege.ObjectNameandRevokedDefaultPrivilege.ObjectNameto strip schema prefixes from function/procedure argument types during IR normalizationFixes #376
Test plan
testdata/diff/privilege/issue_376_grant_revoke_temp_schema_leak/test casePGSCHEMA_TEST_FILTER="privilege/issue_376" go test -v ./internal/diff -run TestDiffFromFilesPGSCHEMA_TEST_FILTER="privilege/" go test -v ./cmd -run TestPlanAndApply🤖 Generated with Claude Code