Skip to content

Fix GRANTED BY ordering in propagated REVOKE statements - #8766

Open
ibrahim halatci (ihalatci) wants to merge 3 commits into
mainfrom
ihalatci-fix-granted-by-revoke-order
Open

Fix GRANTED BY ordering in propagated REVOKE statements#8766
ibrahim halatci (ihalatci) wants to merge 3 commits into
mainfrom
ihalatci-fix-granted-by-revoke-order

Conversation

@ihalatci

@ihalatci ibrahim halatci (ihalatci) commented Aug 12, 2026

Copy link
Copy Markdown
Contributor

Summary

  • emit GRANTED BY before CASCADE/RESTRICT when deparsing propagated shared-object GRANT/REVOKE statements
  • cover both RESTRICT and CASCADE with schema privilege propagation checks on coordinator and worker

Root cause

PostgreSQL grammar places GRANTED BY grantor before the optional CASCADE/RESTRICT behavior clause. AppendGrantSharedSuffix() emitted those clauses in the opposite order, producing worker SQL such as REVOKE ... RESTRICT GRANTED BY .... The coordinator completed its local revoke, while workers rejected the propagated command with a syntax error, leaving privileges inconsistent.

The role path already had the correct ordering (deparse_role_stmts.c), which is what makes this an oversight in the shared-object path rather than a deliberate difference.

Runtime negative control

With the one-line ordering change reverted and the harness re-run, the propagated statement is rejected by the worker:

 REVOKE USAGE ON SCHEMA granted_by_schema FROM role_2 GRANTED BY role_1 RESTRICT;
+ERROR:  syntax error at or near "GRANTED"
+CONTEXT:  while executing command on localhost:xxxxx
 REVOKE USAGE ON SCHEMA granted_by_schema FROM role_3 GRANTED BY role_1 CASCADE;
+ERROR:  syntax error at or near "GRANTED"

Because the worker rejects it, the whole REVOKE aborts and the privilege survives on both coordinator and worker. So unfixed, REVOKE ... GRANTED BY ... CASCADE|RESTRICT on a distributed shared object is not merely mis-ordered text — it is unexecutable. With the fix applied the same test passes with zero diff.

Cross-version validation

A single expected/grant_on_schema_propagation.out satisfies all supported majors — no version-specific variants were needed:

PostgreSQL focused test
16.14 ok - grant_on_schema_propagation
17.10 ok - grant_on_schema_propagation
18.4 ok - grant_on_schema_propagation

Scope

This PR is limited to the shared-object deparser path (database, schema, function, sequence, and foreign server). Table grantor behavior is a separate defect with a separate fix and is intentionally out of scope here.
Tracks #8759.

AppendGrantSharedSuffix() appended the GRANTED BY clause after
CASCADE/RESTRICT, but the GRANT/REVOKE grammar requires the opposite
order. Any REVOKE that combined both clauses was therefore deparsed into
an invalid statement and failed on the workers with

    ERROR:  syntax error at or near "GRANTED"

leaving the privilege in place on the shards while the coordinator had
already revoked it.

The role deparser already emits these clauses in the correct order; only
the object path (database, schema, function, sequence, foreign server)
was affected.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 61c1aeb4-1b56-4313-a35e-d65b59a557de
@codecov

codecov Bot commented Aug 12, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 88.69%. Comparing base (3164f62) to head (f445a71).

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #8766   +/-   ##
=======================================
  Coverage   88.69%   88.69%           
=======================================
  Files         289      289           
  Lines       64889    64889           
  Branches     8180     8180           
=======================================
+ Hits        57553    57554    +1     
+ Misses       4973     4970    -3     
- Partials     2363     2365    +2     
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

ibrahim halatci (ihalatci) added a commit that referenced this pull request Aug 22, 2026
## Summary

Preserve a table `GRANT`/`REVOKE` statement's explicit `GRANTED BY` role
when Citus deparses and propagates the command to physical shards.

This PR is intentionally limited to table-grantor serialization in
`commands/grant.c`. It is separate from #8766, which fixes shared-object
`REVOKE` clause ordering in the deparser. The two are complementary
halves of the same report and must not be conflated.

Tracks #8759
Umbrella tracking: #8597

## Root cause

`PreprocessGrantStmt()` hand-builds the worker DDL string rather than
routing through the deparser, and it never emitted `GrantStmt.grantor`.
The explicit grantor was therefore silently dropped on the way to every
worker.

PostgreSQL 19 (commit `dd1398f1`) widened `GRANTED BY` to accept **any
role the acting role inherits**, not just `current_user`. Once the
grantor is dropped from the worker command, each worker independently
runs `select_best_grantor()` and can pick a different role than the
coordinator did. The result is a coordinator/shard ACL divergence.

On PG16-18 the same statement requires `grantor = current_user`, so the
omission is unobservable there. This is why the defect is only
reachable, and only testable, on PG19.

## Impact

Once the ACLs diverge, a later `REVOKE` that names the coordinator's
grantor matches nothing on the shards. Concretely, the coordinator
reports the privilege as revoked while the physical shards still grant
it — a silent privilege leak that `has_table_privilege()` on the
coordinator will not reveal.

## Fix

`commands/grant.c` only, +16/-4. Builds a `grantedBy` fragment when
`grantStmt->grantor` is set and appends it to both format strings. In
`REVOKE` it lands after the grantee list and before
`CASCADE`/`RESTRICT`, which is the same ordering #8766 establishes for
shared objects, so the two scopes agree by construction.

`GrantStmt.grantor` has existed since PG14, so no version gate is
needed; the code is inert on PG16-18 where the field cannot be set to
anything but the current role.

## Test design

The scenario is deliberately discriminating. A third-party role owns the
table and is **never named as grantor**, so the non-discriminating
`owner == grantor` case cannot mask a failure. Two eligible grantors are
inherited by the acting role, and the statement explicitly names the one
the workers would *not* choose on their own.

Coverage:
- coordinator vs physical-shard `aclexplode(relacl).grantor` parity,
read through `run_command_on_shards()`
- selective-revoke parity via `has_table_privilege()` on coordinator and
shards
- a quoted grantor role name (`"Grant Owner"`), exercising
`RoleSpecString(..., true)`
- `WITH GRANT OPTION` and the matching `REVOKE GRANT OPTION FOR`

Tests are gated on `server_version_ge_19` and are pure additions: zero
removed lines in either `pg19.sql` or `pg19.out`.

## Negative control

Reverting `grant.c` to stock, rebuilding, reinstalling and re-running
turns `pg19` red. Verified twice in independent cycles, the second time
with a binary-level gate confirming the built `citus.so` actually
matched the intended source in each phase (`strings citus.so | grep -c
'GRANT %s ON %s TO %s%s%s'` -> stock 0, fixed 1).

Four assertions flip, and **every one of them is shard-side**. All
coordinator rows are byte-identical between the two runs and appear only
as unchanged context:

| assertion | stock (broken) | with fix |
|---|---|---|
| shard grantor | `pg19_grantor_a` | `pg19_grantor_b` |
| shard privilege after selective revoke | `f` (coordinator reads `t`) |
`t` |
| shard grant option after `GRANT ... WITH GRANT OPTION` |
`pg19_grantor_a, grantable=true` | `"Grant Owner", grantable=true` |
| shard grant option after `REVOKE GRANT OPTION FOR` | `pg19_grantor_a,
grantable=false` | `"Grant Owner", grantable=false` |

The sharpest single line is row 2: `coordinator_privilege = t` sits two
lines above `shard_privilege = f` as unchanged context in the same diff
hunk.

**Disclosed limitation, stated plainly:** rows 3 and 4 discriminate on
grantor *identity* only, not on the `grantable` flag. Unfixed, the shard
ACL entry and the un-attributed revoke both resolve to `pg19_grantor_a`
and therefore coincide, so the grant option is still correctly dropped.
Rows 1 and 2 carry the proof; rows 3 and 4 are corroborating.

## Validation

- base is exactly `origin/pg19-support` @ `db67cd7e2` with zero
intervening delta, so the validated tree is the pushed tree
- 3 files, +305/-4; the two test files are append-only
- `citus_indent --check` clean (exit 0, no `FAIL` lines)
- full-cluster `pg19` regression on a 3-node PostgreSQL 19beta2 harness:
`ok` with the fix, zero diff
- `expected/pg19.out` is harness-generated, never hand-written, and
re-verified byte-identical to a freshly generated `results/pg19.out`

Co-authored-by: Ibrahim Halatci <ihalatci@microsoft.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
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.

Preserve GRANTED BY semantics for distributed object privileges

3 participants