sql: bulk insert/update in implicit txn can retry indefinitely

[fqazi]

Fixes: #69089

Previously, the transaction deadline was only refreshed
when we bubbled back up to the transaction state machinery
inside the SQL layer. This was inadequate for implicit
transactions, since we will not bubble back up and refresh
the deadline and leading to a retry error. If the implicit transaction
takes longer than the lease time, then we will be indefinitely
retrying the transaction. To address this, this patch
will add logic to bubble back up to the SQL layer to
refresh the deadline before trying to commit.

Release justification: low risk and addresses a severe issue with bulk
operations
Release note (bug fix): Bulk insert/update in implicit txn can
retry indefinitely if the statement exceeds the default leasing
deadline of 5 minutes.

[cockroach-teamcity]

This change is 

[ajwerner]

Cool!

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

---

pkg/kv/txn.go, line 760 at r1 (raw file):

	txn.mu.Lock()
	defer txn.mu.Unlock()
	isExpired := !txn.mu.deadline.IsEmpty() &&

nit: just return it instead of assigning to a variable.

---

pkg/kv/txn.go, line 764 at r1 (raw file):

		// it off the sender.
		(txn.mu.deadline.Less(txn.mu.sender.ProvisionalCommitTimestamp()) ||
			txn.mu.deadline.Less(txn.DB().Clock().Now()))

how about: txn.mu.deadline.GoTime().After(txn.DB().Clock().PhysicalTime())) to avoid hitting the hlc. We don't care about the logical bits.

---

pkg/sql/catalog/lease/lease_test.go, line 2854 at r1 (raw file):

	ctx := context.Background()

	beforeAutoCommit := syncutil.Mutex{}

nit: put the mutex near above the variables it protects?

---

pkg/sql/catalog/lease/lease_test.go, line 2868 at r1 (raw file):

	// require the lease to be reacquired.
	lease.LeaseDuration.Override(ctx, &params.SV, 0)
	params.Knobs.SQLExecutor = &sql.ExecutorTestingKnobs{

this is pretty subtle. The subtest works because we inject this hook which only gets called from the conn executor performing the commit as opposed to the commit being performed during execution. Can you add commentary so a reader might be able to understand?

---

pkg/sql/catalog/lease/lease_test.go, line 2875 at r1 (raw file):

				blockAutoCommitResume <- struct{}{}
			}
			beforeAutoCommit.Unlock()

nit: defer?

---

pkg/sql/catalog/lease/lease_test.go, line 3052 at r1 (raw file):

		_, err = conn.ExecContext(ctx, `
INSERT INTO t1 select a from generate_series(1, 100) g(a);
SET SQL_SAFE_UPDATES=no

why the SQL_SAFE_UPDATES?

---

pkg/sql/catalog/lease/lease_test.go, line 3059 at r1 (raw file):

		go func() {
			beforeAutoCommit.Lock()
			blockAutoCommitStmt = "UPDATE t1 SET val = 2"

nit: define this string as a const and then us it here and in the ExecContext call.

[fqazi]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner and @fqazi)

---

pkg/sql/catalog/lease/lease_test.go, line 2868 at r1 (raw file):

Previously, ajwerner wrote…

this is pretty subtle. The subtest works because we inject this hook which only gets called from the conn executor performing the commit as opposed to the commit being performed during execution. Can you add commentary so a reader might be able to understand?

Done.

[ajwerner]

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

---

pkg/sql/catalog/lease/lease_test.go, line 2870 at r3 (raw file):

	// Inject a hook to slow down the autocommit coming from
	// the connection executor side, which will allow us to
	// add potential delays that cause leases to expire.

This comment is making me question the intention of the hook. My reading was that we inject this hook only to make sure that the hook gets called and not to inject any sort of delay for anything to expire. The test does not exercise the case where a transaction issuing an UPDATE would actually fail, it just verifies that the transaction does not attempt to commit in the writing batch. The mechanism for that verfication is the fact that this got called.

If you wanted to ensure that the transaction actually would have failed, then you'd need to force its timestamp to get pushed.

[fqazi]

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

---

pkg/sql/catalog/lease/lease_test.go, line 3052 at r1 (raw file):

Previously, ajwerner wrote…

why the SQL_SAFE_UPDATES?

Done

---

pkg/sql/catalog/lease/lease_test.go, line 3059 at r1 (raw file):

Previously, ajwerner wrote…

nit: define this string as a const and then us it here and in the ExecContext call.

Done

---

pkg/sql/catalog/lease/lease_test.go, line 2870 at r3 (raw file):

Previously, ajwerner wrote…

This comment is making me question the intention of the hook. My reading was that we inject this hook only to make sure that the hook gets called and not to inject any sort of delay for anything to expire. The test does not exercise the case where a transaction issuing an UPDATE would actually fail, it just verifies that the transaction does not attempt to commit in the writing batch. The mechanism for that verfication is the fact that this got called.

If you wanted to ensure that the transaction actually would have failed, then you'd need to force its timestamp to get pushed.

Ugh, yes I misunderstood why I saw a hang without the fix. The auto-commit hook is only called if a 1PC optimization doesn't occur. With this change we expect this optimization to be skipped in the expiry case and bubble to the SQL executor layer for the commit.

I fixed the comment.

[ajwerner]

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

---

pkg/sql/catalog/lease/lease_test.go, line 3051 at r4 (raw file):

	// above via the LeaseDuration override. The auto-commit hook allows
	// us to confirm the 1 phase commit optimization is bypassed due,
	// to the lease expiring.

I'm still unclear on how the initial deadline from leasing related to the transaction timestamp. Did you ever investigate that? I'd have expected that the expiration would be after the transaction timestamp because it'll be at the timestamp of the leasing transaction, which will be after the user transaction timestamp. Then, nothing should push, so I'm not clear on why we hit the retries in this test without the case to bubble back up. I think the deal is that we would not. In that case, the test is a little confusing and complex given the expiration isn't really relevant.

One option would be to block the update request, and then read the data and then unblock the update request which will push the timestamp.

[ajwerner]

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

---

pkg/sql/catalog/lease/lease_test.go, line 3115 at r5 (raw file):

		beforeExecuteResumeStmt = selectQuery
		beforeExecute.Unlock()
		pushLimit := 4

I think this ends up being sort of confusing because we should only ever get pushed once, right? Why even have a limit?

[ajwerner]

Reviewed 1 of 2 files at r2, 1 of 2 files at r6.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @fqazi)

---

pkg/sql/conn_executor_exec.go, line 1797 at r6 (raw file):

	}

	// Attempt to refresh the deadline before the autocommit

nit: period

---

pkg/sql/tablewriter.go, line 226 at r6 (raw file):

Quoted 4 lines of code…

		// We can only auto commit if the rows written guardrail is disabled or
		// we haven't reached the specified limit (the optimizer is responsible
		// for making sure that there is exactly one mutation before enabling
		// the auto commit).

delete this now duplicated comment.

---

pkg/sql/catalog/lease/lease_test.go, line 3029 at r6 (raw file):

}

func TestLeaseBulkInsert(t *testing.T) {

give this a comment explaining the problem that this test is making sure we fixed.

---

pkg/sql/catalog/lease/lease_test.go, line 3047 at r6 (raw file):

	lease.LeaseDuration.Override(ctx, &params.SV, 0)
	params.Knobs.SQLExecutor = &sql.ExecutorTestingKnobs{
		BeforeExecute: func(ctx context.Context, stmt string) {

comment this?

---

pkg/sql/catalog/lease/lease_test.go, line 3108 at r6 (raw file):

		// go through.
		for atomic.LoadUint32(&executingUpdate) == 1 {
			beforeExecuteWait <- struct{}{}

I feel like this synchronization is not what I expected. I'd have expected that the beforeExecute hook sends a channel and then this goroutine closes the channel after it does the read. This here seems racy.

[fqazi]

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

---

pkg/sql/catalog/lease/lease_test.go, line 3051 at r4 (raw file):

Previously, ajwerner wrote…

I'm still unclear on how the initial deadline from leasing related to the transaction timestamp. Did you ever investigate that? I'd have expected that the expiration would be after the transaction timestamp because it'll be at the timestamp of the leasing transaction, which will be after the user transaction timestamp. Then, nothing should push, so I'm not clear on why we hit the retries in this test without the case to bubble back up. I think the deal is that we would not. In that case, the test is a little confusing and complex given the expiration isn't really relevant.

One option would be to block the update request, and then read the data and then unblock the update request which will push the timestamp.

Reworked this push out the transaction a limited amount of time and confirm the autocommit side of things, where we should bubble back up to the top.

---

pkg/sql/catalog/lease/lease_test.go, line 3047 at r6 (raw file):

Previously, ajwerner wrote…

comment this?

Done.

[andreimatei]

can you please explain to me what layer the endless retries were happening at?

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner and @fqazi)

---

pkg/sql/catalog/lease/lease_test.go, line 3127 at r9 (raw file):

			`+selectQuery+";\n"+
				`END;`)
			/*	require.NoError(t, err)

!

[fqazi]

The retires are not necessarily endless, and from transaction, protocol refresh errors due to deadline expirey. Bulk operations can be extremely long, so it will look endless, but not really endless it should eventually timeout. But, that could be a really long time.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner and @fqazi)

[fqazi]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner, @andreimatei, and @fqazi)

---

pkg/kv/txn.go, line 785 at r12 (raw file):

Previously, fqazi (Faizan Qazi) wrote…

s duration conversion. I know we talked about clock offsets but there's a b
This was incorrect on my part, I thought we were storing a duration inside a hlc.Timestamp or something funny, but that's not the case. Let me revisit this.

Done.

---

pkg/sql/catalog/lease/lease_test.go, line 3127 at r9 (raw file):

Previously, andreimatei (Andrei Matei) wrote…

!

Done.

---

pkg/kv/sender.go, line 237 at r12 (raw file):

Previously, ajwerner wrote…

Something seems off about the need to expose this. Also, I was envisioning a duration. This deserves commentary if we need to expose it.

Done.

[ajwerner]

Thanks for the stamina working through this!

Reviewed 1 of 8 files at r12.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner, @andreimatei, and @fqazi)

---

pkg/kv/txn.go, line 766 at r13 (raw file):

// DeadlineMightBeExpired returns true if there currently is a deadline and
// that deadline is earlier than either the ProvisionalCommitTimestamp or
// the current timestamp.  If the expiration is past the current timestamp,

say or the current reading of the node's hlc clock. The second condition is a conservative optimization to deal with the fact that the provisional commit timestamp may not represent the true commit timestamp; the transaction may have been pushed but not yet discovered that fact. Deadlines, in general, should not commonly be at risk of expiring near the current time, except in extraordinary circumstances. In cases where considering it helps, it helps a lot. In cases where considering it does not help, it does not hurt much.

---

pkg/sql/catalog/lease/lease_test.go, line 3127 at r9 (raw file):

Previously, fqazi (Faizan Qazi) wrote…

Done.

nit: I'd just inline the statement in a const

const (
    selectStmt = `SELECT * FROM t1`
   selectTxn = `BEGIN PRIORITY HIGH; `+selectStmt+`; COMMIT;`
)

---

pkg/sql/catalog/lease/lease_test.go, line 3057 at r13 (raw file):

			beforeExecute.Lock()
			if stmt == beforeExecuteStmt {
				log.Errorf(ctx, "TEST: WAITING FOR SELECT")

nit: remove

---

pkg/sql/catalog/lease/lease_test.go, line 3084 at r13 (raw file):

			beforeExecute.Lock()
			if stmt == beforeExecuteResumeStmt {
				log.Errorf(ctx, "TEST: UNPAUSING FOR UPDATE")

nit: remove

---

pkg/sql/catalog/lease/lease_test.go, line 3102 at r13 (raw file):

	_, err := conn.Exec(`
CREATE TABLE t1(val int);
	`)

it might be interesting and worthwhile to split this table so we're doing a cross-range transaction. In practice, the cases we care about involve cross range transactions. It'd be a bummer if a single-range optimization hid some issue.

How about the following in setup here?

ALTER TABLE t1 SPLIT AT VALUES (1)

---

pkg/sql/catalog/lease/lease_test.go, line 3119 at r13 (raw file):

	// transaction readline for this.
	t.Run("validate-lease-txn-deadline-ext-update", func(t *testing.T) {
		runSelects := uint32(1)

thoughts on:

1. making this an atomic.Value with a bool?
2. calling it updateCompleted (and thus initializing it to false)?

---

pkg/sql/catalog/lease/lease_test.go, line 3137 at r13 (raw file):

			// Execute a bulk UPDATE, which will get its
			// timestamp pushed by a read operation.
			_, err := conn.ExecContext(ctx, bulkUpdateQuery)

I'd expect this to use updateConn and the select to use conn (or call it selectConn)?

[fqazi]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner, @andreimatei, and @fqazi)

---

pkg/sql/catalog/lease/lease_test.go, line 3127 at r9 (raw file):

Previously, ajwerner wrote…

nit: I'd just inline the statement in a const

const (
    selectStmt = `SELECT * FROM t1`
   selectTxn = `BEGIN PRIORITY HIGH; `+selectStmt+`; COMMIT;`
)

Done.

---

pkg/sql/catalog/lease/lease_test.go, line 3119 at r13 (raw file):

Previously, ajwerner wrote…

thoughts on:

1. making this an atomic.Value with a bool?
2. calling it updateCompleted (and thus initializing it to false)?

Done.

[ajwerner]

Reviewed 2 of 8 files at r12, 1 of 2 files at r14, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @ajwerner, @andreimatei, and @fqazi)

---

pkg/sql/catalog/lease/lease_test.go, line 3035 at r14 (raw file):

	beforeExecute := syncutil.Mutex{}
	// Statement that will be paused

nit: period

---

pkg/sql/catalog/lease/lease_test.go, line 3039 at r14 (raw file):

	// Statement that will allow any paused
	// statement to resume.

nit: unnecessary wrapping

---

pkg/sql/opt/exec/execbuilder/testdata/autocommit_nonmetamorphic, line 807 at r14 (raw file):

----
dist sender send  r43: sending batch 1 CPut to (n1,s1):1
dist sender send  r43: sending batch 1 EndTxn to (n1,s1):1

I don't think we were trying to change this case. Why did it change?

[fqazi]

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @ajwerner, @andreimatei, and @fqazi)

---

pkg/sql/opt/exec/execbuilder/testdata/autocommit_nonmetamorphic, line 807 at r14 (raw file):

Previously, ajwerner wrote…

----
dist sender send  r43: sending batch 1 CPut to (n1,s1):1
dist sender send  r43: sending batch 1 EndTxn to (n1,s1):1

I don't think we were trying to change this case. Why did it change?

This was caused by me accidentally deleting an equal sign on the row limit enforcement.

[fqazi]

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @ajwerner, @andreimatei, and @fqazi)

---

pkg/kv/txn.go, line 755 at r10 (raw file):

Previously, andreimatei (Andrei Matei) wrote…

Please explain the "current timestamp" part. I think it's only relevant if there are going to be more writes in the txn, so consider taking a flag and letting the client say that interactions with the timestamp cache are impossible because the rest of the txn is read-only. I think that's an important case to consider, and it would also help the code clarify that the MVCC timestamp domain is different from the wall-clock domain - the descriptor leases work in the MVCC domain, so that's the main thing that a method like this cares about.
When the client declares that there's no more writes, you could use Transaction.global_uncertainty_limit instead of ProvisionalCommitTimestamp to account for the possibility of running into ReadWithinUncertaintyErrors that can push the timestamp up.

Done.

---

pkg/kv/txn.go, line 756 at r10 (raw file):

Previously, andreimatei (Andrei Matei) wrote…

I'd either say more or say less about the relation with auto-commit. Consider striking that part of the comment.

Done.

[ajwerner]

Reviewed 2 of 3 files at r15.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @ajwerner and @andreimatei)

[fqazi]

@ajwerner TFTR

[ajwerner]

Can we bors this?

[ajwerner]

@andreimatei want to review the commentary?

[andreimatei]

but see comments.

Reviewable status: complete! 2 of 0 LGTMs obtained (waiting on @ajwerner, @andreimatei, and @fqazi)

---

pkg/kv/txn.go, line 755 at r10 (raw file):

Previously, fqazi (Faizan Qazi) wrote…

Done.

You say "Done", but you haven't taken a position on whether we should make a distinction between transactions that have finished writing vs transactions that still want to write. Instead, you're making the remark that the transaction may have been pushed but not yet discovered that fact. That's interesting; I hadn't considered it. At first I wasn't gonna buy that that's a significant case, but I've read the rangefeed pusher code and I see that it does high-priority pushes, and it also pushes right up to the present time. So I guess I do buy it as likely enough. But still, at the very least, I'd make an additional note of the fact that, if the transaction is writing, it's likely enough to be pushed when it writes. I think that's at least as important as the case where we have already been pushed but we don't know it.

One thing I still struggle with is that this method makes too big of a deal out of the present time. As far as I can think, that's unprincipled and, even worse, confusing. First of all, clocks are desynchronized, so if we've been pushed, we might have been pushed above the present time. Second, the door is open to races with concurrent pushes. Third, if the transaction is writing to non-blocking ranges, then it's commit timestamps is going to be in the future of present time. The method already accounts for the txn's previous writes to non-blocking ranges because it considers the ProvisionalCommitTimestamp, but it doesn't account for future writes to non-blocking ranges. I think we should easily account for all these things by setting the threshold for the commit timestamp a bit in the future. Putting a fudge factor in there will also suggest to the reader that this is all best-effort. Consider using TargetForPolicy(...,LEAD_FOR_GLOBAL_READS) + 1s.

---

pkg/kv/txn.go, line 770 at r15 (raw file):

// the true commit timestamp; the transaction may have been pushed but not yet discovered that fact.
// Deadlines, in general, should not commonly be at risk of expiring near the current time, except in
// extraordinary circumstances. In cases where considering it helps, it helps a lot. In cases where

nit: this paragraph is not wrapped at 80 cols

---

pkg/kv/txn.go, line 774 at r15 (raw file):

func (txn *Txn) DeadlineMightBeExpired() bool {
	txn.mu.Lock()
	txn.mu.sender.ProvisionalCommitTimestamp()

was this call intended?

---

pkg/kv/txn.go, line 778 at r15 (raw file):

	return txn.mu.deadline != nil &&
		!txn.mu.deadline.IsEmpty() &&
		// Avoids getting the txn mutex again by getting

Who's "it"? I don't think this comment will make sense to any reader. I'd add a txn.provisionalCommitTimestampLocked and avoid any comment.

---

pkg/kv/txn.go, line 782 at r15 (raw file):

		(txn.mu.deadline.Less(txn.mu.sender.ProvisionalCommitTimestamp()) ||
			// In case the transaction gets pushed and the push is not observed,
			// we cautiously also indicate that a refresh is needed if the current

This is the first time this function mentions the word "refresh", which is inappropriate. If you want to equate "refresh is needed" with the return value of this function (or generally if you want to talk about what a caller might do with the retval), do so in the function-level commentary. Also, I think you're using the word "refresh" to mean something different than it means throughout the KV layer, which is very confusing. You're using it to mean updating the deadline based on new leases. "Refresh" in KV is widely used as short-form for "refreshing a transaction's read timestamp" through a RefreshRequest. So please don't use this word.

---

pkg/sql/tablewriter.go, line 222 at r15 (raw file):

		// If we bubble back up to SQL then maybe we can get a fresh deadline
		// before committing.
		!tb.txn.DeadlineMightBeExpired() {

Instead of pessimistically inhibiting the auto-commit, have you considered whether it's feasible to reach back in to the lease machinery and call MaybeUpdateDeadline, and then allow the auto-commit regardless of the outcome? If feasible, that'd be more directly encode what we really want.

[fqazi]

Reviewable status: complete! 2 of 0 LGTMs obtained (waiting on @ajwerner and @andreimatei)

---

pkg/kv/txn.go, line 755 at r10 (raw file):

You say "Done", but you haven't taken a position on whether we should make a distinction between transactions that have finished writing vs transactions that still want to write. Instead, you're making the remark that the transaction may have been pushed but not yet discovered that fact. That's interesting; I hadn't considered it. At first I wasn't gonna buy that that's a significant case, but I've read the rangefeed pusher code and I see that it does high-priority pushes, and it also pushes right up to the present time. So I guess I do buy it as likely enough. But still, at the very least, I'd make an additional note of the fact that, if the transaction is writing, it's likely enough to be pushed when it writes. I think that's at least as important as the case where we have already been pushed but we don't know it.
One thing I still struggle with is that this method makes too big of a deal out of the present time. As far as I can think, that's unprincipled and, even worse, confusing. First of all, clocks are desynchronized, so if we've been pushed, we might have been pushed above the present time. Second, the door is open to races with concurrent pushes. Third, if the transaction is writing to non-blocking ranges, then it's commit timestamps is going to be in the future of present time. The method already accounts for the txn's previous writes to non-blocking ranges because it considers the ProvisionalCommitTimestamp, but it doesn't account for future writes to non-blocking ranges. I think we should easily account for all these things by setting the threshold for the commit timestamp a bit in the future. Putting a fudge factor in there will also suggest to the reader that this is all best-effort. Consider using TargetForPolicy(...,LEAD_FOR_GLOBAL_READS) + 1s.

I added the phrasing: for the first part:
// Transactions that write from now on can still get pushed, versus
// transactions which are done writing where it will be less clear
// how those get pushed.

Second part I updated the code and described why we are doing a fudge factor.

---

pkg/kv/txn.go, line 774 at r15 (raw file):

Previously, andreimatei (Andrei Matei) wrote…

was this call intended?

Accidental, removed.

---

pkg/kv/txn.go, line 778 at r15 (raw file):

Previously, andreimatei (Andrei Matei) wrote…

Who's "it"? I don't think this comment will make sense to any reader. I'd add a txn.provisionalCommitTimestampLocked and avoid any comment.

We can't use the Locked one because we need the mutex for the deadline. So, I clarified the comment as:
// Avoid trying to get get the txn mutex again by directly
// invoking ProvisionalCommitTimestamp versus calling
// ProvisionalCommitTimestampLocked on the Txn.

---

pkg/kv/txn.go, line 782 at r15 (raw file):

  // Avoid trying to get get the txn mutex again by directly
  // invoking ProvisionalCommitTimestamp versus calling
  // ProvisionalCommitTimestampLocked on the Txn.

Good point, changed this comment to:
// In case the transaction gets pushed and the push is not observed,
// we cautiously also indicate that the deadline maybe expired if
// the current HLC clock exceeds the deadline.

---

pkg/sql/tablewriter.go, line 222 at r15 (raw file):

Instead of pessimistically inhibiting the auto-commit, have you considered whether it's feasible to reach back in to the lease machinery and call MaybeUpdateDeadline, and then allow the auto-commit regardless of the outcome? If feasible, that'd be more directly encode what we really want.

Are there scenarios where the violation of the existing boundaries is worth keeping this optimization? The state machinery that manages the leasing logic is encapsulated at a higher layer and I think ideally we don't want to deal with that logic in the tableWriter directly, since it would break encapsulation.

[fqazi]

bors r+

[craig]

Build succeeded:

• GitHub CI (Cockroach)

[rafiss]

@fqazi @ajwerner friendly ping - does this still need to be backported?