opt: add implicit FOR UPDATE locks for generic query plans

[mgartner]

Implicit FOR UPDATE locks are now added to the reads in
INSERT .. ON CONFLICT, UPSERT, UPDATE, and DELETE queries when
those mutations use generic query plans.

Fixes #137352

Release note: None

[cockroach-teamcity]

This change is 

[mgartner]

I added the tests for DELETE, but figured I'd wait until #137069 is merged to add support for generic query plans.

[rytaft]

Nice!

Reviewed 4 of 4 files at r1, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @mgartner, @michae2, and @yuzefovich)

---

pkg/sql/opt/exec/execbuilder/mutation.go line 1203 at r1 (raw file):

		// same as the table being updated. Also, don't lock rows if there is an
		// ON condition so that we don't lock rows that won't be updated.
		if updateTableStableID == lookupTableStableID && t.On.IsTrue() {

Can you add a test to show that we don't apply the lock if ON is not true?

---

pkg/sql/opt/exec/execbuilder/mutation.go line 1237 at r1 (raw file):

	case *memo.LookupJoinExpr:
		input = join.Input
		if inner, ok := input.(*memo.LookupJoinExpr); ok && inner.Table == join.Table {

nit: add a comment to explain this

[michae2]

Nice fix!

Reviewed 2 of 4 files at r1, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @mgartner and @yuzefovich)

---

-- commits line 4 at r1:
Do INSERT statements use implicit for update locks? I didn't think they did, but maybe they do for ON CONFLICT?

---

pkg/sql/opt/exec/execbuilder/mutation.go line 1196 at r1 (raw file):

	case *memo.ScanExpr:
		return t.Table
	case *memo.LookupJoinExpr:

This will also add implicit locking in custom plans for statements like the following, which did not have it before:

CREATE TABLE ab (a INT PRIMARY KEY, b INT);
CREATE TABLE cd (c INT PRIMARY KEY, d INT);
UPDATE ab SET b = b + 1 FROM cd WHERE a = d AND c = 1;
UPDATE ab AS ab1 SET b = ab1.b + 1 FROM ab AS ab2 WHERE ab1.a = ab2.b + 1 AND ab2.a = 1;

I think this is fine for the first statement, which will only lock rows that are modified, but I'm not sure about the second statement. The lock on the initial scan of ab2 could lock a row is not going to be updated.

[michae2]

Reviewed 2 of 4 files at r1.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @mgartner and @yuzefovich)

---

pkg/sql/opt/exec/execbuilder/mutation.go line 1196 at r1 (raw file):

Previously, michae2 (Michael Erickson) wrote…

This will also add implicit locking in custom plans for statements like the following, which did not have it before:

CREATE TABLE ab (a INT PRIMARY KEY, b INT);
CREATE TABLE cd (c INT PRIMARY KEY, d INT);
UPDATE ab SET b = b + 1 FROM cd WHERE a = d AND c = 1;
UPDATE ab AS ab1 SET b = ab1.b + 1 FROM ab AS ab2 WHERE ab1.a = ab2.b + 1 AND ab2.a = 1;

I think this is fine for the first statement, which will only lock rows that are modified, but I'm not sure about the second statement. The lock on the initial scan of ab2 could lock a row is not going to be updated.

Oh, I take it back, the second statement won't add a lock on the scan of ab2. So never mind, this seems good.

[yuzefovich]

Nice!

Reviewed 4 of 4 files at r1, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @mgartner and @michae2)

---

-- commits line 4 at r1:

Previously, michae2 (Michael Erickson) wrote…

Do INSERT statements use implicit for update locks? I didn't think they did, but maybe they do for ON CONFLICT?

No, I don't think we use implicit SFU for any type of INSERT.

---

pkg/sql/opt/exec/execbuilder/mutation.go line 1184 at r1 (raw file):

	}

	// Try to match the Update's input expression against the pattern:

nit: consider updating this comment to match the new pattern.

---

pkg/sql/opt/exec/execbuilder/mutation.go line 1204 at r1 (raw file):

		// ON condition so that we don't lock rows that won't be updated.
		if updateTableStableID == lookupTableStableID && t.On.IsTrue() {
			return t.Table

I wonder whether we should be a bit more restrictive here, i.e. should we assert a particular shape of the input to LookupJoinExpr (for example, that it's a ValuesExpr or another LookupJoinExpr followed by ValuesExpr)? I can't think of case where the current code could lead to locking unnecessary rows, but we seem to have been more prescriptive in the existing cases where we apply implicit SFU.

[mgartner]

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

---

-- commits line 4 at r1:

Previously, yuzefovich (Yahor Yuzefovich) wrote…

No, I don't think we use implicit SFU for any type of INSERT.

Fixed.

---

pkg/sql/opt/exec/execbuilder/mutation.go line 1184 at r1 (raw file):

Previously, yuzefovich (Yahor Yuzefovich) wrote…

nit: consider updating this comment to match the new pattern.

Done.

---

pkg/sql/opt/exec/execbuilder/mutation.go line 1196 at r1 (raw file):

Previously, michae2 (Michael Erickson) wrote…

Oh, I take it back, the second statement won't add a lock on the scan of ab2. So never mind, this seems good.

I added a test for the second case with a TODO because we don't use any locks in that case.

---

pkg/sql/opt/exec/execbuilder/mutation.go line 1203 at r1 (raw file):

Previously, rytaft (Rebecca Taft) wrote…

Can you add a test to show that we don't apply the lock if ON is not true?

Done.

---

pkg/sql/opt/exec/execbuilder/mutation.go line 1204 at r1 (raw file):

Previously, yuzefovich (Yahor Yuzefovich) wrote…

I wonder whether we should be a bit more restrictive here, i.e. should we assert a particular shape of the input to LookupJoinExpr (for example, that it's a ValuesExpr or another LookupJoinExpr followed by ValuesExpr)? I can't think of case where the current code could lead to locking unnecessary rows, but we seem to have been more prescriptive in the existing cases where we apply implicit SFU.

Done.

---

pkg/sql/opt/exec/execbuilder/mutation.go line 1237 at r1 (raw file):

Previously, rytaft (Rebecca Taft) wrote…

nit: add a comment to explain this

Done.

[yuzefovich]

Nice!

Reviewed 2 of 5 files at r2, 3 of 3 files at r3, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (and 1 stale) (waiting on @michae2 and @rytaft)

[mgartner]

TFTRs!

bors r+

[craig]

👎 Rejected by code reviews

[mgartner]

bors r+

[craig]

👎 Rejected by code reviews

Concerns have been addressed.

[yuzefovich]

(Becca's review had the "requested changes" status which was blocking the merge.)

bors r+

[craig]

Build succeeded:

• linux_amd64_fips_build
• windows_build
• acceptance
• unit_tests
• linux_amd64_build
• docker_image_amd64
• macos_arm64_build
• linux_arm64_build
• check_generated_code
• local_roachtest_fips
• lint
• macos_amd64_build
• examples_orms
• local_roachtest

[mgartner]

(Becca's review had the "requested changes" status which was blocking the merge.)

Ah, thanks!