sql: only create new txn for Bind if necessary#74423
sql: only create new txn for Bind if necessary#74423craig[bot] merged 1 commit intocockroachdb:masterfrom
Conversation
|
This change is |
ajwerner
left a comment
There was a problem hiding this comment.
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)
|
Yeah, you're right. |
022426f to
7792cdb
Compare
|
I added the check for OID types, but I still am not sure how to test it. |
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
7792cdb to
8f77b90
Compare
ajwerner
left a comment
There was a problem hiding this comment.
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
Pretty wild. Having the internal executor optionally taking a transaction is a little crazy.
Reviewable status:
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
left a comment
There was a problem hiding this comment.
nice find on the internalExecutor! hm...
Reviewable status:
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
left a comment
There was a problem hiding this comment.
Reviewable status:
pkg/sql/conn_executor_prepare.go, line 437 at r3 (raw file):
Previously, rafiss (Rafi Shamim) wrote…
good point .. i'll try moving the
resetPlannerstuff 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.)
|
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
left a comment
There was a problem hiding this comment.
Reviewed all commit messages.
Reviewable status:
|
tftr! bors r=ajwerner |
|
Build failed (retrying...): |
|
Build succeeded: |
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