release-22.2: sql: fix extra round trips during DISCARD, fix temp discard bug #96102
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
If this was not done, there'd be a bug in `DISCARD ALL` and `DISCARD TEMP` which would prevent temporary schemas from working for the rest of the session because the `descs.Collection` would have a handle to the wrong object Release note (bug fix): Fixed a bug in experimental temporary schemas whereby `DISCARD ALL` or `DISCARD TEMP` could prevent temporary tables from working.
|
Thanks for opening a backport. Please check the backport criteria before merging:
If some of the basic criteria cannot be satisfied, ensure that the exceptional criteria are satisfied within.
Add a brief release justification to the body of your PR to justify this backport. Some other things to consider:
|
|
This change is |
|
@ZhouXing19 this has an extra commit tacked on to the front to fix a bug I had introduced in 22.2. I'll see if any part of it needs a forward-port |
Epic: none Release note: None
In cockroachdb#86246 we introduced logic to discard schemas when running DISCARD ALL and DISCARD TEMP. This logic did expensive kv operations unconditionally; if the session knew it had never created a temporary schema, we'd still fetch all databases and proceed to search all databases for a temp schema. This is very expensive. Fixes: cockroachdb#95864 Release note (performance improvement): In 22.2, we introduced support for `DISCARD TEMP` and made `DISCARD ALL` actually discard temp tables. This implementation ran expensive logic to discover temp schemas rather than consulting in-memory data structures. As a result, `DISCARD ALL`, which is issued regularly by connection pools, became an expensive operation when it should be cheap. This problem is now resolved.
This is an uncached round-trip. It makes `DISCARD` expensive. In cockroachdb#86485 we implemented `SET SESSION AUTHORIZATION DEFAULT`, this added an extra sql query to `DISCARD ALL` to check if the role we'd become exists. This is almost certainly unintentional in the case where the role we'd become is the current session role. I think this just happened because of code consolidation. We now no longer check if the current session role exists. This removes the last round-trip from DISCARD ALL. Release note (performance improvement): In 22.2, logic was added to make `SET SESSION AUTHORIZATION DEFAULT` not a no-op. This implementation used more general code for setting the role for a session which made sure that the role exists. The check for whether a role exists is currently uncached. We don't need to check if the role we already are exists. This improves the performance of `DISCARD ALL` in addition to `SET SESSION AUTHORIZATION DEFAULT`.
ajwerner
force-pushed
the
backport22.2-95876
branch
from
928d418 to
4e1f041
Compare
ajwerner
added a commit
to ajwerner/cockroach
that referenced
this pull request
Jan 27, 2023
We had bugs with this in earlier releases. This is a forward-port of a test introduced in cockroachdb#96102. It never failed master, but no reason to ever let this regress. Epic: none Release note: None
|
TFTR! |
craig bot
pushed a commit
that referenced
this pull request
Feb 7, 2023
95370: logictest: add a skip_on_retry directive to the sequences test r=ajwerner a=ajwerner See [this bors run](https://teamcity.cockroachdb.com/buildConfiguration/Cockroach_BazelEssentialCi/8344646?showRootCauses=false&expandBuildChangesSection=true&expandBuildProblemsSection=true&expandBuildTestsSection=true) In the long run, this directive is not great, but it's better than flakes. Epic: none Release note: None 96014: multitenant: prevent tenant from overriding next tenant's span config r=arulajmani a=ecwall Fixes #95882 We want to ensure we have a split at the non-inclusive end of the tenant's keyspace, which also happens to be the inclusive start of the next tenant's (with ID=ours+1). If we didn't do anything here, and we were the tenant with the highest ID thus far, our last range would extend to /Max. If a tenant with a higher ID was created, when installing its initial span config record, it would carve itself off (and our last range would only extend to that next tenant's boundary), but this is a bit awkward for code that wants to rely on the invariant that ranges for a tenant only extend to the tenant's well-defined end key. So how do we ensure this split at this tenant's non-inclusive end key? Hard splits are controlled by the start keys on span config records[^1], so we'll try insert one accordingly. But we cannot do this blindly. We cannot assume that tenants are created in ID order, so it's possible that the tenant with the next ID is already present + running. If so, it may already have span config records that start at the key at which we want to write a span config record for. Over-writing it blindly would be a mistake -- there's no telling what config that next tenant has associated for that span. So we'll do something simple -- we'll check transactionally whether there's anything written already, and if so, do nothing. We already have the split we need. [^1]: See ComputeSplitKey in spanconfig.StoreReader. Release note: None 96111: logictest: extend temp_table test to exercise discard + drop r=ajwerner a=ajwerner We had bugs with this in earlier releases. This is a forward-port of a test introduced in #96102. It never failed master, but no reason to ever let this regress. Epic: none Release note: None 96125: logictest: frontport test from #96124 r=ajwerner a=ajwerner This testing is worth having. It does pass. Epic: None Release note: None 96206: streamingccl,db-console: add Cross-Cluster Replication metrics page r=adityamaru a=adityamaru This change adds a new option to the metric dropdown dashboard for Cross-Cluster Replication. This page will show all the user-facing metrics relevant to C2C replication. Informs: #95995 Release note: None server,db-console: add feature flag for Cross-Cluster Replication dashboard This change adds a feature flag whose value is determined by the cluster setting `cross_cluster_replication.enabled`. This feature flag is then used when rendering the metrics dashboard dropdown options to decide whether to display the cross-cluster replication dashboard. This change also deletes the session variable and associated cluster setting `sql.defaults.experimental_stream_replication.enabled` so as to unify all interactions with cross-cluster replication under a single cluster setting. Fixes: #95995 Release note: None 96499: sql: deflake TestParseClientProvidedSessionParameters r=knz a=rafiss fixes #96492 pgx starts an internal goroutine if a context is cancelled. We need to wait for it to be done in order to avoid a leaked goroutine in the test. Release note: None 96681: *: Properly support partial UNIQUE WITHOUT INDEX referencing type descs r=Xiang-Gu a=Xiang-Gu Previously if a partial UNIQUE WITHOUT INDEX references a type descriptor in its predicate, we didn't add back-references in the type descriptor, both in the legacy and declarative schema changer. This commit fixes this. Fixes #96678 Release note: None 96689: roachtest: return error in StopCockroachGracefullyOnNode r=herkolategan,srosenberg a=renatolabs That function misleadingly returned an (always nil) error, calling `t.Fatal()` functions in it. The calls to `Stop` have been replaced with calls to `StopE`. Epic: none Release note: None Co-authored-by: Andrew Werner <[email protected]> Co-authored-by: Evan Wall <[email protected]> Co-authored-by: Andrew Werner <[email protected]> Co-authored-by: adityamaru <[email protected]> Co-authored-by: Rafi Shamim <[email protected]> Co-authored-by: Xiang Gu <[email protected]> Co-authored-by: Renato Costa <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Backport 3/3 commits from #95876.
Also tacks on another bug fix.
Release justification: fixes severe regression when using pg_bouncer
Fixes #95864
/cc @cockroachdb/release
sql: avoid checking if a role exists when setting to the current role
This is an uncached round-trip. It makes
DISCARDexpensive.In #86485 we implemented
SET SESSION AUTHORIZATION DEFAULT, this added an extra sql query toDISCARD ALLto check if the role we'd become exists. This is almostcertainly unintentional in the case where the role we'd become is the
current session role. I think this just happened because of code
consolidation.
We now no longer check if the current session role exists. This removes
the last round-trip from DISCARD ALL.
sql: use in-memory session data to decide what to do in DISCARD
In #86246 we introduced logic to discard schemas when running DISCARD ALL and DISCARD TEMP. This logic did expensive kv operations unconditionally; if the session knew it had never created a temporary schema, we'd still fetch all databases and proceed to search all databases for a temp schema. This is very expensive.
Fixes: #95864
sql: reset the temporary schemas after injecting them
If this was not done, there'd be a bug in
DISCARD ALLandDISCARD TEMPwhich would prevent temporary schemas from working forthe rest of the session because the
descs.Collectionwould have ahandle to the wrong object
Release note (bug fix): Fixed a bug in experimental temporary schemas whereby
DISCARD ALLorDISCARD TEMPcould prevent temporary tables from working.Release note (performance improvement): In 22.2, logic was added to make
SET SESSION AUTHORIZATION DEFAULTnot a no-op. This implementation usedmore general code for setting the role for a session which made sure that
the role exists. The check for whether a role exists is currently uncached.
We don't need to check if the role we already are exists. This improves the
performance of
DISCARD ALLin addition toSET SESSION AUTHORIZATION DEFAULT.Release note (performance improvement): In 22.2, we introduced support for
DISCARD TEMPand madeDISCARD ALLactually discard temp tables. This implementation ran expensive logic to discover temp schemas rather than consulting in-memory data structures. As a result,DISCARD ALL, which is issued regularly by connection pools, became an expensive operation when it should be cheap. This problem is now resolved.