Skip to content

fix: render per-privilege column lists in grants (#638) - #661

Merged
tianzhou merged 2 commits into
pgplex:mainfrom
shehjaddev:fix/issue-638-column-privileges
Oct 27, 2026
Merged

tianzhou merged 2 commits into
pgplex:mainfrom
shehjaddev:fix/issue-638-column-privileges

Conversation

@shehjaddev

Copy link
Copy Markdown
Contributor

Fixes #638

Column privileges of several types on the same columns were rendered as GRANT INSERT, UPDATE (a, b) .... PostgreSQL attaches a column list only to the privilege right before it, so that grants INSERT on the whole table.

The column list is now attached to every privilege, e.g. GRANT INSERT (a, b), UPDATE (a, b) ON TABLE t TO app;. This applies to GRANT, REVOKE, REVOKE GRANT OPTION FOR and WITH GRANT OPTION statements, in both plan and dump (both use the same generator).

Added testdata/diff/privilege/issue_638_multi_privilege_column_grant, covering narrowing a multi-privilege column grant (revoke + grant) and adding privileges to an existing column grant. It fails without the fix.

@greptile-apps

greptile-apps Bot commented Oct 16, 2026 •

Copy link
Copy Markdown
Contributor

RetriggerView in GreptileConfidence Score: 4/5

[Medium impact] Changes how column-level database privileges are rendered in SQL.

The PR appears safe to merge, with a non-blocking gap in grant-option test coverage.

Findings

  1. P2 Grant-option changes lack tests ▶

Summary

Attaches the column list to every privilege in column-level GRANT and REVOKE statements.

  • Each column privilege now keeps its own column list.

Reviews (1) · Last reviewed commit: "fix: render per-privilege column lists i..." · Reviewed by Greptile

Comment on lines +12 to +14
GRANT INSERT (a), UPDATE (a) ON t TO app_user;

GRANT INSERT (x), SELECT (x), UPDATE (x) ON u TO app_user;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Grant-option changes lack tests

The new regression covers creating, dropping, and adding column privileges, but not the changed grant-option branches. Existing grant-option fixtures use table privileges, so they would not catch a regression in these column-specific statements. Add cases with multiple privileges on the same columns that add and remove WITH GRANT OPTION.

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!

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Approval recommended

The focused formatting change consistently covers the affected SQL paths, with regression fixtures for the principal privilege transitions and no unresolved findings.

No open issues 🎉

What changed in this PR

Fixes #638 by attaching a column list to each privilege in generated SQL, preventing unintended table-wide grants in pgschema’s plan and dump output.

Changes:

  • Shares column-list formatting across GRANT, REVOKE, and grant-option statements while preserving quoting and sorting.
  • Adds regression fixtures for narrowing column grants, adding privileges, and changing grant options.
File Description
testdata/​diff/​privilege/​issue_638_multi_privilege_column_grant/​plan.txt Expected human-readable migration plan.
testdata/​diff/​privilege/​issue_638_multi_privilege_column_grant/​plan.sql Expected SQL plan with per-privilege column lists.
testdata/​diff/​privilege/​issue_638_multi_privilege_column_grant/​plan.json Expected structured plan and operation metadata.
testdata/​diff/​privilege/​issue_638_multi_privilege_column_grant/​old.sql Initial column privileges and grant options.
testdata/​diff/​privilege/​issue_638_multi_privilege_column_grant/​new.sql Desired privileges covering the tested transitions.
testdata/​diff/​privilege/​issue_638_multi_privilege_column_grant/​diff.sql Expected migration statements.
internal/​diff/​column_privilege.go Applies column lists individually to every generated privilege.

Review effort: Balanced

@tianzhou tianzhou left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

Thanks for the contribution.

@tianzhou
tianzhou merged commit a18e2b9 into pgplex:main Oct 27, 2026
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Column privileges of several types render as GRANT INSERT, UPDATE (cols), which grants INSERT on the whole table

3 participants