sql: make sure pgwire bind always happens in a transaction

[rafiss]

fixes #70378
and maybe #64140

The approach of using a transaction matches what we do for pgwire Parse
messages already. This is important to make sure that user-defined types
are leased correctly.

This also updated the SQL EXECUTE command to resolve user-defined types
so that it gets the latest changes.

Release note (bug fix): Adding new values to a user-defined enum type
will previously would cause a prepared statement using that type to not
work. This now works as expected.

[cockroach-teamcity]

This change is 

[ajwerner]

Reviewed 1 of 6 files at r1.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner and @rafiss)

---

pkg/sql/execute.go, line 48 at r1 (raw file):

		// For user-defined types, we need to resolve the type to make sure we get
		// the latest changes to the type.

How does this help us in the case where the type has changed between bind and now?

[rafiss]

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

---

pkg/sql/execute.go, line 48 at r1 (raw file):

Previously, ajwerner wrote…

		// For user-defined types, we need to resolve the type to make sure we get
		// the latest changes to the type.

How does this help us in the case where the type has changed between bind and now?

the code here is for the EXECUTE sql command. binding the placeholders and executing the query is always going to be in the same implicit transaction. in other words:

• PREPARE: creates the prepared statement
• EXECUTE: binds parameters and runs the query in the same implicit txn

but if you're asking about the type changing between the pgwire Bind and the pgwire Exec (which is the code changed in conn_executor_prepare.go), this commit still helps. previously, the Bind would use whichever previous txn happened to be on the conn_executor, and now it gets its own txn. i don't see why Bind and Exec should share the same transaction. first of all, if anyone does change the type in between Bind and Exec, that seems like more of an edge case than what's being fixed here. but even so, it sounds correct to me that the type resolution during Bind should use the version of the type as it was at the time of the Bind command

but maybe i misunderstand you. if so, could you type out the sequence of pgwire commands and type changes that you're asking about?

[ajwerner]

i don't see why Bind and Exec should share the same transaction. first of all, if anyone does change the type in between Bind and Exec, that seems like more of an edge case than what's being fixed here.

I agree fully that they should not be using the same transaction. This change is good. I think the hazard is that if there's a change in the type (or, more likely, in the regclass) between the Bind and Exec then we'll use the old value.

Consider the below contrived example:

SELECT * FROM system.namespace WHERE id = $1::regclass;

Now, consider the argument is a table name and that between bind and exec we rename the table. Nothing will tell us we got it wrong. I think we can paper over that, but it's interesting. For enum values, we're a bit better off because it's highly likely that the execution plan tracked the version of the enum it used and when we check memo.IsStale we'll notice it is and re-plan. Anyway, I think that's all #64140 and isn't intended to be fixed by this.

My question was to check my understanding. Thanks

[rafiss]

Ah thanks for explaining.

I think the hazard is that if there's a change in the type (or, more likely, in the regclass) between the Bind and Exec then we'll use the old value.

I guess I'm not really convinced that it's incorrect to use the old value.

But also, turns out the PostgreSQL spec doesn't allow anything to happen in between Bind and Exec.

From https://www.postgresql.org/docs/current/protocol-flow.html#PROTOCOL-FLOW-EXT-QUERY

If successfully created, a named portal object lasts till the end of the current transaction

In other words, after Sync, the portal that was bound is destroyed. Indeed, if you try this test against Postgres, it results in portal "p8" does not exist

send
Parse {"Name": "s8", "Query": "SELECT oid, relname FROM pg_class WHERE oid = $1::regclass;"}
Bind {"DestinationPortal": "p8", "PreparedStatement": "s8", "ResultFormatCodes": [0], "Parameters": [{"text":"t"}]}
Sync
----

until
ReadyForQuery
----
{"Type":"ParseComplete"}
{"Type":"BindComplete"}
{"Type":"ReadyForQuery","TxStatus":"I"}

send
Execute {"Portal": "p8"}
Sync
----

until
ReadyForQuery
----
{"Type":"DataRow","Values":[{"text":"52"},{"text":"t"}]}
{"Type":"CommandComplete","CommandTag":"SELECT 1"}
{"Type":"ReadyForQuery","TxStatus":"I"}

[rafiss]

Filed #71665

[rafiss]

Ah, but we actually can get a test like the one that you're asking for by using an explicit transaction. Added one to bind_and_resolve and confirmed that we are matching the PostgreSQL behavior.

hm i guess it's actually still different from #64140 since that issue is only talking about non-explicit txns. well, still a good test to have i think.

[ajwerner]

Ah, but we actually can get a test like the one that you're asking for by using an explicit transaction. Added one to bind_and_resolve and confirmed that we are matching the PostgreSQL behavior.

In an explicit transaction, won't all three steps will be using the same transaction. We'll bypass

cockroach/pkg/sql/conn_executor_prepare.go

Lines 420 to 427 in 7669013

	// This is a huge kludge to deal with the fact that we're resolving types
	// using a planner with a committed transaction. This ends up being almost
	// okay because the execution is going to re-acquire leases on these types.
	// Regardless, holding this lease is worse than not holding it. Users might
	// expect to get type mismatch errors if a rename of the type occurred.
	if ex.getTransactionState() == NoTxnStateStr {
	ex.planner.Descriptors().ReleaseAll(ctx)
	}

and the case I think we're currently talking about.

I guess I'm not really convinced that it's incorrect to use the old value.

I sort of hear that. The reason I'd say it is is that we say that once return from a rename, we claim that no transaction which happens after (has a higher commit timestamp) will observe the old name. It's possible in this edge case. I think it'd be a problem in postgres if you ran under serializable.

But also, turns out the PostgreSQL spec doesn't allow anything to happen in between Bind and Exec.

My claim is regarding something happening of a different connection between bind and exec. The rename would occur on a separate connection, concurrently, between the bind and exec.

[otan]

maybe if typ.UserDefined() {?

[rafiss]

done

[otan]

nit: full stop at end of comment

[rafiss]

done

[otan]

can you comment on why this is crdb_only?

[rafiss]

done

[rafiss]

flaky CI

tftr!

bors r=otan

[craig]

Build succeeded:

• GitHub CI (Cockroach)

[nvanbenschoten]

@rafiss I'm not familiar with this change, but I noticed in passing while looking at a few benchmarks that it made connExecutor.execBind more expensive. In a read-only, CPU-bound workload, execBind is now responsible for 4.2% of CPU utilization. Most of that can be attributed to the cost of creating a new short-lived transaction, which is a relatively heavy-weight operation compared to everything else in execBind.

I know this was a bug fix and I unfortunately don't have any good suggestions (beyond #74350) to make this cheaper, but I figured I'd mention it here in case there were alternative fixes that didn't require a separate txn.

[rafiss]

One optimization potential here is that we only need to create/use a transaction if we end up calling ResolveTypeByOID -- that is, if bindCmd.Args is non-empty and contains user-defined types.

[rafiss]

@nvanbenschoten I made that optimization here: #74423