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.

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

validate: use immutable descriptors only #95827

Closed
postamar opened this issue Jan 25, 2023 · 0 comments · Fixed by #95830
Closed

validate: use immutable descriptors only #95827

postamar opened this issue Jan 25, 2023 · 0 comments · Fixed by #95830
Assignees
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

@postamar
Copy link
Contributor

postamar commented Jan 25, 2023

Descriptor validation doesn't care about descriptor mutability and validating mutable tables can end up having quite a performance impact due to having to rebuild the whole cache in the descriptor implementation every time we want to access a column or an index.

We're already pretty good about passing immutable descriptors to validation in most cases except during schema changes we do a bunch of validate.Self calls on the mutable descriptor for all kinds of good reasons.

In any case, the callers of the validation logic shouldn't have to concern themselves about mutability in the first place.

Jira issue: CRDB-23766

@postamar postamar added the C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) label Jan 25, 2023
@postamar postamar self-assigned this Jan 25, 2023
@blathers-crl blathers-crl bot added the T-sql-schema-deprecated Use T-sql-foundations instead label Jan 25, 2023
craig bot pushed a commit that referenced this issue Jan 25, 2023
95013: sql: add ability set, edit, read tenant capabilities r=knz a=ecwall

Fixes #87851

Add new SQL syntax for
1) Setting tenant capabilities:
`ALTER TENANT t GRANT CAPABILITY capabilitiy_name=capability_value;`
2) Resetting tenant capabilities:
`ALTER TENANT t REVOKE CAPABILITIY capability_name;`
3) Reading tenant capabilities:
`SHOW TENANT t WITH CAPABILITIES;`

Release note: None

95797: sql: improve stack trace for get-user-timeout timeouts r=knz a=ecwall

Fixes #95794

The cause of the `get-user-timeout` errors is unknown. Part of the problem is that the stack trace gets cut off at
```
  |   | github.com/cockroachdb/cockroach/pkg/sql.retrieveSessionInitInfoWithCache
  |   | 	github.com/cockroachdb/cockroach/pkg/sql/user.go:238
```
which does not explain what is actually being blocked.

The reason that the stack trace is cut off is that the timeout is initiated by `contextutil.RunWithTimeout` which results in a "simple" (no stack trace) `context.DeadlineExceeded` error.

`retrieveSessionInitInfoWithCache` is the first line in the stack trace because it calls `errors.Wrap` on `context.DeadlineExceeded`.

To get a fuller stack trace, `context.DeadlineExceeded` must be wrapped immediately (`errors.Wrap` or `errors.WithStack`) before it bubbles up.

Release note: None

95830: validate: use immutable descriptors only r=postamar a=postamar

The descriptor validation logic will accept any implementation of catalog.Descriptor be it mutable or immutable, it doesn't care. However, using mutable implementations can have a significant performance impact especially in the case of tables, where every column or index or constraint lookup will lead to the cache being regenerated for the whole descriptor.

This commit fixes this by having validate.Validate replace any mutable descriptor instances it encounters with immutable copies. This doesn't change anything except performance.

Fixes #95827.

Release note: None

95852: ui: cache sqlroles results r=maryliag a=maryliag

Previously, the call to get sql roles was constantly being requested. This commits adds a cache limit, so it will only get request after the expiration time.

https://www.loom.com/share/6814309f91234fa2b17490df8160bde6

Epic: None
Release note: None

95863: storage: reorder EventListeners r=jbowens a=jbowens

To be defensive, sequence the EventListener responsible for crashing the process during a disk stall first, before the Pebble logging event listener.

Informs #94373.
Epic: None
Release note: None

Co-authored-by: Evan Wall <[email protected]>
Co-authored-by: Marius Posta <[email protected]>
Co-authored-by: maryliag <[email protected]>
Co-authored-by: Jackson Owens <[email protected]>
@craig craig bot closed this as completed in b3713c7 Jan 26, 2023
@exalate-issue-sync exalate-issue-sync bot added T-sql-foundations SQL Foundations Team (formerly SQL Schema + SQL Sessions) and removed T-sql-schema-deprecated Use T-sql-foundations instead labels May 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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
Development

Successfully merging a pull request may close this issue.

1 participant