sql: implement row_security session variable #155110
Conversation
|
This change is |
spilchen
force-pushed
the
gh-138916/251008/1708/rls-row-security/pr-ready
branch
from
25385a4 to
906f4fc
Compare
|
Note: this is separate from the INSPECT work I have been doing recently. @shghasemi and I worked through part of this in our 1-on-1 pair programming session. So, just finishing this off from that session. |
There was a problem hiding this comment.
Nice! Just a few nits from me,
@yuzefovich reviewed 9 of 9 files at r1, all commit messages.
Reviewable status:complete! 1 of 0 LGTMs obtained (waiting on @rytaft)
pkg/sql/opt/memo/memo.go line 498 at r1 (raw file):
m.disableSlowCascadeFastPathForRBRTables != evalCtx.SessionData().OptimizerDisableCrossRegionCascadeFastPathForRBRTables || m.useImprovedHoistJoinProject != evalCtx.SessionData().OptimizerUseImprovedHoistJoinProject || m.rowSecurity != evalCtx.SessionData().RowSecurity ||
nit: we should extend TestMemoIsStale for this addition.
pkg/sql/logictest/testdata/logic_test/row_level_security line 5766 at r1 (raw file):
SET SESSION row_security = 'off'; statement error pq: query would be affected by row-level security policy for table "accounts"
nit: it's nice to also check the PG error code (if we match postgres).
pkg/sql/vars.go line 1688 at r1 (raw file):
`row_security`: { Get: func(evalCtx *extendedEvalContext, _ *kv.Txn) (string, error) {
nit: I don't immediately see why / what the difference is, but we also usually set GetStringVal.
Complete the implementation of the `row_security` session variable, which was previously defined but non-functional. When set to `'off'`, non-admin queries that would be affected by row-level security (RLS) fail with an error instead of silently filtering rows. This matches postgres' behavior. Admin users and table owners (without FORCE) remain exempt from RLS regardless of this setting. The variable defaults to `'on'` and is tracked in the memo to invalidate cached plans when changed. Closes cockroachdb#138916. Epic: none Release note (sql change): The `row_security` session variable now behaves like in postgres, allowing users to detect when RLS is applied.
spilchen
force-pushed
the
gh-138916/251008/1708/rls-row-security/pr-ready
branch
from
906f4fc to
7e43381
Compare
There was a problem hiding this comment.
Reviewable status:
pkg/sql/vars.go line 1688 at r1 (raw file):
Previously, yuzefovich (Yahor Yuzefovich) wrote…
nit: I don't immediately see why / what the difference is, but we also usually set
GetStringVal.
Done
pkg/sql/opt/memo/memo.go line 498 at r1 (raw file):
Previously, yuzefovich (Yahor Yuzefovich) wrote…
nit: we should extend
TestMemoIsStalefor this addition.
Done
pkg/sql/logictest/testdata/logic_test/row_level_security line 5766 at r1 (raw file):
Previously, yuzefovich (Yahor Yuzefovich) wrote…
nit: it's nice to also check the PG error code (if we match postgres).
Done. Yes, we match postgres.
There was a problem hiding this comment.
@yuzefovich reviewed 3 of 3 files at r2, all commit messages.
Reviewable status:
There was a problem hiding this comment.
lgtm! it's nice that we're able to add this
Reviewable status:
|
TFTRs! bors r+ |
4 participants
Complete the implementation of the
row_securitysession variable, which was previously defined but non-functional.When set to
'off', non-admin queries that would be affected by row-level security (RLS) fail with an error instead of silently filtering rows. This matches postgres' behavior.Admin users and table owners (without FORCE) remain exempt from RLS regardless of this setting.
The variable defaults to
'on'and is tracked in the memo to invalidate cached plans when changed.Closes #138916.
Epic: none
Release note (sql change): The
row_securitysession variable now behaves like in postgres, allowing users to detect when RLS is applied.