sql: add implicit SELECT FOR SHARE locking to FK checks

[michae2]

explain: add transaction information to EXPLAIN ANALYZE

Add transaction isolation, priority, and quality-of-service to the
output of EXPLAIN ANALYZE.

Release note (sql change): EXPLAIN ANALYZE output now includes:

• the isolation level of the statement's transaction
• the priority of the statement's transaction
• the quality of service level of the statement's transaction

---

opt: do not use LockDurabilityGuaranteed under serializable isolation

This is a follow-up from #103734.

We do not want to use guaranteed-durable (a.k.a. replicated) locking
under serializable isolation, because it prevents pipelining and other
optimizations, and is unnecessary for correctness. This commit amends
8cbc6d1 to only set durability for
SELECT FOR UPDATE locking under weaker isolation levels.

This means that query plans will be slightly different under different
isolation levels, and so we must add isolation level to the optimizer
memo staleness calculation.

Furthermore, this commit changes the error message added by
e633d5e to be about guaranteed-durable
locking rather than SELECT FOR UPDATE, because in a later commit this
specific error will also be triggered by foreign key checks under weaker
isolation levels.

Informs: #100144, #100156, #100193, #100194

Release note: None

---

opt: show locking durability in EXPLAIN (OPT) output

Because the "guaranteed-durable locking not yet implemented" error
condition is checked in execbuilder, it prevents not only execution but
also EXPLAIN of queries using guaranteed-durable locking. Thankfully
EXPLAIN (OPT) bypasses execbuilder, and hence still works, so use this
for now to verify that we are enabling durable locking for SELECT FOR UPDATE under read committed isolation.

(Note that we have not yet fixed the SELECT FOR UPDATE plans to use
more precise locking, that will come in a later PR.)

Informs: #100194

Release note: None

---

sql: add implicit SELECT FOR SHARE locking to FK parent checks

Add SELECT FOR SHARE locking to FK parent checks. Under serializable
isolation, this locking is only used when
enable_implicit_fk_locking_for_serializable is set. Under weaker
isolation levels (snapshot and read committed) this locking is always
used.

We only need to lock during the insertion-side FK checks, which verify
the existence of a parent row. Deletion-side FK checks verify the
non-existence of a child row, and these do not need to lock. Instead, to
prevent concurrent inserts or updates to the child that would violate
the FK constraint, we rely on the intent(s) created by the deletion
conflicting with the FK locking of those concurrent inserts or updates.

Fixes: #80683
Informs: #100156

Epic: CRDB-25322

Release note (sql change): Add a new session variable,
enable_implicit_fk_locking_for_serializable, which controls locking
during foreign key checks under serializable isolation. With this set to
true, foreign key checks of the referenced (parent) table, such as those
performed during an INSERT or UPDATE of the referencing (child) table,
will lock the referenced row using SELECT FOR SHARE locking. (This is
somewhat analogous to the existing enable_implicit_select_for_update
variable but applies to the foreign key checks of a mutation statement
instead of the initial row fetch.)

Under weaker isolation levels such as read committed, SELECT FOR SHARE
locking will always be used to ensure the database maintains the foreign
key constraint, regardless of the current setting of
enable_implicit_fk_locking_for_serializable.

[cockroach-teamcity]

This change is 

[michae2]

We've gone from "duable" to "durable" so this is now RFAL. 🙂

[DrewKimball]

Nice work! apart from a few nits.

Reviewed 11 of 11 files at r1, 12 of 12 files at r2, 3 of 3 files at r3, 28 of 28 files at r4, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @mgartner, @michae2, @nvanbenschoten, and @rytaft)

---

pkg/sql/opt/optbuilder/mutation_builder_fk.go line 616 at r4 (raw file):

// buildOtherTableScan builds a Scan of the "other" table.
func (h *fkCheckHelper) buildOtherTableScan(parent bool) (outScope *scope, tabMeta *opt.TableMeta) {

[nit] Maybe explain what parent means.

---

pkg/sql/row/locking.go line 47 at r4 (raw file):

}

// getWaitPolicy returns the configured lock wait policy to use for key-value

[nit] getWaitPolicy -> GetWaitPolicy

---

pkg/sql/opt/exec/execbuilder/testdata/fk line 417 at r4 (raw file):

# Even with implicit locking enabled, we should not acquire any shared locks
# when checking a delete from the parent.

Could you explain why the shared locks are unnecessary? Is it because inserts/updates to the child table will already check for existence of the parent row?

---

pkg/sql/opt/exec/execbuilder/testdata/fk_read_committed line 24 at r4 (raw file):

           └── anti-join (lookup jars)
                ├── lookup columns are key
                ├── locking: for-share,duability-guaranteed

This says duability still :)

[rytaft]

Nice work! Just a couple of questions.

Reviewed 11 of 11 files at r1, 12 of 12 files at r2, 3 of 3 files at r3, 28 of 28 files at r4, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @mgartner, @michae2, and @nvanbenschoten)

---

pkg/sql/conn_executor_exec.go line 2931 at r1 (raw file):

		// add qos?
		// add asoftime?
		// add readonly?

Did you mean to actually add these here? If not, this should probably be a TODO with your name / issue number

---

pkg/sql/logictest/testdata/logic_test/fk_read_committed line 35 at r4 (raw file):

UPDATE cookies SET j = 2 WHERE c = 1

# Foreign key checks of the child do not require locking.

Why not? What happens if you update or delete a jar that has no cookies in it, but a concurrent RC transaction tries to insert a cookie?

[michae2]

Thanks for the reviews! Fixed the nits and added more tests and comments.

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @DrewKimball, @mgartner, @nvanbenschoten, and @rytaft)

---

pkg/sql/conn_executor_exec.go line 2931 at r1 (raw file):

Previously, rytaft (Rebecca Taft) wrote…

Did you mean to actually add these here? If not, this should probably be a TODO with your name / issue number

Oh, whoops, I forgot about these. Opened #107318 and added TODOs.

---

pkg/sql/opt/optbuilder/mutation_builder_fk.go line 616 at r4 (raw file):

Previously, DrewKimball (Drew Kimball) wrote…

[nit] Maybe explain what parent means.

Good call, done.

---

pkg/sql/row/locking.go line 47 at r4 (raw file):

Previously, DrewKimball (Drew Kimball) wrote…

[nit] getWaitPolicy -> GetWaitPolicy

Done.

---

pkg/sql/logictest/testdata/logic_test/fk_read_committed line 35 at r4 (raw file):

Previously, rytaft (Rebecca Taft) wrote…

Why not? What happens if you update or delete a jar that has no cookies in it, but a concurrent RC transaction tries to insert a cookie?

The concurrent RC transaction will perform its own FK check of the parent row, which will acquire a lock on the parent jar. This lock will conflict with the update or delete of the parent jar.

This is a bit tricky so I added some comments in various places about it.

---

pkg/sql/opt/exec/execbuilder/testdata/fk line 417 at r4 (raw file):

Previously, DrewKimball (Drew Kimball) wrote…

Could you explain why the shared locks are unnecessary? Is it because inserts/updates to the child table will already check for existence of the parent row?

Yes, exactly. Inserts/updates to the child table will do their own FK checks which will acquire locks on the parent row. Those locks will conflict with deletions from the parent table, preventing violations of the FK constraint.

This is a bit tricky so I added more comments about it.

---

pkg/sql/opt/exec/execbuilder/testdata/fk_read_committed line 24 at r4 (raw file):

Previously, DrewKimball (Drew Kimball) wrote…

This says duability still :)

Gah, thank you. Done.

[nvanbenschoten]

Very nice!

Reviewed 10 of 11 files at r1, 40 of 40 files at r5, 12 of 12 files at r6, 3 of 3 files at r7, 31 of 32 files at r8, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (and 1 stale) (waiting on @DrewKimball, @mgartner, @michae2, and @rytaft)

---

pkg/sql/insert_fast_path.go line 194 at r8 (raw file):

		lockWaitPolicy := row.GetWaitPolicy(descpb.ToScanLockingWaitPolicy(c.Locking.WaitPolicy))
		// All FK check lock wait policies should be BLOCK.
		if r.fkBatch.Header.WaitPolicy != lockWaitPolicy {

nit: why compare against r.fkBatch.Header.WaitPolicy (which we assume has not been set yet) instead of directly against lock.WaitPolicy_Block?

---

pkg/sql/vars.go line 2805 at r8 (raw file):

	// CockroachDB extension.
	`enable_implicit_fk_locking`: {

Should this setting mention "serializable" somehow, since setting it to false is ignored for weaker isolation levels?

---

pkg/sql/opt/exec/explain/output.go line 415 at r5 (raw file):

	txnIsoLevel isolation.Level, txnPriority roachpb.UserPriority, txnQoSLevel sessiondatapb.QoSLevel,
) {
	ob.AddTopLevelField("isolation level", txnIsoLevel.String())

If we want to match the other fields in the output, we could use StringLower() here.

---

pkg/sql/opt/memo/expr_format.go line 1563 at r7 (raw file):

	case tree.LockDurabilityBestEffort:
	case tree.LockDurabilityGuaranteed:
		durability = ",durability-guaranteed"

nit: should we add default: panic(errors.AssertionFailedf("unexpected durability")) to catch new durability levels?

---

pkg/sql/opt/optbuilder/mutation_builder_fk.go line 650 at r8 (raw file):

		locking = lockingSpec{
			&tree.LockingItem{
				// TODO(michae2): Change this to ForKeyShare when it is supported.

You could set it to row.ForKeyShare now if you'd like. That's handled the same as tree.ForShare in row.GetKeyLockingStrength.

But also, feel free to leave this as is if you think it would be confusing to users, given that the difference doesn't matter yet.

---

pkg/sql/opt/optbuilder/select.go line 694 at r6 (raw file):

	if locking.isSet() {
		private.Locking = locking.get()
		if b.evalCtx.TxnIsoLevel != isolation.Serializable {

Did we want to expose a cluster setting and/or session var to allow users to opt-in to guaranteed lock durability for SFU under Serializable isolation?

---

pkg/sql/sessiondatapb/local_only_session_data.proto line 410 at r8 (raw file):

  // ImplicitFKLocking is true if FOR SHARE locking may be used while checking
  // the referenced table during an insert or update to a table with a foreign
  // key.

Same point about qualifying this somehow with the fact that it only applies to serializable.

[michae2]

Reviewable status: complete! 1 of 0 LGTMs obtained (and 1 stale) (waiting on @DrewKimball, @mgartner, @nvanbenschoten, and @rytaft)

---

pkg/sql/opt/optbuilder/select.go line 694 at r6 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

Did we want to expose a cluster setting and/or session var to allow users to opt-in to guaranteed lock durability for SFU under Serializable isolation?

Oh, good idea! I'm thinking a session var. @nvanbenschoten do you have any preferences about the name of the setting? How about "lock_durability" or something?

[nvanbenschoten]

Reviewable status: complete! 1 of 0 LGTMs obtained (and 1 stale) (waiting on @DrewKimball, @mgartner, @michae2, and @rytaft)

---

pkg/sql/opt/optbuilder/select.go line 694 at r6 (raw file):

Previously, michae2 (Michael Erickson) wrote…

Oh, good idea! I'm thinking a session var. @nvanbenschoten do you have any preferences about the name of the setting? How about "lock_durability" or something?

Maybe enable_durable_select_for_update_locks? If possible, making the variable boolean-valued seems like the way to go, as it makes the knob easier to use.

I think we'll also want to mention something about "serializable", like we'll want to do for enable_implicit_fk_locking (see other comment). Maybe suffix both with _with_serializable, or something like that.

[rytaft]

Reviewed 40 of 40 files at r5, 12 of 12 files at r6, 3 of 3 files at r7, 32 of 32 files at r8, all commit messages.
Reviewable status: complete! 2 of 0 LGTMs obtained (and 1 stale) (waiting on @DrewKimball, @mgartner, and @michae2)

---

pkg/sql/logictest/testdata/logic_test/fk_read_committed line 35 at r4 (raw file):

Previously, michae2 (Michael Erickson) wrote…

The concurrent RC transaction will perform its own FK check of the parent row, which will acquire a lock on the parent jar. This lock will conflict with the update or delete of the parent jar.

This is a bit tricky so I added some comments in various places about it.

Thanks for the explanation!

[michae2]

TFTRs!

Reviewable status: complete! 0 of 0 LGTMs obtained (and 3 stale) (waiting on @DrewKimball, @mgartner, @nvanbenschoten, and @rytaft)

---

pkg/sql/insert_fast_path.go line 194 at r8 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

nit: why compare against r.fkBatch.Header.WaitPolicy (which we assume has not been set yet) instead of directly against lock.WaitPolicy_Block?

We're comparing against the header because there's an impedance mismatch between the r.fkChecks[i].Locking.WaitPolicy which is per-fk-check and the r.fkBatch.Header.WaitPolicy which is per-batch (one for all the fk checks). So just verifying that they all match whatever the batch is set to.

I changed addFKChecks to now explicitly set r.fkBatch.Header.WaitPolicy to blocking (was relying on it being implicitly set to blocking during defer n.run.fkBatch.Reset() in runFKChecks).

---

pkg/sql/vars.go line 2805 at r8 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

Should this setting mention "serializable" somehow, since setting it to false is ignored for weaker isolation levels?

Good point, I added "_for_serializable" to be similar to Rafi's "max_retries_for_read_committed_transactions" in #107044.

---

pkg/sql/opt/exec/explain/output.go line 415 at r5 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

If we want to match the other fields in the output, we could use StringLower() here.

Oh! Thank you! Done.

---

pkg/sql/opt/memo/expr_format.go line 1563 at r7 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

nit: should we add default: panic(errors.AssertionFailedf("unexpected durability")) to catch new durability levels?

Good catch, done.

---

pkg/sql/opt/optbuilder/mutation_builder_fk.go line 650 at r8 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

You could set it to row.ForKeyShare now if you'd like. That's handled the same as tree.ForShare in row.GetKeyLockingStrength.

But also, feel free to leave this as is if you think it would be confusing to users, given that the difference doesn't matter yet.

Since this will be shown in EXPLAIN output, it felt wrong to show FOR KEY SHARE if we're not yet doing it, so I kept it this way for now.

---

pkg/sql/opt/optbuilder/select.go line 694 at r6 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

Maybe enable_durable_select_for_update_locks? If possible, making the variable boolean-valued seems like the way to go, as it makes the knob easier to use.

I think we'll also want to mention something about "serializable", like we'll want to do for enable_implicit_fk_locking (see other comment). Maybe suffix both with _with_serializable, or something like that.

I started doing this but it got a little long, will do it in the next PR.

---

pkg/sql/sessiondatapb/local_only_session_data.proto line 410 at r8 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

Same point about qualifying this somehow with the fact that it only applies to serializable.

Done.

[mgartner]

Great work! I appreciate the thoughtful organization of commits - it made reviewing pleasant.

Reviewed 10 of 11 files at r1, 40 of 40 files at r5, 12 of 12 files at r6, 3 of 3 files at r7, 32 of 32 files at r8, 52 of 52 files at r9, 12 of 12 files at r10, 3 of 3 files at r11, 32 of 32 files at r12, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (and 3 stale) (waiting on @DrewKimball and @michae2)

---

pkg/sql/opt/exec/execbuilder/testdata/fk_read_committed line 86 at r8 (raw file):

                └── filters (true)

# Foreign key checks of the child do not require locking.

Is this explained somewhere in more detail? Is this safe because any updates to the child rows would lock this parent row?

[michae2]

Thanks, all! This should be ready now...

bors r=DrewKimball,nvanbenschoten,rytaft,mgartner

Reviewable status: complete! 0 of 0 LGTMs obtained (and 4 stale) (waiting on @DrewKimball, @mgartner, @nvanbenschoten, and @rytaft)

---

pkg/sql/opt/exec/execbuilder/testdata/fk_read_committed line 86 at r8 (raw file):

Previously, mgartner (Marcus Gartner) wrote…

Is this explained somewhere in more detail? Is this safe because any updates to the child rows would lock this parent row?

Yes, that's correct. It is explained in a little more detail in comments in pkg/sql/opt/optbuilder/mutation_builder_fk.go as well as in the 4th commit message.

[craig]

Build succeeded:

• Bazel Essential CI (Cockroach)

[michae2]

Maybe enable_durable_select_for_update_locks? If possible, making the variable boolean-valued seems like the way to go, as it makes the knob easier to use.

I think we'll also want to mention something about "serializable", like we'll want to do for enable_implicit_fk_locking (see other comment). Maybe suffix both with _with_serializable, or something like that.

I started doing this but it got a little long, will do it in the next PR.

Follow-up PR is: #107749