sql: deadlock due to planning query for SHOW TABLE

[ajwerner]

When planning a query we use a different transaction. We do this, at least in part, to ensure that reads of the schema performed during planning are cacheable. See here:

cockroach/pkg/sql/conn_executor_prepare.go

Lines 163 to 172 in e5bb268

	// Preparing needs a transaction because it needs to retrieve db/table
	// descriptors for type checking.
	// TODO(andrei): Needing a transaction for preparing seems non-sensical, as
	// the prepared statement outlives the txn. I hope that it's not used for
	// anything other than getting a timestamp.
	txn := kv.NewTxn(ctx, ex.server.cfg.DB, ex.server.cfg.NodeID.Get())
	ex.statsCollector.reset(&ex.server.sqlStats, ex.appStats, &ex.phaseTimes)
	p := &ex.planner
	ex.resetPlanner(ctx, p, txn, ex.server.cfg.Clock.PhysicalTime() /* stmtTS */)

Unfortunately, we do use this transaction for more than just a timestamp. This transaction makes its way to name resolution. An example of a problematic case is here:

cockroach/pkg/sql/delegate/show_table.go

Line 172 in e5bb268

dataSource, resName, err := d.catalog.ResolveDataSource(d.ctx, flags, &tn)

Imagine that we have Txn1 and Txn2:

Txn1: BEGIN;
Txn1: ALTER TABLE foo ...
Txn2: BEGIN;
Txn2: ALTER TABLE bar ...
Txn2: ALTER TABLE foo ... // blocks on Txn1
Txn1: SHOW COLUMNS FROM bar // blocks on Txn2 in planner txn

Had the planner been using the user's transaction then deadlock detection would have kicked in and we would have been fine. The problem here is that deadlock detection has no way to associate the planner txn with Txn1.

The fix for the short term is likely to be to follow the pattern of #46384 and #46170 and make the transaction used here high priority. Unlike in those cases however, the result of this query is not necesarily going to be cached as a shared resource. In fact, if we do this, then there's a chance that we can introduce a live-lock scenario where concurrent SHOW COLUMN ... can hold off a schema change. I'm not certain that that's the worst thing. I think for now it's the right thing to do.

A better solution might be to explicitly relate the planner txn to Tx1 for deadlock detection. Perhaps something like a read-only child transaction which carries its parent in push requests. @nvanbenschoten I think you've been having thoughts in this area.

---

This is fallout from #46402.

[ajwerner]

cc @andreimatei

[jordanlewis]

Should we try to get this done for 20.1, or is it too complex?

[ajwerner]

The easy fix is, well, 1-line sort of easy and it does seem to work. I’ll post the PR.

I would love some thoughts from Nathan. My best guess is that, yes, we should fix it with the high priority fix and then revisit for 20.2.

[andreimatei]

I would not make that preparing txn high priority. That seems very weird and fragile to me. I'd look into using the session's txn instead (if we are in an explicit txn).

But I'm also curious to hear Nathan's thoughts on how easy/opportune it'd be to express dependencies between txns beyond the current dependencies that we track - one txn trying to push another one.

[ajwerner]

I'd look into using the session's txn instead (if we are in an explicit txn).

I'll investigate what it would take to ensure that the results aren't cached. My current understanding is that that's the main problem. I wonder to what extent it'd be okay to cache the results because we should be hitting the in-memory table collection when retrieving modified resources.

[andreimatei]

Well my assumption is that there's already no caching of anything for the troublesome schema lookups happening when planning the show columns. Is there?

[ajwerner]

Well my assumption is that there's already no caching of anything for the troublesome schema lookups happening when planning the show columns. Is there?

No, but unless we do something invasive down in that code, we'd need to make the decision about which txn to use at a much higher level. I was hoping to not involve anything in delegate with which txn we're using. Ideally we'd just look at whether this is an explicit transaction inside of connExecutor.prepare()

[andreimatei]

Right, absolutely. It's in connExecutor.prepare() that I'd change things.

What I was saying about the caching is that, hopefully, the d.catalog.ResolveDataSource() call never caches anything, or at least not for use across transactions.

[ajwerner]

d.catalog.ResolveDataSource() call never caches anything, or at least not for use across transactions.

Yeah that seems right. I went through and looked at the database cache and table descriptors and seems like the only caching that happens underneath planning is scoped to the planner's TableCollection. Let me try that out and see if anything breaks.