Propagate GRANTED BY when deparsing table grants - #8767
Open
ibrahim halatci (ihalatci) wants to merge 1 commit into
Open
Propagate GRANTED BY when deparsing table grants#8767ibrahim halatci (ihalatci) wants to merge 1 commit into
ibrahim halatci (ihalatci) wants to merge 1 commit into
Conversation
PreprocessGrantStmt() builds the DDL it sends to the workers by hand and never emitted GrantStmt.grantor, so the GRANTED BY clause was silently dropped on the way to the shards. This was unreachable before PostgreSQL 19: until then GRANTED BY only accepted the current user, so the grantor the coordinator recorded was always the role we connect to the workers as, and each worker independently arrived at the same answer. PostgreSQL 19 (commit dd1398f1) relaxed the clause to accept any role whose privileges the current user inherits. Once the named grantor can differ from the connecting role, the workers no longer have enough information to reproduce the coordinator's ACL: they run select_best_grantor() on their own and may legitimately pick a different eligible role. The result is a coordinator/shard ACL divergence that survives a revoke. Granting the same privilege twice under two different grantors and then revoking only one of them leaves the coordinator reporting has_table_privilege() = true while the shards report false, so the grantee's queries fail with a permission error even though the coordinator says the privilege is held. The mirror construction also leaks privileges on the shards after a coordinator-side revoke. Emit GRANTED BY in the deparsed statement so the workers record the same grantor the coordinator did. The clause goes after WITH GRANT OPTION for GRANT, and after the grantee list but before CASCADE/RESTRICT for REVOKE. GrantStmt.grantor has existed since PG14, so no version gate is needed; the field is simply never set on PG16-18. The regression test uses a third-party table owner and two eligible grantors, both inherited by the acting role. The extra owner matters: if the named grantor were also the role select_best_grantor() would have chosen anyway, the test would pass even with the bug present. It asserts the grantor recorded in the shard ACLs via aclexplode(), not just that some privilege exists, and checks has_table_privilege() parity between the coordinator and the shards after a selective revoke. A grantor whose name requires quoting covers RoleSpecString(), and a grant carrying WITH GRANT OPTION plus a matching REVOKE GRANT OPTION FOR exercise both slots the grantor clause shares with the grant option. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## pg19-support #8767 +/- ##
================================================
- Coverage 88.74% 88.71% -0.04%
================================================
Files 289 289
Lines 65065 65068 +3
Branches 8200 8202 +2
================================================
- Hits 57743 57726 -17
- Misses 4957 4978 +21
+ Partials 2365 2364 -1 🚀 New features to boost your workflow:
|
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Summary
Preserve a table
GRANT/REVOKEstatement's explicitGRANTED BYrole when Citus deparses and propagates the command to physical shards on PostgreSQL 19.This PR is intentionally limited to table-grantor serialization. It is separate from #8766 and does not address that issue's scope.
Tracks #8759
Umbrella tracking: #8597
Problem and impact
PostgreSQL 19 allows an acting role to name an eligible inherited role as the grantor. Citus previously omitted that grantor while rebuilding the worker command, allowing each shard to select a different best grantor from the coordinator.
The regression reproduces this with a third-party table owner and two eligible grantors inherited by the acting role. Without the fix, coordinator and physical-shard ACLs diverge. A selective revoke can then leave the coordinator reporting the privilege while a shard rejects access, or leave shard privileges after the coordinator-side privilege is revoked.
Coverage
has_table_privilege()parityWITH GRANT OPTIONand matchingREVOKE GRANT OPTION FORGRANTED BYPG19 validation
git diff --check origin/pg19-support...HEADcitus_indent --quiet --checkforsrc/backend/distributed/commands/grant.csrc/test/regress/citus_tests/run_test.py pg19(all 7 scheduled tests passed)