sql: some schema changes use incorrect mutable descriptor #79704
Labels
branch-master
Failures and bugs on the master branch.
branch-release-22.1
Used to mark GA and release blockers, technical advisories, and bugs for 22.1
C-bug
Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior.
GA-blocker
T-sql-foundations
SQL Foundations Team (formerly SQL Schema + SQL Sessions)
Comments
ajwerner
added
C-bug
craig bot
pushed a commit
that referenced
this issue
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]>
|
Backport has been merged, closing. |
exalate-issue-sync
bot
added
T-sql-foundations
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Describe the problem
The
Mutableform of descriptors track internally the "original" version of the descriptor. This is useful for various things, including the split calculation done in #77337. When the mutable form of the descriptor is always retrieved from the collection, the "original" version will be properly maintained. There are, for better or for worse, other ways to get your hands on a mutable descriptor. In particular, one can use aBuilderto get from an immutable to a mutable. Sadly, when you do that, you lose the history of the descriptor.One example of this bug was #79561.
Generally, detecting this bug turns out to be easy: #79580
Unfortunately, there are other cases where we'd hit this issue. One is here:
cockroach/pkg/sql/database.go
Line 101 in d61df4c
Additional context
This is loosely related to #64673.
Jira issue: CRDB-14986
The text was updated successfully, but these errors were encountered: