sql: hazardous use of descriptors in BIND outside of explicit transaction

[ajwerner]

Describe the problem

In SQL you can bind arguments to a prepared statement outside of a transaction. This generally happens in the binary wire protocol if I understand correctly. The client will prepare a statement and then it will bind the arguments to an unnamed portal and then it will execute the portal. The prepare is safe and fine because we'll re-acquire leases inside the implicit transaction when checking if the plan is stale. The bind, however, has a hazardous and unsafe properties.

When the type of argument to a bind is either a user-defined type or refers to an OID type by name, we resolve the type and value during while doing the BIND. This is problematic because it will use the transaction currently sitting on the connExecutor. This is generally the previously committed transaction (I suspect there may be cases where this is nil but I don't know). This happens here (with the regclass/regtype resolution happening underneath tree.ParseDOid):

cockroach/pkg/sql/conn_executor_prepare.go

Lines 368 to 381 in d91205f

	typ, ok := types.OidToType[t]
	if !ok {
	var err error
	typ, err = ex.planner.ResolveTypeByOID(ctx, t)
	if err != nil {
	return nil, err
	}
	}
	d, err := pgwirebase.DecodeDatum(
	ex.planner.EvalContext(),
	typ,
	qArgFormatCodes[i],
	arg,
	)

At that point we'll attach a deadline due to leasing to an already committed transaction. If we attach the following sane invariant to the deadline code we get failures right and left:

--- a/pkg/kv/txn.go
+++ b/pkg/kv/txn.go
@@ -695,6 +695,9 @@ func (txn *Txn) UpdateDeadlineMaybe(ctx context.Context, deadline hlc.Timestamp)
 
        txn.mu.Lock()
        defer txn.mu.Unlock()
+       if txn.status().IsFinalized() {
+               panic(errors.WithContextTags(errors.AssertionFailedf("UpdateDeadlineMaybe() called on finalized txn"), ctx))
+       }

Worse yet are cases where we cannot use a cached, leased descriptor, like a number of system tables. For example, the following test fails rather surprisingly with this error:

func TestRegclassBind(t *testing.T) {
	defer leaktest.AfterTest(t)()
	defer log.Scope(t).Close(t)

	ctx := context.Background()
	tc := testcluster.StartTestCluster(t, 1, base.TestClusterArgs{})
	defer tc.Stopper().Stop(ctx)
	_, err := tc.ServerConn(0).Exec("SELECT $1::REGCLASS::INT", "system.descriptor")
	require.NoError(t, err)
}

    system_table_test.go:48: 
        	Error Trace:	system_table_test.go:48
        	Error:      	Received unexpected error:
        	            	pq: error in argument for $1: TransactionStatusError: client already committed or rolled back the transaction. Trying to execute: 1 Get (REASON_UNKNOWN)
        	Test:       	TestRegclassBind

Possible Solution

It seems to me like we should move the conn executor into an open state when binding (or when creating a portal?). It feels weird to me to have a bound portal outside of a transaction. I may not know enough to know though. We do this already for example when processing an ExecCmd.

Additional context

There are some edge cases where I think we may miss renames or interpret arguments as the wrong type. It's more just a general soundness problem.

Jira issue: CRDB-6916

[rytaft]

@ajwerner I'm assigning you since it looks like you have a PR in progress -- feel free to let us know if you'd like someone else to take a look

[ajwerner]

@rytaft I took a stab on Friday. I don't think that approach has legs. It's a bit of a pickle I don't see an easy way out of. Fortunately it seems to mostly cause problems in edge cases involving regclass or user-defined types concurrent with schema changes. I'll place it back in a triage state if my approach doesn't pan out.

[fqazi]

There are scenarios where the transaction can be nil, which is also problematic @ajwerner. I'm wondering if we should even hold onto leases in that case? It can lead to problems with the read timestamp related failures inside UpdateDeadline

[rafiss]

for the case of resolving a name into an OID/regclass, perhaps we could special-case the placeholder typing logic as a follow-up to this PR #60949

to guide our solution for that and the other issues, i will throw in a plug for our datadriven pgwire tests. you can write the exact sequence of pgwire commands you want and run them against either cockroachdb or postgres.

e.g. look at pkg/sql/pgwire/testdata/pgtest/portals and try running make test PKG=./pkg/sql/pgwire TESTS=TestPGTest TESTFLAGS="-addr=localhost:5432 -user=rafiss" to run the test against postgres

[ajwerner]

[written in parallel with the above comment]

Discussed offline with @rafiss. In the short term, the main thing we want to fix is the panic when you PREPARE/BIND something referencing UDTs or regclasses as the first thing you do on a connection. We can hack that by literally just putting a committed txn in place if txn is nil.

The next step is likely to revert part of #60949 to not resolve regclass or regtype values to their numeric value. If we did that, then I think we'd fix nearly all of the correctness issues. The remaining correctness issue would be to not convert the enum values from their logical to their physical representation until execution. I think that'd solve ~all of the correctness problems.

It still leaves us in a weird place where we can hit odd type mismatch errors when swapping type names (I don't care all that much about that case). The more glaring issue it leaves us with is a bizarre use of the descs.Collection while there is no open transaction. As we move forward, the descs.Collection is gaining a tighter coupling to the transaction. We'll need to negotiate that somehow.

I'll pick up the panic fix and leave the rest for follow-up.

[rafiss]

i was curious myself about what postgres did for regclass placeholders so i tried it out. it turns out that postgres will handle SELECT $1::REGCLASS::INT by typing $1 as a regclass (oid=2205). this was the test i used

send
Query {"String": "CREATE TABLE t (a INT PRIMARY KEY)"}
----

until
ReadyForQuery
----
{"Type":"CommandComplete","CommandTag":"CREATE TABLE"}
{"Type":"ReadyForQuery","TxStatus":"I"}


# 83 = ASCII 'S' for Statement
# 84 = ASCII 'T'
# ParameterFormatCodes = [0] for text format
send
Parse {"Name": "s7", "Query": "SELECT $1::REGCLASS::INT8"}
Describe {"ObjectType": 83, "Name": "s7"}
Bind {"DestinationPortal": "p7", "PreparedStatement": "s7", "ParameterFormatCodes": [0], "Parameters": [[84]]}
Execute {"Portal": "p7"}
Sync
----

until
ReadyForQuery
----
{"Type":"ParseComplete"}
{"Type":"ParameterDescription","ParameterOIDs":[2205]}
{"Type":"RowDescription","Fields":[{"Name":"int8","TableOID":0,"TableAttributeNumber":0,"DataTypeOID":23,"DataTypeSize":8,"TypeModifier":-1,"Format":0}]}
{"Type":"BindComplete"}
{"Type":"DataRow","Values":[{"text":"26494"}]}
{"Type":"CommandComplete","CommandTag":"SELECT 1"}
{"Type":"ReadyForQuery","TxStatus":"I"}

so i think reverting that part of #60949 could just bring back the incompatibility that caused us to write it in the first place -- a client might get confused that the placeholder is a different type than it expects.

the other thing i noticed is that we should try to do better error handling for cases where the regclass resolution fails. e.g. if we try to bind a non-existent table name for the placeholder, cockroachdb returns error in argument for $1: TransactionStatusError: client already committed or rolled back the transaction. Trying to execute: 1 Get (REASON_UNKNOWN) but postgres returns relation "blah" does not exist

anyway, for the fixes for regclass placeholders, feel free to hand off to sql-experience.

[ajwerner]

the other thing i noticed is that we should try to do better error handling for cases where the regclass resolution fails. e.g. if we try to bind a non-existent table name for the placeholder, cockroachdb returns error in argument for $1: TransactionStatusError: client already committed or rolled back the transaction. Trying to execute: 1 Get (REASON_UNKNOWN) but postgres returns relation "blah" does not exist

Heh this is exactly the issue! If the descriptor doesn't exist for leasing we attempt to read it from the store using the bogus finalized transaction :). If we fixed the root cause here, we'd return the right error.

[ajwerner]

so i think reverting that part of #60949 could just bring back the incompatibility that caused us to write it in the first place -- a client might get confused that the placeholder is a different type than it expects.

I think what I'd want is to change is that where we call DecodeDatum today we always return a a datum which means that for regclass, we have no choice but to return the integer oid. What I'd prefer is if we could instead return a TypedExpr which is the cast of the text to the right type. Ultimately we need a TypedExpr and not a Datum. That way we could defer the cast until execution.

The above approach doesn't solve the type problems.

[rafiss]

That makes a lot of sense in retrospect!

[rytaft]

Moving this over to SQL Experience since it seems like the remaining issues will fall to that team. Send it back if you need any help from SQL Queries - thanks!

[ajwerner]

@jameswsj10 your work to avoid the fallback reading from store by using export requests to find historical versions should be able to address the weirdness in #64140 (comment). It's just another band-aid over a bigger problem, but it's a good one.

[rafiss]

I think #76792 completely resolves this, but please reopen if there are still concerns.