sql/catalog/descs: enforce invariant that OriginalVersion does not ch… #79580
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
|
This change is |
…ange It's a bug any time we increment a version more than once in the context of a single descriptor collection. Let's enforce it. Release note: None
ajwerner
force-pushed
the
ajwerner/enforce-version-invariant
branch
from
258d474 to
1da50fd
Compare
postamar
approved these changes
Apr 8, 2022
There was a problem hiding this comment.
Reviewed 1 of 1 files at r1, all commit messages.
Reviewable status:complete! 1 of 0 LGTMs obtained (waiting on @ajwerner)
|
TFTR! bors r+ |
|
Build succeeded: |
ajwerner
added a commit
to ajwerner/cockroach
that referenced
this pull request
Apr 8, 2022
As soon as cockroachdb#79580 merged, it tickled flakes. These flakes were caused by operations to alter the database which would build new mutable tables from immutable tables and thus overwrite existing entries. The test is very flakey without this patch. Release note: None
This was referenced Apr 8, 2022
ajwerner
added a commit
to ajwerner/cockroach
that referenced
this pull request
Apr 11, 2022
…database As soon as cockroachdb#79580 merged, it tickled flakes. These flakes were caused by operations to alter the database which would build new mutable tables from immutable tables and thus overwrite existing entries. The fix here is to retrieve the proper descriptors using the collection, and to make sure that those descriptors are properly hydrated. This, in turn, revealed that our policy checking in validation to avoid using descriptors which had been already checked out was too severe and would cause excessive numbers of extra round-trip. I'll be honest, I haven't fully internalized why that policy was there. I supposed it was there to ensure that we didn't have a case where we check out a descriptor and then fail to write it to the store. I don't exactly know what to do to re-establish the desired behavior of the code. At the very least, it feels like if we did re-read the descriptor once, then we should do something about resetting the status. I guess I could try to unravel the mystery leading to the checkout in the first place. The test is very flakey without this patch. Release note: None
craig bot
pushed a commit
that referenced
this pull request
Apr 13, 2022
79477: ccl/sqlproxyccl: add server name indication (SNI) support r=darinpp a=darinpp Previously the proxy supported two ways of providing tenant id and cluster name information. One was through the database name. The second was through the options parameter. As part of the serverless routing changes, we want to support routing with a tenant id passed through SNI. This PR adds the ability to route using SNI info. Release justification: Low risk, high reward changes to existing functionality Release note: None 79629: clisqlshell: implement `COPY ... FROM STDIN` for CLI r=otan a=otan Resolves #16392 Steps: * Add a lower level API to lib/pq for use. * Add some abstraction boundary breakers in `clisqlclient` that allow a lower level handling of the COPY protocol. * Altered the state machine in `clisqlshell` to account for copy. Release note (cli change): COPY ... FROM STDIN now works from the cockroach CLI. It is not supported inside transactions. 79677: sql: session variable to allow multiple modification subqueries of table r=rytaft a=michae2 Add a new session variable, enable_multiple_modifications_of_table, which can be used instead of sql.multiple_modifications_of_table.enabled to allow execution of statements with multiple modification subqueries of the same table. Instead of making the original cluster setting the GlobalDefault of this new session setting, the original cluster setting is kept in the optbuilder logic. This is to avoid breaking applications that are already toggling the cluster setting mid-session to allow statements. Fixes: #76261 Release note (sql change): Add a new session variable, enable_multiple_modifications_of_table, which can be used instead of cluster variable sql.multiple_modifications_of_table.enabled to allow statements containing multiple INSERT ON CONFLICT, UPSERT, UPDATE, or DELETE subqueries modifying the same table. Note that underlying issue 70731 is not fixed. As with sql.multiple_modifications_of_table.enabled, be warned that with this session variable enabled there is nothing to prevent the table corruption seen in issue 70731 from occuring if the same row is modified multiple times by different subqueries of a single statment. It's best to rewrite these statements, but the session variable is provided as an aid if this is not possible. 79697: sql: ensure descriptors versions change only once, fix related bugs r=ajwerner a=ajwerner The first commit adds back the change from #79580. The second commit fixes fallout. As soon as #79580 merged, it tickled flakes. These flakes were caused by operations to alter the database which would build new mutable tables from immutable tables and thus overwrite existing entries. The fix here is to retrieve the proper descriptors using the collection, and to make sure that those descriptors are properly hydrated. This, in turn, revealed that our policy checking in validation to avoid using descriptors which had been already checked out was too severe and would cause excessive numbers of extra round-trip. I'll be honest, I haven't fully internalized why that policy was there. I supposed it was there to ensure that we didn't have a case where we check out a descriptor and then fail to write it to the store. I don't exactly know what to do to re-establish the desired behavior of the code. At the very least, it feels like if we did re-read the descriptor once, then we should do something about resetting the status. I guess I could try to unravel the mystery leading to the checkout in the first place. The test is very flakey without this patch. The third commit sets `AvoidLeased` flags which ought to be set. It's sad that they ought to be set and that it's possible that there were bugs due to the flag not being set. Backport will address #79704. Release note: None 79857: ccl/sqlproxyccl: cleanup balancer and connection tracker r=JeffSwenson a=jaylim-crl This is a follow up to #79725. Previously, there were two issues: 1. Some of the ConnTracker APIs (e.g. GetConns and OnDisconnect) were forcing creation of tenant entries even though that was unnecessary. 2. We were holding onto a large portion of memory (with connections from all tenants). This commit addresses the two issues above: (1) tenant entries are no longer being created when that was unnecessary, and (2) instead of holding onto all the connections at once, we will retrieve the tenant's connections one by one. At the same time, to avoid extra allocations that are solely used during data transformations, we update the ConnTracker API to return a list of connection handles indexed by pod's address directly. Note that we may reintroduce the data transformation in future PRs, depending on how we implement the connection rebalancing logic. That is currently unclear. Release note: None 79874: ccl/sqlproxyccl: add metrics to track the number of rebalance requests r=JeffSwenson a=jaylim-crl This commit addresses a TODO by adding metrics to track the number of rebalance requests (total, queued, and running). The following metrics are added: - proxy.balancer.rebalance.running - proxy.balancer.rebalance.queued - proxy.balancer.rebalance.total Release note: None Co-authored-by: Darin Peshev <[email protected]> Co-authored-by: Oliver Tan <[email protected]> Co-authored-by: Michael Erickson <[email protected]> Co-authored-by: Andrew Werner <[email protected]> Co-authored-by: Andrew Werner <[email protected]> Co-authored-by: Jay <[email protected]>
blathers-crl bot
pushed a commit
that referenced
this pull request
Apr 13, 2022
…database As soon as #79580 merged, it tickled flakes. These flakes were caused by operations to alter the database which would build new mutable tables from immutable tables and thus overwrite existing entries. The fix here is to retrieve the proper descriptors using the collection, and to make sure that those descriptors are properly hydrated. This, in turn, revealed that our policy checking in validation to avoid using descriptors which had been already checked out was too severe and would cause excessive numbers of extra round-trip. I'll be honest, I haven't fully internalized why that policy was there. I supposed it was there to ensure that we didn't have a case where we check out a descriptor and then fail to write it to the store. I don't exactly know what to do to re-establish the desired behavior of the code. At the very least, it feels like if we did re-read the descriptor once, then we should do something about resetting the status. I guess I could try to unravel the mystery leading to the checkout in the first place. The test is very flakey without this patch. Release note: None
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.
…ange
It's a bug any time we increment a version more than once in the context
of a single descriptor collection. Let's enforce it. This would have prevented bugs
like the one fixed in #79561.
Release note: None