Skip to content

feat: 1/7 expose PostgreSQL role parser surface#25521

Open
tabVersion wants to merge 4 commits intomainfrom
ralph/rbac-split-01-parser-surface
Open

feat: 1/7 expose PostgreSQL role parser surface#25521
tabVersion wants to merge 4 commits intomainfrom
ralph/rbac-split-01-parser-surface

Conversation

@tabVersion
Copy link
Copy Markdown
Contributor

@tabVersion tabVersion commented Apr 28, 2026

I hereby agree to the terms of the RisingWave Labs, Inc. Contributor License Agreement.

What's changed and what's your intention?

This PR is 1/7 in the PostgreSQL-style RBAC alignment stack tracked by #25528. It extracts the parser-facing and statement-surface part from the full RBAC alignment work in #25449 so reviewers can validate the SQL grammar, AST, display output, parser tests, and minimal frontend compile glue before the storage/meta/frontend/session/catalog semantics land in later PRs.

User-facing SQL surface added by this PR

Users and downstream RBAC implementation PRs can now rely on RisingWave recognizing and formatting these PostgreSQL-style role-management statements at the parser/AST layer:

  • role lifecycle syntax:
    • CREATE ROLE ...
    • ALTER ROLE ...
    • DROP ROLE ...
  • supported role option spellings needed by the RBAC stack, including:
    • LOGIN / NOLOGIN
    • INHERIT / NOINHERIT
    • CREATEDB / NOCREATEDB
    • CREATEROLE / NOCREATEROLE
    • existing RisingWave user/admin/superuser option aliases
  • role-membership syntax:
    • GRANT role_name TO member_name
    • GRANT role_name TO member_name WITH ADMIN OPTION
    • GRANT role_name TO member_name WITH INHERIT TRUE|FALSE, SET TRUE|FALSE
    • GRANT ... GRANTED BY ...
    • REVOKE role_name FROM member_name [RESTRICT|CASCADE]
    • REVOKE ADMIN|INHERIT|SET OPTION FOR role_name FROM member_name
    • REVOKE ... GRANTED BY ...
  • session-role syntax:
    • SET ROLE role_name
    • SET ROLE NONE
    • SET SESSION ROLE ...
    • SET LOCAL ROLE ...
    • RESET ROLE

Example SQL

The intended SQL surface can be understood through examples like:

-- Role lifecycle parser surface.
CREATE ROLE analyst WITH NOLOGIN INHERIT;
ALTER ROLE analyst WITH CREATEROLE;
DROP ROLE IF EXISTS old_analyst;

-- Role membership parser surface.
GRANT analyst TO marketing_user;
GRANT analyst TO marketing_user WITH ADMIN OPTION;
GRANT analyst TO marketing_user WITH INHERIT FALSE, SET FALSE;
GRANT analyst TO marketing_user GRANTED BY current_user;

-- Role membership revoke parser surface.
REVOKE analyst FROM marketing_user;
REVOKE ADMIN OPTION FOR analyst FROM marketing_user;
REVOKE INHERIT OPTION FOR analyst FROM marketing_user;
REVOKE SET OPTION FOR analyst FROM marketing_user;
REVOKE analyst FROM marketing_user GRANTED BY current_user;

-- Session role parser surface.
SET ROLE analyst;
SET LOCAL ROLE analyst;
SET ROLE NONE;
RESET ROLE;

These examples are meant to document the parser / AST / formatter contract introduced by this PR. The executable RBAC behavior for role storage, authorization checks, session-role effects, and catalog visibility is implemented by the downstream PRs in #25528.

The parser testdata also documents the normalized formatter output for the new statements, so later PRs can implement execution semantics against a stable SQL surface.

Stack / review context

Intentional compatibility boundary

This PR is not the full RBAC runtime implementation by itself. It intentionally does not add the authoritative role-membership storage, meta-side grant/revoke/drop behavior, frontend role dispatch/cache behavior, SET ROLE session semantics, or PostgreSQL catalog compatibility. Those pieces are split across #25522, #25523, #25524, #25525, #25526, and #25527.

Until the full stack lands, the user-facing guarantee of this PR is the parser/AST/display surface and compile-safe frontend boundary for the new statement variants, not complete executable PostgreSQL RBAC behavior.

Tests / verification

Checklist

  • I have written necessary rustdoc comments.
  • I have added necessary unit tests and integration tests.
  • I have added test labels as necessary.
  • I have added fuzzing tests or opened an issue to track them.
  • My PR contains breaking changes.
  • My PR changes performance-critical code, so I will run (micro) benchmarks and present the results.
  • I have checked the Release Timeline and Currently Supported Versions to determine which release branches I need to cherry-pick this PR into.

Documentation

  • My PR needs documentation updates.
Release note

This PR is the parser-surface slice of RisingWave's PostgreSQL-style RBAC alignment stack. It makes RisingWave recognize and format PostgreSQL-style role-management SQL such as CREATE ROLE, ALTER ROLE, DROP ROLE, role-membership GRANT/REVOKE with membership options and GRANTED BY, plus SET ROLE / RESET ROLE.

The full executable RBAC behavior is intentionally split into follow-up PRs tracked by #25528, so this PR should be read as compatibility groundwork for the SQL surface rather than complete PostgreSQL RBAC runtime support by itself.

This starts the RBAC split stack with parser and display support for PostgreSQL role syntax while keeping frontend behavior behind compile-safe unsupported paths. The later stack layers add storage, meta authority, frontend behavior, session semantics, and catalog compatibility.\n\nConstraint: New parser enum variants must not break downstream exhaustive matches before behavior lands.\nRejected: Include full frontend role handling in this parser PR | would collapse the planned review boundary.\nConfidence: high\nScope-risk: narrow\nTested: cargo fmt --check; cargo test -p risingwave_sqlparser granted_by --test sqlparser_common; cargo check -p risingwave_frontend\nNot-tested: Full workspace CI and runtime SQL behavior; covered by later stack PRs
Copy link
Copy Markdown

@capy-ai capy-ai Bot left a comment

Choose a reason for hiding this comment

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

Added 1 comment

Comment thread src/sqlparser/src/parser.rs
@tabVersion tabVersion changed the title rbac: expose PostgreSQL role parser surface feat: 1/7 expose PostgreSQL role parser surface Apr 28, 2026
@github-actions github-actions Bot added type/feature Type: New feature. and removed Invalid PR Title labels Apr 28, 2026
tabVersion added a commit that referenced this pull request Apr 29, 2026
Avoid treating option-keyword role names as REVOKE <option> OPTION FOR unless OPTION follows, so names like admin, inherit, and set remain valid role targets.\n\nConstraint: PR #25521 only exposes parser surface and should keep the fix localized to sqlparser.\nRejected: reserving admin/inherit/set as role-option prefixes unconditionally | PostgreSQL role names can use these words and the parser already supports identifier role targets.\nConfidence: high\nScope-risk: narrow\nDirective: Keep role-option detection lookahead-based so role names are not consumed before the clause is proven.\nTested: cargo fmt --check; cargo test -p risingwave_sqlparser parse_revoke_role_named_option_words_without_option_clause --test sqlparser_common; cargo test -p risingwave_sqlparser parse_revoke_set_option_for_role --test sqlparser_common; cargo test -p risingwave_sqlparser granted_by --test sqlparser_common\nNot-tested: full workspace check
Avoid treating option-keyword role names as REVOKE <option> OPTION FOR unless OPTION follows, so names like admin, inherit, and set remain valid role targets.

Constraint: PR #25521 only exposes parser surface and should keep the fix localized to sqlparser.
Rejected: reserving admin/inherit/set as role-option prefixes unconditionally | PostgreSQL role names can use these words and the parser already supports identifier role targets.
Confidence: high
Scope-risk: narrow
Directive: Keep role-option detection lookahead-based so role names are not consumed before the clause is proven.
Tested: cargo fmt --check; cargo test -p risingwave_sqlparser parse_revoke_role_named_option_words_without_option_clause --test sqlparser_common; cargo test -p risingwave_sqlparser parse_revoke_set_option_for_role --test sqlparser_common; cargo test -p risingwave_sqlparser granted_by --test sqlparser_common
Not-tested: full workspace check
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Adds PostgreSQL-style RBAC SQL surface area (parser + AST + formatting) for role-oriented statements, along with initial frontend/pgwire fallbacks and documentation, as the first lane in the RBAC split stack.

Changes:

  • Extend SQL AST/parser to support CREATE ROLE, ALTER ROLE, role-membership GRANT/REVOKE (incl. GRANTED BY), plus SET ROLE/RESET ROLE.
  • Add/extend parser test coverage via sqlparser_common unit tests and YAML contract tests (including new role.yaml and updated drop.yaml).
  • Add compile-safe handling in pgwire statement-type inference and frontend statement dispatch for new variants (mostly as staged “not implemented” paths).

Reviewed changes

Copilot reviewed 13 out of 13 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
src/utils/pgwire/src/pg_response.rs Map new role-related Statement variants to existing StatementTypes for response inference.
src/sqlparser/tests/testdata/role.yaml New YAML parser/formatter contract coverage for role syntax, options, and GRANTED BY.
src/sqlparser/tests/testdata/drop.yaml Extend DROP YAML contracts to cover DROP ROLE.
src/sqlparser/tests/sqlparser_common.rs Add unit tests for DROP ROLE, role create/alter, role membership grant/revoke, and set/reset role.
src/sqlparser/src/parser.rs Implement parsing for role statements/options, role-membership grant/revoke, and RESET ROLE.
src/sqlparser/src/keywords.rs Add ADMIN and ROLE keywords.
src/sqlparser/src/ast/statement.rs Add role context/modifier and role-option AST/display support; extend UserOption parsing/display.
src/sqlparser/src/ast/mod.rs Add new Statement variants (GrantRole, RevokeRole, CreateRole, AlterRole, SetRole, ResetRole) and ObjectType::Role display/parse support.
src/frontend/src/handler/mod.rs Add DROP ROLE frontend fallback (explicit not-implemented).
src/frontend/src/handler/create_user.rs Reject newly-parsed role-related user options with a staged “unsupported yet” error.
src/frontend/src/handler/alter_user.rs Reject newly-parsed role-related user options with a staged “unsupported yet” error.
docs/dev/src/design/rbac-parser-surface.md New design note documenting the staged parser/dispatch surface.
docs/dev/src/SUMMARY.md Add the RBAC parser surface doc to the dev docs summary.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread docs/dev/src/design/rbac-parser-surface.md
Comment thread src/frontend/src/handler/create_user.rs
Comment thread src/frontend/src/handler/alter_user.rs
Copy link
Copy Markdown
Contributor Author

@tabVersion tabVersion left a comment

Choose a reason for hiding this comment

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

I did a stack-aware pass focused on the parser surface boundary. The split and PR description look aligned with #25528/#25449 overall; I found one parser ambiguity worth fixing before this lane becomes the stable SQL surface for the downstream RBAC PRs.

Comment thread src/sqlparser/src/parser.rs Outdated
Role membership GRANT/REVOKE was selected only when the first token was not a privilege keyword, so roles named connect, create, select, or usage could be created but not granted or revoked unquoted. The parser now disambiguates by the statement shape: object privileges must reach ON before the role separator, while membership statements reach TO/FROM first.\n\nConstraint: This PR is the parser-surface lane for a stacked RBAC rollout; runtime RBAC semantics remain out of scope.\nRejected: Ban privilege-keyword role names | CREATE ROLE already accepts them and quoted-only follow-up would make the parser contract inconsistent.\nRejected: Full checkpoint fallback parse | broader control-flow change than needed for this grammar split.\nConfidence: high\nScope-risk: narrow\nTested: cargo fmt --check; cargo test -p risingwave_sqlparser --test sqlparser_common; cargo test -p risingwave_sqlparser --test parser_test; manual sqlparser probes for role and object-privilege GRANT/REVOKE\nNot-tested: Full workspace check
Copilot noted that the parser-surface lane documented stale YAML coverage and lacked tests for the staged user-handler boundary around role options. Keep the documentation aligned with the actual role/drop parser fixtures and add focused frontend tests that the newly parsed CREATEROLE/INHERIT variants still fail explicitly until runtime RBAC lanes implement them.\n\nConstraint: This PR should not implement runtime role-option semantics.\nConfidence: high\nScope-risk: narrow\nTested: cargo fmt --check; cargo test -p risingwave_frontend test_create_user_rejects_role_options --lib; cargo test -p risingwave_frontend test_alter_user_rejects_role_options --lib; cargo test -p risingwave_sqlparser --test parser_test -- role.yaml; cargo test -p risingwave_sqlparser --test parser_test -- drop.yaml\nNot-tested: Full ./risedev c
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

type/feature Type: New feature.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants