sql: only create new txn for Bind if necessary

[rafiss]

01ca1e5 changed Bind so that it always
uses a transaction to resolve the arguments. This caused a performance
hit, leading to this code using 4.2% of the CPU utilization in a
read-only workload. To optimize the code, a new transaction is only
created when necessary, which is when we need to resolve user-defined
types or OID types.

No release note since the optimization only applies to unreleased
versions.

Release note: None

[cockroach-teamcity]

This change is 

[ajwerner]

Do we also need one for evaluating the various oid types as cases. Like IIRC SELECT $1::regclass when you bind a string will do a catalog lookup. I could be wrong, but I recall this causing bugs in some very delicate edge cases where we fall back to avoiding the lease manager. Even when we don't it only subtly worked because the planner had not been reset and the committed previous transaction was used for the lease or something like that.

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

[rafiss]

Yeah, you're right. pgwirebase.DecodeDatum ends up calling planner.ResolveOIDFromOID which needs a transaction. I think the tests in pkg/sql/pgwire/testdata/pgtest/bind_and_resolve were supposed to cover this, but I guess they are getting lucky like you said.

[rafiss]

I added the check for OID types, but I still am not sure how to test it.

[ajwerner]

The reason this worked before is pretty insane! It worked before because when we end up resolving the OID, we use the InternalExecutor, which, when it gets a nil txn uses its own. ☹️

cockroach/pkg/sql/resolve_oid.go

Lines 77 to 78 in 203c7f4

	results, err := ie.QueryRowEx(ctx, "queryOid", txn,
	sessiondata.NoSessionDataOverride, q, toResolve)

Pretty wild. Having the internal executor optionally taking a transaction is a little crazy.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @nvanbenschoten and @rafiss)

---

pkg/sql/conn_executor_prepare.go, line 437 at r3 (raw file):

			// Use the existing transaction.
			if err := resolve(ctx, txn); err != nil {
				return retErr(err)

Wait, if we have a txn, doesn't that mean the planner is already in a good state? Do we need to reset it and all that jazz?

Code quote:

			// Use the existing transaction.
			if err := resolve(ctx, txn); err != nil {
				return retErr(err)
			}

[rafiss]

nice find on the internalExecutor! hm...

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

---

pkg/sql/conn_executor_prepare.go, line 437 at r3 (raw file):

Previously, ajwerner wrote…

Wait, if we have a txn, doesn't that mean the planner is already in a good state? Do we need to reset it and all that jazz?

good point .. i'll try moving the resetPlanner stuff into the branch where a new txn is created

[rafiss]

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

---

pkg/sql/conn_executor_prepare.go, line 437 at r3 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

good point .. i'll try moving the resetPlanner stuff into the branch where a new txn is created

actually, i think it's fine to call resetPlanner each time. it's already called any time any pgwire command is exec'd. (e,g, we call resetPlanner in execStmtInOpenState even though there already is a txn.)

[rafiss]

This is ready for review. Given that this is just an optimization, and no existing tests regressed, I think I feel pretty safe about merging this.

[ajwerner]

Reviewed all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @nvanbenschoten and @otan)

[rafiss]

tftr!

bors r=ajwerner

[craig]

Build failed (retrying...):

• GitHub CI (Cockroach)

[craig]

Build succeeded:

• GitHub CI (Cockroach)