Skip to content
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.

Sign up for GitHub

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

Jump to bottom

sql: refactor pg_builtin to use actual grant options #73129

Closed
jackcwu opened this issue Nov 24, 2021 · 0 comments
Closed

sql: refactor pg_builtin to use actual grant options #73129

jackcwu opened this issue Nov 24, 2021 · 0 comments
Assignees
@ecwall
Labels
C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) T-sql-foundations SQL Foundations Team (formerly SQL Schema + SQL Sessions)

Comments

@jackcwu
Copy link
Contributor

jackcwu commented Nov 24, 2021 •
edited by rafiss
Loading

After #72123 is merged, change privilege inquiry functions like has_database_privilege, has_table_privilege, and pg_has_role in pkg/sql/sem/builtins/pg_builtins.go to use the newly-added grant option information. Currently, there is a workaround where grant options are determined for a certain privilege by checking if the user holds the GRANT privilege as well.

In addition to simplifying the code, GRANT will eventually be deprecated and removed so this change is necessary for these functions to work in the future.

For this issue:

  • Update the comment describing the old way of looking at the GRANT privilege.
  • Update the runSinglePrivilegeCheck so it doesn't look at the GRANT privilege directly.
    • The new logic should only be used if the new cluster version is active. See an example of a version gate here.
  • This might require changing planner.HasPrivilege so that it accepts a boolean parameter for withGrantOption; and the implementation should call planner.CheckGrantOption.
  • We likely don't need new tests. But for current tests to pass, step 2 of sql: deprecate "GRANT" privilege #73065 needs to be completed.

See #72512 (comment) for more context

Epic CRDB-2587
@jackcwu jackcwu added C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) T-sql-foundations SQL Foundations Team (formerly SQL Schema + SQL Sessions) labels Nov 24, 2021
@rafiss rafiss changed the title Refactor pg_builtin to use actual grant options sql: refactor pg_builtin to use actual grant options Dec 10, 2021
@rafiss rafiss mentioned this issue Jan 12, 2022
Merged
craig bot pushed a commit that referenced this issue Jan 12, 2022
@ecwall @irfansharif
Merge #74172 #74596
a4ef738 
74172: sql: refactor pg_builtin to use actual grant options r=rafiss a=ecwall

refs #73129

The builtins has_table_privilege, has_column_privilege,
has_any_column_privilege now use privileges.Priv.GrantOption instead 
of privileges.Kind.GRANT.

Refactor builtins.priv -> privilege.Privilege.
    
Replace privilege.Kind with privilege.Privilege in functions that need
access to privilege.Privilege.GrantOption.


74596: dev: introduce common benchmarking flags r=irfansharif a=irfansharif

Specifically:
  --bench-mem (to print out memory allocations);
  --bench-time (to control how long each benchmark is run for);
  --count (how many times to run it, also added to `dev test`);
  -v and --show-logs (similar to `dev test`)

We also add supports for args-after-double-dash-are-for-bazel within
`dev bench`. This commit is light on testing (read: there isn't any), so
doesn't bump DEV_VERSION to roll it out to everyone just yet.

Release note: None

Co-authored-by: Evan Wall <[email protected]>
Co-authored-by: irfan sharif <[email protected]>
@ecwall ecwall mentioned this issue Jan 27, 2022
Merged
craig bot pushed a commit that referenced this issue Jan 28, 2022
@ecwall
Merge #75591
f54cf8b 
75591: sql: Remove redundant runSinglePrivilegeCheck grant check r=rafiss a=ecwall

refs #73129
    
HasPrivilege checks for privilege and grant permissions so runSinglePrivilegeCheck is no longer needed.

Co-authored-by: Evan Wall <[email protected]>
@ecwall ecwall mentioned this issue Jan 31, 2022
Merged
craig bot pushed a commit that referenced this issue Feb 2, 2022
@ajwerner @ecwall
@nvanbenschoten @yuzefovich @ericharmeling
Merge #74612 #75726 #75770 #75771 #75807 #75813 #75836
63988a4 
74612: rangefeedcache,settingswatcher,systemcfgwatcher: lose gossip deps r=ajwerner a=ajwerner

This is a pile of commits which supersedes #69269 and pretty much puts in place all of the pieces to move on #70560. 

75726: sql: refactor pg_has_role to remove privilege.GRANT usage r=RichardJCai a=ecwall

refs #73129

Also combines some layers of privilege checking code.

Release note: None

75770: vendor: bump cockroachdb/apd to v3.1.0, speed up decimal division r=nvanbenschoten a=nvanbenschoten

Picks up two PRs that improved the performance of `Quo`, `Sqrt`, `Cbrt`, `Exp`, `Ln`, `Log`, and `Pow`:
- cockroachdb/apd#114
- cockroachdb/apd#115

Almost all of the testing changes here are due to the rounding behavior in cockroachdb/apd#115. This brings us closer to PG's behavior, but also creates a lot of noise in this diff. To verify that this noise wasn't hiding any correctness regressions caused by the rewrite of `Context.Quo` in the first PR, I created #75757, which only includes the first PR. #75757 passes CI with minimal testing changes. The testing changes that PR did require all have to do with trailing zeros, and most of them are replaced in this PR.

Release note (performance improvement): The performance of many DECIMAL arithmetic operators has been improved by as much as 60%. These operators include division (`/`), `sqrt`, `cbrt`, `exp`, `ln`, `log`, and `pow`.

----

### Speedup on TPC-DS dataset

The TPC-DS dataset is full of decimal columns, so it's a good playground to test this change. Unfortunately, the variance in the runtime performance of the TPC-DS queries themselves is high (many queries varied by 30-40% per attempt), so it was hard to get signal out of them. Instead, I imported the TPC-DS dataset with a scale factor of 10 and ran some custom aggregation queries against the largest table (web_sales, row count = 7,197,566):

```sql
# q1
select sum(ws_wholesale_cost / ws_ext_list_price) from web_sales;

# q2
select sum(ws_wholesale_cost / ws_ext_list_price - sqrt(ws_net_paid_inc_tax)) from web_sales;
```

Here's the difference in runtime of these two queries before and after this change on an `n2-standard-8` instance:

```
name              old s/op   new s/op   delta
TPC-DS/custom/q1  22.4 ± 0%   8.6 ± 0%  -61.33%  (p=0.002 n=6+6)
TPC-DS/custom/q2  91.4 ± 0%  32.1 ± 0%  -64.85%  (p=0.008 n=5+5)
```

75771: colexec: close the ordered sync used by the external sorter r=yuzefovich a=yuzefovich

**colexec: close the ordered sync used by the external sorter**

Previously, we forgot to close the ordered synchronizer that is used by
the external sorter to merge already sorted partitions. This could
result in some tracing spans never being finished and is now fixed.

Release note: None

**colexec: return an error rather than logging it on Close in some cases**

This error eventually will be logged anyway, but we should try to
propagate it up the stack as much as possible.

Release note: None

75807: ui: Add CircleFilled component r=ericharmeling a=ericharmeling

Previously, there was no component for a filled circle icon in the `ui` package.
Recent product designs have requested this icon for the DB Console (see #67510, #73463).
This PR adds a `CircleFilled` component to the `ui` package.

Release note: None

75813: sql: fix flakey TestShowRangesMultipleStores r=ajwerner a=ajwerner

Fixes #75699

Release note: None

75836: dev: add generate protobuf r=ajwerner a=ajwerner

Convenient, fast.

Release note: None

Co-authored-by: Andrew Werner <[email protected]>
Co-authored-by: Evan Wall <[email protected]>
Co-authored-by: Nathan VanBenschoten <[email protected]>
Co-authored-by: Yahor Yuzefovich <[email protected]>
Co-authored-by: Eric Harmeling <[email protected]>
@ecwall ecwall closed this as completed Feb 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@ecwall ecwall

Labels
C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) T-sql-foundations SQL Foundations Team (formerly SQL Schema + SQL Sessions)
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@ecwall @jackcwu