sql: add implicit SELECT FOR SHARE locking to unique checks#107961
sql: add implicit SELECT FOR SHARE locking to unique checks#107961craig[bot] merged 1 commit intocockroachdb:masterfrom
Conversation
|
It looks like your PR touches production code but doesn't add or edit any test code. Did you consider adding tests to your PR? 🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf. |
|
This change is |
|
@nvanbenschoten is there another term you'd prefer over "range granularity" for these locks? I was thinking about key-range locks used by MS SQL Server under serializable isolation when I called them that, and also this slack conversation, but I can see how that name isn't great. Maybe some other ideas are "span granularity" or something about "gap locking" or "next key locking" as in InnoDB (but maybe that's an implementation detail?). |
|
The concept of "granularity" is related to but distinct from the concept of locking absent keys (still with single-key granularity). I'm not sure exactly what concept we'd want to express in the optimizer, but I don't think any multi-key locking granularity will be supported by KV any time soon. What KV can support is a lock policy that dictates whether locks should be acquired on "present" keys or "present and absent" keys. |
Makes sense. Would it be alright with you if we gave them the same name in the optimizer regardless of single-key or multi-key, but then did not permit plans using the multi-key version? From the optimizer's perspective the important property is locking both rows and gaps, regardless of the number of keys covered, but I can see that the implementation might be very different in KV for single-key vs multi. |
|
@nvanbenschoten how about something like: type LockingClairvoyance byte
const (
LockExisting LockingClairvoyance = iota
LockPhantoms
)(I guess Sybase called them phantom locks so maybe something in that vein?) |
|
Latest version uses these names: type LockingClass byte
const (
LockRecord LockingClass = iota
LockPredicate
)Which better match Postgres' terminology (predicate locks) and the original terminology from The Notions of Consistency and Predicate Locks in a Database System (1976). WDYT? |
|
I like "predicate" locks, as it doesn't on its own imply the granularity of such a lock, just that it prevents writes from other transactions that would impact queries with such a predicate. Maybe |
57244ff to
c87f245
Compare
|
This needs more tests but is generally RFAL. One open question: do we want a session variable like |
nvb
left a comment
There was a problem hiding this comment.
Looks great!
Reviewed 22 of 22 files at r2, all commit messages.
Reviewable status:complete! 0 of 0 LGTMs obtained (waiting on @DrewKimball, @mgartner, and @michae2)
pkg/sql/opt/optbuilder/mutation_builder_unique.go line 418 at r2 (raw file):
// Unique checks must ensure the non-existence of certain rows // (i.e. they error on semi-join instead of anti-join) so we must use // predicate locks instead of record locks to prevent insertion of new
"prevent insertion of new rows into the locked span(s) by other concurrent transactions."
pkg/sql/opt/exec/execbuilder/testdata/unique line 4 at r2 (raw file):
statement ok SET experimental_enable_unique_without_index_constraints = true
What allowed us to remove this? It looks like the default for experimental_enable_unique_without_index_constraints is still false.
c87f245 to
8ab5a63
Compare
michae2
left a comment
There was a problem hiding this comment.
Added more tests and locking during UPSERT and INSERT ON CONFLICT. I think this is RFAL.
Reviewable status:
pkg/sql/opt/optbuilder/mutation_builder_unique.go line 418 at r2 (raw file):
Previously, nvanbenschoten (Nathan VanBenschoten) wrote…
"prevent insertion of new rows into the locked span(s) by other concurrent transactions."
Done.
pkg/sql/opt/exec/execbuilder/testdata/unique line 4 at r2 (raw file):
Previously, nvanbenschoten (Nathan VanBenschoten) wrote…
What allowed us to remove this? It looks like the default for
experimental_enable_unique_without_index_constraintsis still false.
Oh, good catch, I deleted this by accident.
nvb
left a comment
There was a problem hiding this comment.
Reviewed 39 of 39 files at r3, all commit messages.
Reviewable status:
pkg/sql/opt/optbuilder/mutation_builder_arbiter.go line 299 at r3 (raw file):
// use predicate locks instead of record locks to prevent insertion of // new rows into the locked span(s) by other concurrent transactions. Form: tree.LockPredicate,
Where will we make the distinction between single-record predicate locks and multi-record predicate locks? Is the thinking to push that all the way down to the logic that selects between Get and Scan? Or to keep it up in optbuilder?
pkg/ccl/logictestccl/testdata/logic_test/regional_by_row_read_committed line 44 at r3 (raw file):
UNIQUE INDEX (origin), INVERTED INDEX (location), FAMILY (name, origin, location, region)
This reminds me, do we have an issue describing the changes we'll need to make to support rows with multiple column families under weak isolation?
pkg/sql/logictest/testdata/logic_test/unique_read_committed line 102 at r3 (raw file):
) statement error pgcode 0A000 guaranteed-durable locking not yet implemented
This will eventually become the "unsupported multi-key predicate lock" error, right?
mgartner
left a comment
There was a problem hiding this comment.
Reviewed 4 of 22 files at r2, 36 of 39 files at r3, all commit messages.
Reviewable status:
pkg/sql/opt/optbuilder/mutation_builder_arbiter.go line 299 at r3 (raw file):
Previously, nvanbenschoten (Nathan VanBenschoten) wrote…
Where will we make the distinction between single-record predicate locks and multi-record predicate locks? Is the thinking to push that all the way down to the logic that selects between Get and Scan? Or to keep it up in optbuilder?
If tree.LockPredicate is only allowed on single-key spans, this anti-join will have to be converted to a lookup anti-join in order for the locking to work, correct? What happens if the optimized picks a plan without a lookup anti-join?
pkg/sql/opt/optbuilder/mutation_builder_arbiter.go line 403 at r3 (raw file):
// use predicate locks instead of record locks to prevent insertion of // new rows into the locked span(s) by other concurrent transactions. Form: tree.LockPredicate,
Similar to my question in the other thread—how do we ensure we'll have single-key spans for this lock form?
pkg/sql/opt/optbuilder/mutation_builder_unique.go line 423 at r3 (raw file):
// predicate locks instead of record locks to prevent insertion of new // rows into the locked span(s) by other concurrent transactions. Form: tree.LockPredicate,
And this will require a constrained scan, correct?
8ab5a63 to
c567195
Compare
michae2
left a comment
There was a problem hiding this comment.
Ok, a few more changes:
- Added join hints so that we're forcing lookup joins.
- Changed the locking strength of the UPSERT / INSERT ON CONFLICT UPDATE arbiter to be for-update instead of for-share, since we'll be modifying the row if it exists. I'm not sure this is strictly necessary but I think it avoids a lock upgrade.
- Added some missing validation to
validateLockingInFrom.
As mentioned below I might need to fix #108489 for us to always use Gets for these checks, but if so I'd prefer to do that in another PR.
So this should be RFAL.
Reviewable status:
pkg/sql/opt/optbuilder/mutation_builder_arbiter.go line 299 at r3 (raw file):
Where will we make the distinction between single-record predicate locks and multi-record predicate locks?
I think this will have to be after optimization, because I believe there are some exploration rules that must fire to get single-key spans for RBR tables (e.g. SplitLimitedScanIntoUnionScans and SplitLimitedSelectIntoUnionSelects). It could be as late as kv_batch_fetcher during execution, but that would allow some queries with multi-key predicate locks to succeed (if they never have to create the lock request) which would be confusing. So probably in execbuilder.
If
tree.LockPredicateis only allowed on single-key spans, this anti-join will have to be converted to a lookup anti-join in order for the locking to work, correct?
Oh, good point! I've added lookup join hints to all of these joins.
I'm currently trying to determine if #108489 is needed in order to always use Get requests for these checks, or if what we have is sufficient. But if we do need that, I would prefer to do it in another PR.
pkg/sql/opt/optbuilder/mutation_builder_arbiter.go line 403 at r3 (raw file):
Previously, mgartner (Marcus Gartner) wrote…
Similar to my question in the other thread—how do we ensure we'll have single-key spans for this lock form?
Added a lookup join hint, which is the first part.
pkg/sql/opt/optbuilder/mutation_builder_unique.go line 423 at r3 (raw file):
Previously, mgartner (Marcus Gartner) wrote…
And this will require a constrained scan, correct?
Yes, for all of these checks, if we cannot form them as constrained scans (well, specifically, as lookup joins on single-key spans) then we won't be able to execute them under RC.
pkg/sql/logictest/testdata/logic_test/unique_read_committed line 102 at r3 (raw file):
Previously, nvanbenschoten (Nathan VanBenschoten) wrote…
This will eventually become the "unsupported multi-key predicate lock" error, right?
Yes, exactly.
nvb
left a comment
There was a problem hiding this comment.
Reviewed 37 of 37 files at r4, all commit messages.
Reviewable status:
pkg/sql/opt/optbuilder/select.go line 1476 at r4 (raw file):
// Exclusive locking on the entire row. case tree.ForNoKeyUpdate: // Exclusive locking on only non-key(s) of the row. Currently unimplemented.
"Currently unimplemented and treated identically to ForUpdate".
pkg/sql/opt/optbuilder/select.go line 1480 at r4 (raw file):
// Shared locking on the entire row. case tree.ForKeyShare: // Shared locking on only key(s) of the row. Currently unimplemented.
"Currently unimplemented and treated identically to ForShare".
mgartner
left a comment
There was a problem hiding this comment.
Reviewed 37 of 37 files at r4, all commit messages.
Reviewable status:
pkg/sql/opt/optbuilder/mutation_builder_unique.go line 423 at r3 (raw file):
Previously, michae2 (Michael Erickson) wrote…
Yes, for all of these checks, if we cannot form them as constrained scans (well, specifically, as lookup joins on single-key spans) then we won't be able to execute them under RC.
If there are known cases that we cannot handle we should add some tests and make sure the error messages make sense. I'm worried an error message like "could not produce a plan that conforms to the hints" will be very confusing when the user didn't use explicit hints.
a08bec0 to
0457c56
Compare
Unique constraints not directly enforced by a unique index are maintained using unique checks. These unique checks verify the non-existence of conflicting rows during mutation statements. We use unique checks for `UNIQUE WITHOUT INDEX` constraints and `UNIQUE` constraints on `PARTITION ALL BY` and `REGIONAL BY ROW` tables. For example, the following insert must use unique checks to verify that there is no conflicting username in any region. ``` CREATE TABLE users ( username STRING NOT NULL UNIQUE, ... ) LOCALITY REGIONAL BY ROW; INSERT INTO users VALUES (...); ``` Under snapshot and read committed isolation, we must lock during all system-maintained constraint checks to prevent concurrent transactions from violating the constraints. This commit adds locking to unique checks, both when those checks verify the constraint and when those checks are used as arbiters in `UPSERT` and `INSERT ON CONFLICT` statements. The locking is: - guaranteed-durable, because it is required for correctness - for-share locking, because we only need to prevent modification of the checked rows, not prevent reading - predicate locking, because we must also prevent insertion of new rows matching the scanned span This last requirement leads us to create a new locking property, locking form, which distinguishes predicate locking from record locking. This will eventually be passed down to the KV layer but is currently only an optimizer property. Fixes: cockroachdb#100156 Epic: CRDB-25322 Release note: None
0457c56 to
0aa36db
Compare
|
Failure is #110645 so I'll go ahead and merge. There will be a follow-up PR. TFTRs! bors r=nvanbenschoten,mgartner |
|
Build succeeded: |
|
Follow-up PR is #110879 which disables unique checks under Read Committed. |
110879: sql: disallow explicit unique checks under read committed isolation r=mgartner,DrewKimball a=michae2 (This is a follow-on PR after #107961.) Because we do not yet support predicate locking, add a check to execbuilder that disallows any query using it (currently only unique checks under read committed isolation). Informs: #110873 Release note (sql): We do not yet support explicit unique checks under Read Committed isolation. This means that some `INSERT`, `UPDATE`, and `UPSERT` statements against some `REGIONAL BY ROW` tables will fail under Read Committed isolation with the following error: ``` unimplemented: explicit unique checks are not yet supported under read committed isolation SQLSTATE: 0A000 ``` For more details about which `REGIONAL BY ROW` tables are affected, please see: https://go.crdb.dev/issue-v/110873/v23.2 Co-authored-by: Michael Erickson <michae2@cockroachlabs.com>
Unique constraints not directly enforced by a unique index are maintained using unique checks. These unique checks verify the non-existence of conflicting rows during mutation statements. We use unique checks for
UNIQUE WITHOUT INDEXconstraints andUNIQUEconstraints onPARTITION ALL BYandREGIONAL BY ROWtables.For example, the following insert must use unique checks to verify that there is no conflicting username in any region.
Under snapshot and read committed isolation, we must lock during all system-maintained constraint checks to prevent concurrent transactions from violating the constraints. This commit adds locking to unique checks, both when those checks verify the constraint and when those checks are used as arbiters in
UPSERTandINSERT ON CONFLICTstatements.The locking is:
This last requirement leads us to create a new locking property, locking form, which distinguishes predicate locking from record locking. This will eventually be passed down to the KV layer but is currently only an optimizer property.
Fixes: #100156
Epic: CRDB-25322
Release note: None