builtins: improve performance of has_table_privilege and has_column_privilege#65766
Conversation
|
This change is |
d5beb86 to
e24485f
Compare
* Eliminate the OID lookup internal query by realising that OID maps to descpb.ID directly. * Eliminate the information_schema.table_privileges internal lookup query by consulting the table descriptor directly. Release note (performance improvement): Improve the performance of has_table_privilege by using an internal cache for performing privilege lookups.
Release note (performance improvement): Improved the performance of has_any_column_privilege by removing some internal queries.
RichardJCai
left a comment
There was a problem hiding this comment.
LGTM mod nits
Reviewable status:
complete! 0 of 0 LGTMs obtained (waiting on @dt, @otan, and @rafiss)
pkg/sql/resolver.go, line 336 at r2 (raw file):
hasPrivilege := func(priv privilege.Kind) bool { for _, role := range allRoles { for _, pu := range privilegeDesc.Users {
I think we can call privilegeDesc.CheckPrivilege here
pkg/sql/sem/builtins/pg_builtins.go, line 1333 at r4 (raw file):
return nil, err } colArg := tree.UnwrapDatum(ctx, args[1])
Can we leave a similar comment to before stating that the column arg is only checked to see if the column exists in the table?
Makes it more clear that we're still only checking the table for privileges.
Use the same tricks as has_table_privilege, but insert specifiers in HasPrivilegeSpecifier to also check the column's existence. Release note (performance improvement): Improve the performance of has_column_privilege by removing excessive queries.
otan
left a comment
There was a problem hiding this comment.
Reviewable status:
pkg/sql/resolver.go, line 336 at r2 (raw file):
Previously, RichardJCai (Richard Cai) wrote…
I think we can call
privilegeDesc.CheckPrivilegehere
i changed it, but it revealed (what i think is a) bug. can you re-check the last commit?
otan
left a comment
There was a problem hiding this comment.
Reviewable status:
pkg/sql/sem/builtins/pg_builtins.go, line 1333 at r4 (raw file):
Previously, RichardJCai (Richard Cai) wrote…
Can we leave a similar comment to before stating that the column arg is only checked to see if the column exists in the table?
Makes it more clear that we're still only checking the table for privileges.
Done.
Release note (bug fix): Fixed a bug where owners of a table has privileges to SELECT from it, but would return false on has_*_privilege related functions.
…lege Release note: None
|
bors r=richardjcai |
|
Build succeeded: |
See individual commits for details
Resolves #65551