sql: remove the artifact of canceling the txn-scoped context

[yuzefovich]

This commit removes an old artifact of having a txn-scoped context
cancellation that is performed when finishing the txn. As Andrei points
out, this txn-scoped cancellation is likely a leftover from ancient
times and is no longer needed. In particular, this also fixes the bug of
using the span after it was finished (which would occur with high
vmodule on context.go file).

Fixes: #83739.

Release note: None

[cockroach-teamcity]

This change is 

[mgartner]

Nice find!

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

[andreimatei]

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

---

pkg/sql/exec_util.go line 2294 at r1 (raw file):

		// context cancellation). To go around that, we pass the responsibility
		// of finishing the span to the cancellation function.
		cancelTxnCtx := st.ex.state.cancel

I think finishing sp here was better. I think a better fix would be to call st.ex.state.cancel here, and then overwrite it.
But, actually, I don't understand why ex.state.cancel exists - why do we have a transaction-level cancelation? There's logically no such thing as canceling a transaction, is there? I think the only use of state.cancel is CANCEL QUERY, which could do with a statement-level cancel func. A statement-level func I think could be scoped to execStmtInOpenState. Then we wouldn't need this more troublesome and unclearly scoped transaction-level cancelation.

The transaction-level cancelation might have been a left-over from an ancient time when
a) we had parallel statements (returning nothing)
b) the TxnCoordSender's heartbeat loop was using a transaction-scoped SQL ctx.
Neither of these are true any more.

WDYT?

[yuzefovich]

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

---

pkg/sql/exec_util.go line 2294 at r1 (raw file):

Previously, andreimatei (Andrei Matei) wrote…

I think finishing sp here was better. I think a better fix would be to call st.ex.state.cancel here, and then overwrite it.
But, actually, I don't understand why ex.state.cancel exists - why do we have a transaction-level cancelation? There's logically no such thing as canceling a transaction, is there? I think the only use of state.cancel is CANCEL QUERY, which could do with a statement-level cancel func. A statement-level func I think could be scoped to execStmtInOpenState. Then we wouldn't need this more troublesome and unclearly scoped transaction-level cancelation.

The transaction-level cancelation might have been a left-over from an ancient time when
a) we had parallel statements (returning nothing)
b) the TxnCoordSender's heartbeat loop was using a transaction-scoped SQL ctx.
Neither of these are true any more.

WDYT?

I think you are right on all points, so I implemented your suggestion. Let's see what the CI says, but I'm optimistic that nothing will break 🤞

[yuzefovich]

Looks like CI is happy, and I like this approach more. PTAL.

[andreimatei]

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

[yuzefovich]

TFTRs!

bors r+

[craig]

Build failed (retrying...):

• GitHub CI (Cockroach)

[craig]

Build succeeded:

• GitHub CI (Cockroach)

[blathers-crl]

Encountered an error creating backports. Some common things that can go wrong:

1. The backport branch might have already existed.
2. There was a merge conflict.
3. The backport branch contained merge commits.

You might need to create your backport manually using the backport tool.

---

error creating merge commit from cf69a32 to blathers/backport-release-22.1-84628: POST https://api.github.com/repos/cockroachdb/cockroach/merges: 409 Merge conflict []

you may need to manually resolve merge conflicts with the backport tool.

Backport to branch 22.1.x failed. See errors above.

---

_{🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is otan.}