-
Notifications
You must be signed in to change notification settings - Fork 470
Add docs for Row-level security (RLS) #19527
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Files changed:
|
✅ Deploy Preview for cockroachdb-interactivetutorials-docs canceled.
|
✅ Deploy Preview for cockroachdb-api-docs canceled.
|
✅ Netlify Preview
To edit notification comments on pull requests, go to your Netlify site configuration. |
dc16ae3
to
2cb3af9
Compare
56780c5
to
f07bb90
Compare
181509f
to
0046485
Compare
Fixes DOC-10439, DOC-12948 Summary of changes: - Add 'Row-level security' overview page - Add or update SQL statement docs for: - `CREATE POLICY` - `ALTER POLICY` - `DROP POLICY` - `SHOW POLICIES` - `ALTER TABLE {ENABLE,DISABLE} ROW LEVEL SECURITY` - `ALTER TABLE {FORCE,UNFORCE} ROW LEVEL SECURITY` - `CREATE ROLE ... WITH BYPASSRLS` - `ALTER ROLE ... WITH BYPASSRLS`
0046485
to
20ddc91
Compare
thanks for the review @spilchen, i've tried to address your feedback, this is RFAL - changes are in latest commit |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is looking good.
thanks for the additional comments @spilchen , this is RFAL again! |
src/current/v25.2/alter-policy.md
Outdated
ALTER POLICY policy_name ON table_name RENAME TO new_policy_name; | ||
|
||
ALTER POLICY policy_name ON table_name | ||
[ TO { role_name | PUBLIC | CURRENT_USER | SESSION_USER } [, ...] ] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are the { } brackets here part of the SQL syntax convention or are you using it to denote a placeholder as we do elsewhere in the docs? If the latter, I'd recommend excluding it from the SQL syntax as it might be confusing to mix with [ ] and ( ).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah my bad, just a mistake, i have changed them from {}
to ()
since the latter usually represents a list of OR
things in a BNF grammar AIUI
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These still look like { } to me, but maybe I'm out of sync?
... also fix a busted link
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me. Thanks for revising.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have a few follow-up comments but overall LGTM!
src/current/v25.2/alter-policy.md
Outdated
ALTER POLICY policy_name ON table_name RENAME TO new_policy_name; | ||
|
||
ALTER POLICY policy_name ON table_name | ||
[ TO { role_name | PUBLIC | CURRENT_USER | SESSION_USER } [, ...] ] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These still look like { } to me, but maybe I'm out of sync?
Fixes DOC-10439, DOC-12948
Summary of changes:
CREATE POLICY
ALTER POLICY
DROP POLICY
SHOW POLICIES
ALTER TABLE {ENABLE,DISABLE} ROW LEVEL SECURITY
ALTER TABLE {FORCE,UNFORCE} ROW LEVEL SECURITY
CREATE ROLE ... WITH BYPASSRLS
ALTER ROLE ... WITH BYPASSRLS