Skip to content

sql: pluralize the object type in abbreviated grants - #38438

Merged
sjwiesman merged 1 commit into
MaterializeInc:mainfrom
sjwiesman:sql-parser-abbreviated-grant-plural
Aug 24, 2026
Merged

sql: pluralize the object type in abbreviated grants#38438
sjwiesman merged 1 commit into
MaterializeInc:mainfrom
sjwiesman:sql-parser-abbreviated-grant-plural

Conversation

@sjwiesman

@sjwiesman sjwiesman commented Aug 24, 2026

Copy link
Copy Markdown
Contributor

Motivation

The grammar spells the plural of a network policy as the single keyword
POLICIES, and the parser maps it straight to ObjectType::NetworkPolicy.
Rendering went the other way through the singular display, NETWORK POLICY,
with a bare S appended:

f.write_node(&self.object_type);   // "NETWORK POLICY"
f.write_str("S TO ");              // -> "NETWORK POLICYS TO"

So a statement parsed from ON POLICIES redisplays as ON NETWORK POLICYS,
which is wrong twice over. The plural grammar does not use the word NETWORK at
all, and POLICY does not pluralize to POLICYS. The result does not reparse,
which breaks the display/parse round trip the parser is supposed to hold.

Every other object type escapes this because its singular display is already the
plural stem and English only needs the S: TABLE to TABLES, SCHEMA to
SCHEMAS, CLUSTER to CLUSTERS. NETWORK POLICY is the one irregular spelling
in the set.

This same defect was already found and fixed once, for the
GRANT ... ON ALL POLICIES IN SCHEMA ... form. That fix is
write_grant_object_type_plural, and test_grant_revoke_all_policies_roundtrips
is its regression test, filed off a parse_display_roundtrip fuzz crash on
GRANT CREATE ON ALL POLICIES TO j. The helper landed with three callers, all
inside GrantTargetSpecification. The two abbreviated statement formatters were
never routed through it, and the acl testdata has no POLICIES case, so the
second instance survived.

What this does

Points AbbreviatedGrantStatement and AbbreviatedRevokeStatement at the helper
that already handles this, two lines each. No new logic.

Where the bad text is observable

Statement logging records two SQL columns, and they are computed differently.
sql keeps the raw text as the user typed it unless
StatementKind::is_sensitive covers the statement, so for an
ALTER DEFAULT PRIVILEGES it is untouched. redacted_sql is rendered from the
AST unconditionally:

redacted_sql: stmt.map(|s| s.to_ast_string_redacted()).unwrap_or_default(),

So every such statement is stored with the mis-pluralized text, reachable as:

SELECT DISTINCT redacted_sql
FROM mz_internal.mz_sql_text
WHERE redacted_sql LIKE '%NETWORK POLICYS%';

and through everything built on it: mz_sql_text_redacted,
mz_recent_sql_text, mz_recent_activity_log, and
mz_recent_activity_log_redacted.

This matters most for mz_monitor_redacted consumers, since the redacted column
is the only SQL text they can read, and for these statements it hands them
something that will not reparse. Anyone replaying or diffing redacted history
gets a syntax error rather than the statement that ran.

The planner is unaffected. sql::plan::statement::acl reads the AST and never
reformats it, so execution has always been correct. Only the rendered text is
wrong.

Tests

Added test_alter_default_privileges_policies_roundtrips in
src/sql-parser/tests/sqlparser_common.rs, mirroring the sibling test for the
ON ALL POLICIES form. It asserts both the grant and the revoke direction render
ON POLICIES, never POLICYS, and round-trip through
assert_display_roundtrips. Reverting the change makes it fail with
ON NETWORK POLICYS.

Release notes

This release will fix the SQL text recorded for ALTER DEFAULT PRIVILEGES ... ON POLICIES statements in statement logging, which was stored in the redacted_sql
column as the unparseable ON NETWORK POLICYS.

🤖 Generated with Claude Code

`AbbreviatedGrantStatement` and `AbbreviatedRevokeStatement` pluralized an object type by appending `S`, so a rule naming a network policy rendered as `ON NETWORK POLICYS`, which the grammar spells `ON POLICIES` and will not parse. Route both through `write_grant_object_type_plural`, the helper the surrounding grant statements already use.
@sjwiesman
sjwiesman requested a review from a team as a code owner August 24, 2026 20:10

@SangJunBak SangJunBak 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 fix!

@sjwiesman
sjwiesman merged commit 96c4e5a into MaterializeInc:main Aug 24, 2026
87 checks 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.

2 participants