Allow fast path insert into unique indices with implicit partitioning columns under read-committed

[mw5h]

See commit messages for details.

[cockroach-teamcity]

This change is 

[DrewKimball]

BTW, @mgartner has a nice way to generate a PR message using commits, described here.

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

---

-- commits line 14 at r3:
Maybe just a Reviewable artifact, but it looks like a commit was duplicated, or something.

---

pkg/sql/insert_fast_path.go line 220 at r4 (raw file):

				if c.Locking.Form == tree.LockPredicate {
					if !isInputRow {
						err := row.PutUniqLock(ctx, &r.ti.putter, c.Locking.WaitPolicy, span.Key, r.traceKV)

I believe this will execute the unique checks for the other partitions in the same batch as the insert, rather than with other unique checks / FK checks. Is that intentional? If so, we should call it out in a comment here.

[nit] Along the lines of the above comment, I think it might be helpful to inline the logic from PutUniqueLock here unless there's some dependency issue, so it's clear what request we're sending and in what batch. It's fine if you think it's better this way, though.

---

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

		// If there is a unique index with a implicit partitioning columns, the fast
		// path can write tombstones to lock the row in all partitions.
		allowPredicateLocks := execFastPathCheck.ReferencedTable.Unique(execFastPathCheck.CheckOrdinal).HasIndexWithImplicitPartitioningColumn()

[nit] Since we have access to the index here, is it necessary to add HasIndexWithImplicitPartitioningColumn? Could we just check if ImplicitPartitioningColumnCount() > 0 instead?

---

pkg/sql/opt/exec/execbuilder/relational.go line 3141 at r4 (raw file):

}

// TODO: Delete this function once predicate locks are universally supported.

[nit] Is there an issue we can associate the TODO with, like TODO(#foo):?

[DrewKimball]

Nice work figuring this out!

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @michae2 and @mw5h)

[michae2]

Awesome!! 🔥 This is tricky stuff!

It's very satisfying that the error message for the tombstone CPut works exactly for the whole INSERT statement!

Reviewed 1 of 1 files at r1, 8 of 8 files at r3, 8 of 8 files at r4, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @DrewKimball and @mw5h)

---

-- commits line 47 at r4:
nit: Maybe add "when using the insert fast path" to make it clear when it applies.

---

pkg/sql/insert_fast_path.go line 220 at r4 (raw file):

Previously, DrewKimball (Drew Kimball) wrote…

I believe this will execute the unique checks for the other partitions in the same batch as the insert, rather than with other unique checks / FK checks. Is that intentional? If so, we should call it out in a comment here.

[nit] Along the lines of the above comment, I think it might be helpful to inline the logic from PutUniqueLock here unless there's some dependency issue, so it's clear what request we're sending and in what batch. It's fine if you think it's better this way, though.

I was wondering about this, too. I'm guessing it's because the logic to handle the response is not the same as in runUniqChecks (because the tombstone CPut error message can simply become the statement error message) so it doesn't make sense to include in the uniqBatch. Agree that a comment would help.

---

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

		execFastPathCheck.CheckOrdinal = c.CheckOrdinal

		// If there is a unique index with a implicit partitioning columns, the fast

typo: "implicit" instead of "a implicit"

---

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

Previously, DrewKimball (Drew Kimball) wrote…

[nit] Since we have access to the index here, is it necessary to add HasIndexWithImplicitPartitioningColumn? Could we just check if ImplicitPartitioningColumnCount() > 0 instead?

Is there a reason we can't always use true here? Oh, right, is it because this tombstone technique only works for single-row predicate locks, and unique checks from UNIQUE WITHOUT INDEX might require range predicate locks? 🤔

I wonder if checking for implicit partitioning is enough to always prevent range predicate locks? For example, would it catch something like this? (Which I think will have a unique check for the UNIQUE WITHOUT INDEX (b) that uses the implicitly-partitioned index on (b, c).)

SET experimental_enable_unique_without_index_constraints = true;
SET experimental_enable_implicit_column_partitioning = true;

CREATE TABLE abc (
  a INT NOT NULL CHECK (a IN (4, 5, 6)),
  b INT NOT NULL,
  c INT NOT NULL,
  INDEX (b, c),
  UNIQUE WITHOUT INDEX (b)
) PARTITION ALL BY LIST (a) (
  PARTITION p1 VALUES IN ((4)),
  PARTITION p2 VALUES IN ((5)),
  PARTITION p3 VALUES IN ((6))
);
EXPLAIN INSERT INTO abc VALUES (6, 7, 8);

---

pkg/sql/opt/exec/execbuilder/relational.go line 3143 at r4 (raw file):

// TODO: Delete this function once predicate locks are universally supported.
func (b *Builder) buildLocking(toLock opt.TableID, locking opt.Locking) (opt.Locking, error) {
	return b.buildLockingImpl(toLock, locking, false)

nit: Usually literal arguments are decorated with a comment like ..., false /* allowPredicateLocks */).

---

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

) error {
	if waitPolicy != tree.LockWaitBlock {
		return errors.Errorf("Non-blocking predicate locks are not implemented")

Could this check be moved to buildLockingImpl?

---

pkg/ccl/logictestccl/testdata/logic_test/regional_by_row_read_committed line 223 at r4 (raw file):

statement async conflict error pgcode 23505 pq: duplicate key value violates unique constraint "university_pkey"
INSERT INTO university VALUES ('CMU', 'Chippewas', '97858');

Haha, I appreciate this!

[mw5h]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @DrewKimball)

---

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

Previously, michae2 (Michael Erickson) wrote…

Is there a reason we can't always use true here? Oh, right, is it because this tombstone technique only works for single-row predicate locks, and unique checks from UNIQUE WITHOUT INDEX might require range predicate locks? 🤔

I wonder if checking for implicit partitioning is enough to always prevent range predicate locks? For example, would it catch something like this? (Which I think will have a unique check for the UNIQUE WITHOUT INDEX (b) that uses the implicitly-partitioned index on (b, c).)

SET experimental_enable_unique_without_index_constraints = true;
SET experimental_enable_implicit_column_partitioning = true;

CREATE TABLE abc (
  a INT NOT NULL CHECK (a IN (4, 5, 6)),
  b INT NOT NULL,
  c INT NOT NULL,
  INDEX (b, c),
  UNIQUE WITHOUT INDEX (b)
) PARTITION ALL BY LIST (a) (
  PARTITION p1 VALUES IN ((4)),
  PARTITION p2 VALUES IN ((5)),
  PARTITION p3 VALUES IN ((6))
);
EXPLAIN INSERT INTO abc VALUES (6, 7, 8);

demo@127.0.0.1:26257/demoapp/movr> SET SESSION CHARACTERISTICS AS TRANSACTION ISOLATION LEVEL READ COMMITTED;
SET

Time: 1ms total (execution 1ms / network 0ms)

demo@127.0.0.1:26257/demoapp/movr> EXPLAIN INSERT INTO abc VALUES (6, 7, 8);
ERROR: unimplemented: explicit unique checks are not yet supported under read committed isolation
SQLSTATE: 0A000
HINT: You have attempted to use a feature that is not yet implemented.
See: https://go.crdb.dev/issue-v/110873/v24.2

[mw5h]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @DarrylWong, @DrewKimball, @herkolategan, @michae2, and @msbutler)

---

-- commits line 14 at r3:

Previously, DrewKimball (Drew Kimball) wrote…

Maybe just a Reviewable artifact, but it looks like a commit was duplicated, or something.

I think it's just Reviewable.

---

pkg/sql/insert_fast_path.go line 220 at r4 (raw file):

Previously, michae2 (Michael Erickson) wrote…

I was wondering about this, too. I'm guessing it's because the logic to handle the response is not the same as in runUniqChecks (because the tombstone CPut error message can simply become the statement error message) so it doesn't make sense to include in the uniqBatch. Agree that a comment would help.

Done.

---

pkg/sql/opt/exec/execbuilder/relational.go line 3141 at r4 (raw file):

Previously, DrewKimball (Drew Kimball) wrote…

[nit] Is there an issue we can associate the TODO with, like TODO(#foo):?

There isn't yet an issue for predicate locks, though maybe I should file one.

[DrewKimball]

Reviewed 1 of 1 files at r8, 4 of 8 files at r9, 10 of 10 files at r11, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @DarrylWong, @herkolategan, @msbutler, and @mw5h)

---

pkg/sql/insert_fast_path.go line 226 at r11 (raw file):

				if c.Locking.Form == tree.LockPredicate {
					if !isInputRow {
						r.ti.putter.CPut(span.Key, nil, nil)

Did you mean to remove the tracing messages from before? Did it turn out that we already write to logs for CPut?

[mw5h]

Did you mean to remove the tracing messages from before? Did it turn out that we already write to logs for CPut?

I did because I thought there was already tracing around the CPut, but I was mistaken.

[michae2]

Very nice!

Reviewed 1 of 1 files at r8, 4 of 8 files at r9, 6 of 10 files at r11, 4 of 4 files at r12, 4 of 4 files at r13, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (and 1 stale) (waiting on @DarrylWong, @DrewKimball, @herkolategan, @msbutler, and @mw5h)

[mw5h]

bors r+

[craig]

Build succeeded:

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