sql: correctly reset the planner in the copy shim #89112
Conversation
Previously, we forgot to update the txn of the eval context of the planner. Release note: None
|
This change is |
There was a problem hiding this comment.
Reviewable status:
complete! 0 of 0 LGTMs obtained (waiting on @cucaroach and @yuzefovich)
pkg/sql/copyshim.go line 101 at r1 (raw file):
p.optPlanningCtx.init(p) p.resetPlanner(ctx, txn, stmtTS, p.sessionDataMutatorIterator.sds.Top()) p.extendedEvalCtx.Context.Txn = txn
Should we be doing this in planner.resetPlanner? It kind of feels like a footgun that we have to remember to do this.
There was a problem hiding this comment.
Reviewable status:
pkg/sql/copyshim.go line 101 at r1 (raw file):
Previously, DrewKimball (Drew Kimball) wrote…
Should we be doing this in
planner.resetPlanner? It kind of feels like a footgun that we have to remember to do this.
This was my original intention too, but when I tried it, things failed in other spots (namely in newInternalPlanner the panic with makeInternalPlanner called with a transaction without timestamps happened), so I opted for this solution. On the main code path the txn is being reset connExecutor.resetEvalCtx after planner.resetPlanner (in connExecutor.resetPlanner), and this commit kinda does things in a similar fashion. Additionally, this copy shim is a test-only utility, so it seems safer to limit the change only to this testing kind since planner.resetPlanner is used in the production code.
There was a problem hiding this comment.
Thanks for the explanation. Did you encounter a bug that led you to find this?
Reviewable status:
|
I hit it when performing a clean up in #89052. That PR attempts to separate out the shutting down of "flow" infrastructure from closing of the "plan" stuff (currently we have to "close" the latter before cleaning up the former). This bug can be hit when evaluating postqueries, but previously it was hidden because we delayed shutting down the "flow" infrastructure (which correctly unsets the txn in the eval context) until after postqueries complete. TFTR! bors r+ |
|
Build succeeded: |
|
Should we backport this, or are we unlikely to encounter it when backporting future PRs to 22.2? |
|
copyshim.go will be gone from master soon and then backported to 22.2 for the .1 release so it will solely exist on 22.2.0 branch. I don't see the point of fixing it there. |
|
Furthermore, this is a test-only code with this bug being hidden on 22.2 due to some other code. |
Previously, we forgot to update the txn of the eval context of the planner.
Release note: None