sql/opt: add implicit SELECT FOR SHARE support for FK checks

[michae2]

Similar to #50180 and #50181 we should add implicit SELECT FOR UPDATE locking to the FK checks (and other constraint checks) performed by mutation statements, in accordance with the enable_implicit_select_for_update session variable.

Here's a demonstration:

CREATE TABLE account (id INT PRIMARY KEY, name STRING);
CREATE TABLE transfer (
  from_id INT,
  to_id INT,
  amount INT,
  FOREIGN KEY (from_id) REFERENCES account (id),
  FOREIGN KEY (to_id) REFERENCES account (id)
);
INSERT INTO account VALUES (1, 'Alice'), (2, 'Bob');

-- connection 1 (single explicit transaction)
BEGIN;
SELECT * FROM transfer FOR UPDATE;

-- connection 2 (two implicit transactions)
UPDATE account SET name = 'Carol' WHERE id = 2;
SELECT * FROM transfer;

-- connection 1 (resuming open transaction)
INSERT INTO transfer VALUES (1, 2, 100);
COMMIT;

The FK checks performed by the INSERT do not use SELECT FOR UPDATE and so are at a timestamp before the update transaction in connection 2, and hence impossible to refresh.

[email protected]:26257/defaultdb  OPEN> COMMIT;
ERROR: restart transaction: TransactionRetryWithProtoRefreshError: TransactionRetryError: retry txn (RETRY_SERIALIZABLE - failed preemptive refresh due to a conflict: committed value on key /Table/104/1/2/0): "sql txn" meta={id=bc8069e8 key=/Table/105/1 pri=0.00422485 epo=0 ts=1651120066.267562000,2 min=1651120005.931353000,0 seq=1} lock=true stat=PENDING rts=1651120005.931353000,0 wto=false gul=1651120006.431353000,0
SQLSTATE: 40001
HINT: See: https://www.cockroachlabs.com/docs/v22.1/transaction-retry-error-reference.html#retry_serializable

Jira issue: CRDB-15550

Epic CRDB-25322

[michae2]

(It might help to SET CLUSTER SETTING kv.closed_timestamp.target_duration = '10 min'; in this repro if you want to be sure that the RETRY_SERIALIZABLE error is due to contention with another transaction rather than hitting the closed timestamp interval.)

[nvanbenschoten]

I'm not sure this is quite the same thing as the other cases where we use implicit SELECT FOR UPDATE. In this case, we aren't planning to immediately return and write to the row that we read, so we probably wouldn't want to grab an exclusive lock during the FK check. Doing so would eliminate some retries, but at the expense of reduced concurrency. I think you could make an argument that we could grab implicit SELECT FOR SHARE locks during the read, but is this any different than other reads in CockroachDB, which defaults to optimistic reads over pessimistic reads?

One key area where FK checks differ from standard reads is that they only check for existence of the referenced row — they don't care about the value of the referenced row. This distinction is exploited in Postgres with the FOR KEY SHARE locking mode, which commutes with FOR NO KEY UPDATE locks. This eliminates conflicts between FK checks and updates to non-PK columns in rows. We discuss this and other related ideas in #52420.

[michae2]

I'm not sure this is quite the same thing as the other cases where we use implicit SELECT FOR UPDATE. In this case, we aren't planning to immediately return and write to the row that we read, so we probably wouldn't want to grab an exclusive lock during the FK check. Doing so would eliminate some retries, but at the expense of reduced concurrency. I think you could make an argument that we could grab implicit SELECT FOR SHARE locks during the read,

Good point! Yes, seems like SELECT FOR SHARE locks would be good enough. (Or FOR KEY SHARE locks, cool!)

but is this any different than other reads in CockroachDB, which defaults to optimistic reads over pessimistic reads?

I propose doing this for mutation queries when enable_implicit_select_for_update is true. I think in that case the user is choosing (or defaulting into) pessimism over optimism, and we should respect that.

[nvanbenschoten]

enable_implicit_select_for_update defaults to true, so I don't think it's an indication that they're choosing pessimism over optimism. I think (and hope!) most users aren't even aware that there's a choice to be made — things should just work.

I'm curious whether you think there's a meaningful difference between the reads (e.g. FK checks) performed in a mutation query and the reads performed by other queries in a read-write transaction. Are reads in a mutation query more likely to be contended? Are reads in a mutation query more likely to be precise point reads?

If there's no meaningful difference then I don't see how you can stop at the statement boundary when saying that reads should be more pessimistic. I think you'd need to go all the way to saying that all reads in a read-write transaction should be pessimistic. We can have that discussion, but it's a big one with many implications across the system.

[michae2]

enable_implicit_select_for_update defaults to true, so I don't think it's an indication that they're choosing pessimism over optimism. I think (and hope!) most users aren't even aware that there's a choice to be made — things should just work.

I'm curious whether you think there's a meaningful difference between the reads (e.g. FK checks) performed in a mutation query and the reads performed by other queries in a read-write transaction.

The only difference I was thinking about was the lack of a way to choose pessimism for these FK checks if they are involved in contention. For reads performed by other select queries in the transaction, there is FOR UPDATE or FOR SHARE. For reads performed by updates, there is enable_implicit_select_for_update. But for these FK checks there is no way to choose pessimism if it is beneficial.

Maybe piggybacking on enable_implicit_select_for_update is the wrong way to go about this. Maybe a separate variable would be better?

[rytaft]

Is this a priority for 22.2?

[michae2]

Is this a priority for 22.2?

I think it could be dropped from 22.2.