New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
sql: don't bind a Context in BoundAccount #10478
Conversation
Reviewed 17 of 17 files at r1. pkg/sql/explain.go, line 149 at r1 (raw file):
a lot of work has gone into unwinding this pattern (of storing a context in structures) from the core. can you explain (in a comment) why it is necessary here? AFAICT, it's to avoid plumbing a context into Start, which would deserve a TODO. pkg/sql/explain.go, line 257 at r1 (raw file):
ditto pkg/sql/group.go, line 174 at r1 (raw file):
ditto pkg/sql/table_join.go, line 44 at r1 (raw file):
ditto pkg/sql/values.go, line 35 at r1 (raw file):
ditto pkg/sql/mon/mem_usage.go, line 456 at r1 (raw file):
nit: you could remove this Comments from Reviewable |
Review status: all files reviewed at latest revision, 6 unresolved discussions, some commit checks failed. pkg/sql/explain.go, line 149 at r1 (raw file):
|
On the one hand I support the motion of being more explicit about contexts, however the way you did it looks messy. See my comments below. I recommend removing the nit: I believe it's "fleshed out" not "flushed out." Reviewed 17 of 17 files at r1. pkg/server/admin.go, line 190 at r1 (raw file):
probably pkg/server/admin.go, line 224 at r1 (raw file):
ditto pkg/server/admin.go, line 276 at r1 (raw file):
not sure this is needed, you can pull it from session pkg/server/admin.go, line 282 at r1 (raw file):
ditto pkg/server/admin.go, line 320 at r1 (raw file):
ditto pkg/server/admin.go, line 481 at r1 (raw file):
ditto pkg/server/admin.go, line 487 at r1 (raw file):
ditto pkg/server/admin.go, line 634 at r1 (raw file):
ditto pkg/server/admin.go, line 676 at r1 (raw file):
ditto pkg/server/admin.go, line 716 at r1 (raw file):
arg not needed - pkg/server/admin.go, line 736 at r1 (raw file):
see above pkg/server/admin.go, line 783 at r1 (raw file):
see above pkg/server/admin.go, line 789 at r1 (raw file):
see above pkg/server/admin.go, line 802 at r1 (raw file):
see above pkg/server/admin.go, line 815 at r1 (raw file):
see above pkg/server/admin.go, line 845 at r1 (raw file):
see above pkg/server/admin.go, line 1319 at r1 (raw file):
see above pkg/server/admin.go, line 1325 at r1 (raw file):
see above pkg/server/admin.go, line 1353 at r1 (raw file):
see above pkg/server/admin.go, line 1367 at r1 (raw file):
see above pkg/server/admin.go, line 1374 at r1 (raw file):
see above pkg/server/admin.go, line 1399 at r1 (raw file):
see above pkg/server/admin.go, line 1403 at r1 (raw file):
see above pkg/server/admin_test.go, line 243 at r1 (raw file):
pkg/server/admin_test.go, line 271 at r1 (raw file):
see above pkg/server/admin_test.go, line 302 at r1 (raw file):
see above pkg/server/admin_test.go, line 408 at r1 (raw file):
see above pkg/server/admin_test.go, line 487 at r1 (raw file):
see above pkg/server/admin_test.go, line 577 at r1 (raw file):
see above pkg/server/admin_test.go, line 616 at r1 (raw file):
see above pkg/server/admin_test.go, line 673 at r1 (raw file):
see above pkg/server/admin_test.go, line 725 at r1 (raw file):
see above pkg/server/admin_test.go, line 776 at r1 (raw file):
see above pkg/sql/executor.go, line 577 at r1 (raw file):
pkg/sql/executor.go, line 1048 at r1 (raw file):
ditto pkg/sql/executor.go, line 1224 at r1 (raw file):
ditto pkg/sql/explain.go, line 114 at r1 (raw file):
capture p not ctx pkg/sql/explain.go, line 133 at r1 (raw file):
ditto pkg/sql/explain.go, line 182 at r1 (raw file):
drop the argument, see below pkg/sql/explain.go, line 210 at r1 (raw file):
make this a method of pkg/sql/explain.go, line 257 at r1 (raw file):
|
I think having the nodes figure out if they want the session ctx or the txn context would be going the wrong way. I think they never want anything other than the inner-most context available to them. And the question is whether that can be anything other than the transaction one... And I think the problem is with preparing... because that can happen with or without a transaction being active (right?). Review status: all files reviewed at latest revision, 102 unresolved discussions, some commit checks failed. Comments from Reviewable |
I've replied to the comments on the first few files, up to where the planNodes start. Review status: all files reviewed at latest revision, 102 unresolved discussions, some commit checks failed. pkg/server/admin.go, line 190 at r1 (raw file):
|
4145320
to
d0b0e1b
Compare
Unless you're in a hurry I'd like to spend a few days mulling over alternatives. Let's chat Friday. |
Reviewed 1 of 2 files at r2. Comments from Reviewable |
d0b0e1b
to
2cb025f
Compare
I've rewritten this PR, plumbing contexts through the cc @tamird Review status: 0 of 101 files reviewed at latest revision, 102 unresolved discussions, some commit checks failed. Comments from Reviewable |
Reviewed 3 of 83 files at r3. pkg/sql/parser/eval.go, line 1744 at r3 (raw file):
why? is this really just to avoid renaming "ctx" in existing methods? Comments from Reviewable |
To say that this is hard to review would be an understatement. You've removed some stored contexts, added others, changed some plumbing, and all this is spread out over multiple commits that don't appear to be meaningfully distinct. What's the big picture? Why does this stop short of plumbing contexts everywhere they are needed? Which stored contexts now remain? Reviewed 80 of 83 files at r3, 25 of 25 files at r4, 17 of 44 files at r5. pkg/sql/alter_table.go, line 473 at r3 (raw file):
throughout: you don't need this underscore. they're only needed when some of the arguments are named. pkg/sql/distsql_physical_planner.go, line 1398 at r4 (raw file):
proliferating more stored contexts is not the way I'd like to see this achieved. pkg/sql/executor.go, line 1383 at r3 (raw file):
well why doesn't it? pkg/sql/lease.go, line 1257 at r3 (raw file):
while you're here, can you extract all these contexts into a local? pkg/sql/lease_test.go, line 202 at r3 (raw file):
i mean if you're going to do this, you can just pkg/sql/schema_changer.go, line 840 at r3 (raw file):
? pkg/sql/subquery.go, line 286 at r3 (raw file):
again, why not finish this here? pkg/sql/walk.go, line 75 at r3 (raw file):
👎 pkg/sql/pgwire/v3.go, line 336 at r4 (raw file):
why not replace this comment with Comments from Reviewable |
Side note: this may fix #13837. |
Reviewed 45 of 83 files at r3, 13 of 25 files at r4, 42 of 44 files at r5, 1 of 1 files at r6. pkg/sql/distsql_physical_planner.go, line 1398 at r4 (raw file): Previously, tamird (Tamir Duberstein) wrote…
That's true, meanwhile I also think this can be addressed in a subsequent PR. pkg/sql/executor.go, line 577 at r1 (raw file): Previously, andreimatei (Andrei Matei) wrote…
So I take it that we're keeping a context reference in the session after all? pkg/sql/executor.go, line 1293 at r6 (raw file):
Please store the context reference in a local variable here and reuse it throughout. Will increase legibility. pkg/sql/lease.go, line 1257 at r3 (raw file): Previously, tamird (Tamir Duberstein) wrote…
👍 pkg/sql/lease_test.go, line 202 at r3 (raw file): Previously, tamird (Tamir Duberstein) wrote…
👍 pkg/sql/pg_catalog.go, line 1237 at r6 (raw file):
I'd extend the pkg/sql/schema_changer.go, line 840 at r3 (raw file): Previously, tamird (Tamir Duberstein) wrote…
Please phrase the comment as a TODO and/or (preferred) file a follow-up issue. pkg/sql/schema_changer.go, line 318 at r6 (raw file):
pkg/sql/schema_changer.go, line 337 at r6 (raw file):
pkg/sql/schema_changer.go, line 440 at r6 (raw file):
pkg/sql/session.go, line 176 at r6 (raw file):
Why s.Ctx() here and not s.context? pkg/sql/session.go, line 475 at r6 (raw file):
Is there no better context to plumb from the caller here? pkg/sql/subquery.go, line 286 at r3 (raw file): Previously, tamird (Tamir Duberstein) wrote…
In this particular case I tend to disagree. I think that visitors whose work have a properly scoped lifetime should store the context. Stored context are not the devil, as long as the lifetime bounds are properly documented (please add a comment here to explain this)n pkg/sql/trace.go, line 108 at r6 (raw file):
I would kinda prefer names other than "hijack" for these methods (here and on planner). Perhaps something based on "graft" instead? or "wrap"? or "instrument"? pkg/sql/trace.go, line 146 at r6 (raw file):
Please clarify what happens if pkg/sql/values_test.go, line 116 at r6 (raw file):
t.Context? here and in other tests. pkg/sql/walk.go, line 75 at r3 (raw file): Previously, tamird (Tamir Duberstein) wrote…
YEah no. Binding the context in the visitor is fine but you don't need to create a new instance for that. pkg/sql/distsqlrun/aggregator.go, line 120 at r6 (raw file):
? pkg/sql/distsqlrun/aggregator_test.go, line 285 at r6 (raw file):
? pkg/sql/distsqlrun/distinct.go, line 50 at r6 (raw file):
? pkg/sql/distsqlrun/flow.go, line 47 at r6 (raw file):
I think this is time to document the fields of this struct. pkg/sql/distsqlrun/flow.go, line 51 at r6 (raw file):
? pkg/sql/distsqlrun/joinerbase.go, line 64 at r6 (raw file):
? pkg/sql/distsqlrun/joinreader.go, line 73 at r6 (raw file):
? pkg/sql/distsqlrun/joinreader_test.go, line 99 at r6 (raw file):
? pkg/sql/distsqlrun/mergejoiner_test.go, line 292 at r6 (raw file):
? pkg/sql/distsqlrun/processors.go, line 231 at r6 (raw file):
? pkg/sql/distsqlrun/server.go, line 111 at r6 (raw file):
Can you extend this comment (or file an issue) to sketch how this should be done. pkg/sql/distsqlrun/sorter.go, line 51 at r6 (raw file):
? pkg/sql/distsqlrun/tablereader.go, line 95 at r6 (raw file):
? pkg/sql/distsqlrun/tablereader_test.go, line 118 at r6 (raw file):
? pkg/sql/distsqlrun/values.go, line 51 at r6 (raw file):
? pkg/sql/parser/eval.go, line 1720 at r6 (raw file):
Why this type alias instead of pkg/sql/parser/normalize.go, line 19 at r6 (raw file):
I'm not sure this change belongs to this PR. pkg/sql/pgwire/v3.go, line 336 at r4 (raw file): Previously, tamird (Tamir Duberstein) wrote…
same question as tamir Comments from Reviewable |
Reviewed 1 of 83 files at r3, 1 of 44 files at r5. Comments from Reviewable |
(Discussed offline: the change in the flow interface is fine, but the rationale for it should be in the final commit message.) |
2cb025f
to
63f78fd
Compare
Review status: 30 of 102 files reviewed at latest revision, 43 unresolved discussions, some commit checks failed. pkg/sql/alter_table.go, line 473 at r3 (raw file): Previously, tamird (Tamir Duberstein) wrote…
Done. pkg/sql/distsql_physical_planner.go, line 1398 at r4 (raw file): Previously, knz (kena) wrote…
I'll leave this one for now... No easy solution pkg/sql/executor.go, line 577 at r1 (raw file): Previously, knz (kena) wrote…
well you have to save a context somewhere... If a tracing span is to be used across queries, for example. I think keeping a ctx in the session makes sense. pkg/sql/executor.go, line 1383 at r3 (raw file): Previously, tamird (Tamir Duberstein) wrote…
Done. pkg/sql/executor.go, line 1293 at r6 (raw file): Previously, knz (kena) wrote…
Done. pkg/sql/group.go, line 174 at r1 (raw file): Previously, tamird (Tamir Duberstein) wrote…
Done. pkg/sql/lease.go, line 1257 at r3 (raw file): Previously, knz (kena) wrote…
Done. pkg/sql/lease_test.go, line 202 at r3 (raw file): Previously, knz (kena) wrote…
pkg/sql/pg_catalog.go, line 1237 at r6 (raw file): Previously, knz (kena) wrote…
Done. pkg/sql/schema_changer.go, line 840 at r3 (raw file): Previously, knz (kena) wrote…
added TODO pkg/sql/schema_changer.go, line 318 at r6 (raw file): Previously, knz (kena) wrote…
Done. pkg/sql/schema_changer.go, line 337 at r6 (raw file): Previously, knz (kena) wrote…
Done. pkg/sql/schema_changer.go, line 440 at r6 (raw file): Previously, knz (kena) wrote…
Done. pkg/sql/session.go, line 176 at r6 (raw file): Previously, knz (kena) wrote…
changed pkg/sql/session.go, line 475 at r6 (raw file): Previously, knz (kena) wrote…
there is indeed. done. pkg/sql/subquery.go, line 286 at r3 (raw file): Previously, knz (kena) wrote…
I could go either way on these visitors. They do have a limited lifespan, but also they have more than one external method (so you may want different contexts for the pre/post stages) and it somehow would feel natural for these methods to take a context. I'll leave this as is for now. pkg/sql/trace.go, line 108 at r6 (raw file): Previously, knz (kena) wrote…
I think "hijack" signifies that something funky is going on - a layer that you wouldn't think should have any control over this session context wants to... hijack it. pkg/sql/trace.go, line 146 at r6 (raw file): Previously, knz (kena) wrote…
the call to pkg/sql/values.go, line 35 at r1 (raw file): Previously, tamird (Tamir Duberstein) wrote…
Done. pkg/sql/values_test.go, line 116 at r6 (raw file): Previously, knz (kena) wrote…
not a thing pkg/sql/walk.go, line 75 at r3 (raw file): Previously, knz (kena) wrote…
discussed with Rafa, there's nothing to do here. I'm not creating any more instances than before. pkg/sql/distsqlrun/aggregator.go, line 120 at r6 (raw file): Previously, knz (kena) wrote…
Done. pkg/sql/distsqlrun/aggregator_test.go, line 285 at r6 (raw file): Previously, knz (kena) wrote…
Done. pkg/sql/distsqlrun/distinct.go, line 50 at r6 (raw file): Previously, knz (kena) wrote…
Done. pkg/sql/distsqlrun/flow.go, line 47 at r6 (raw file): Previously, knz (kena) wrote…
the AmbientContext? I don't think we document it elsewhere... Not sure what to say other than the documentation on the type definition... I'll leave it alone. pkg/sql/distsqlrun/flow.go, line 51 at r6 (raw file): Previously, knz (kena) wrote…
Done. pkg/sql/distsqlrun/joinerbase.go, line 64 at r6 (raw file): Previously, knz (kena) wrote…
Done. pkg/sql/distsqlrun/joinreader.go, line 73 at r6 (raw file): Previously, knz (kena) wrote…
Done. pkg/sql/distsqlrun/joinreader_test.go, line 99 at r6 (raw file): Previously, knz (kena) wrote…
Done. pkg/sql/distsqlrun/mergejoiner_test.go, line 292 at r6 (raw file): Previously, knz (kena) wrote…
Done. pkg/sql/distsqlrun/processors.go, line 231 at r6 (raw file): Previously, knz (kena) wrote…
Done. pkg/sql/distsqlrun/server.go, line 111 at r6 (raw file): Previously, knz (kena) wrote…
I don't really have anything else to say at the moment :) pkg/sql/distsqlrun/sorter.go, line 51 at r6 (raw file): Previously, knz (kena) wrote…
Done. pkg/sql/distsqlrun/tablereader.go, line 95 at r6 (raw file): Previously, knz (kena) wrote…
Done. pkg/sql/distsqlrun/tablereader_test.go, line 118 at r6 (raw file): Previously, knz (kena) wrote…
Done. pkg/sql/distsqlrun/values.go, line 51 at r6 (raw file): Previously, knz (kena) wrote…
Done. pkg/sql/mon/mem_usage.go, line 456 at r1 (raw file): Previously, tamird (Tamir Duberstein) wrote…
Done. pkg/sql/parser/eval.go, line 1744 at r3 (raw file): Previously, tamird (Tamir Duberstein) wrote…
it's not about renaming, but about not changing the Eval interface. Thought some more after discussing with you; I again think there's no reason to change that interface and extract the ctx from the EvalCtx. pkg/sql/parser/eval.go, line 1720 at r6 (raw file): Previously, knz (kena) wrote…
well because we'll be assigning this to pkg/sql/pgwire/v3.go, line 336 at r4 (raw file): Previously, knz (kena) wrote…
because pkg/sql/parser/normalize.go, line 19 at r6 (raw file): Previously, knz (kena) wrote…
not intentional, removed. Comments from Reviewable |
Reviewed 18 of 73 files at r7, 13 of 24 files at r8, 42 of 43 files at r9, 1 of 1 files at r10. pkg/sql/executor.go, line 874 at r5 (raw file):
Why did you remove this line? It seems to me that executing statements may change the txn context (e.g. via begin/commit), so we'd need to refresh the pkg/sql/lease.go, line 1217 at r10 (raw file):
Perhaps here either a TODO comment with a suggestion as to what needs to be done (ambient context? more plumbing?) and/or an issue to follow-up. pkg/sql/trace.go, line 108 at r6 (raw file): Previously, andreimatei (Andrei Matei) wrote…
ack pkg/sql/trace.go, line 146 at r6 (raw file): Previously, andreimatei (Andrei Matei) wrote…
But that's the thing: I do not see (without additional explanation) why this conditional is sufficient. What makes pkg/sql/distsqlrun/flow.go, line 47 at r6 (raw file): Previously, andreimatei (Andrei Matei) wrote…
I was thinking about documenting all the fields in this struct, not just this one. pkg/sql/parser/eval.go, line 1720 at r6 (raw file): Previously, andreimatei (Andrei Matei) wrote…
Oh ok carry on. Comments from Reviewable |
63f78fd
to
c3a71b6
Compare
Review status: all files reviewed at latest revision, 19 unresolved discussions, some commit checks failed. pkg/sql/executor.go, line 874 at r5 (raw file): Previously, knz (kena) wrote…
you're right. As we've discussed, I've tried to plumb a pkg/sql/lease.go, line 1217 at r10 (raw file): Previously, knz (kena) wrote…
I don't have anything insightful to write in an issue, and there's still pkg/sql/trace.go, line 146 at r6 (raw file): Previously, knz (kena) wrote…
the relevant condition was pkg/sql/distsqlrun/flow.go, line 47 at r6 (raw file): Previously, knz (kena) wrote…
I've added a small commit commenting them. Did my best :P. Comments from Reviewable |
Reviewed 6 of 61 files at r11, 13 of 24 files at r12, 40 of 43 files at r13, 1 of 1 files at r14, 1 of 1 files at r15. pkg/sql/lease.go, line 1217 at r10 (raw file): Previously, andreimatei (Andrei Matei) wrote…
👍 pkg/sql/prepare.go, line 85 at r15 (raw file):
Well then I'd suggest making the methods of the pkg/sql/trace.go, line 146 at r6 (raw file): Previously, andreimatei (Andrei Matei) wrote…
Nice, thanks. pkg/sql/distsqlrun/flow.go, line 47 at r6 (raw file): Previously, andreimatei (Andrei Matei) wrote…
Excellent. pkg/sql/parser/builtins.go, line 1571 at r15 (raw file):
Doesn't seem that this belongs to this PR. Comments from Reviewable |
c3a71b6
to
72cee49
Compare
Review status: 47 of 102 files reviewed at latest revision, 17 unresolved discussions, some commit checks failed. pkg/sql/prepare.go, line 85 at r15 (raw file): Previously, knz (kena) wrote…
This sounds off. Ping me when you can :) pkg/sql/parser/builtins.go, line 1571 at r15 (raw file): Previously, knz (kena) wrote…
What doesn't belong? Depending on what diff you look at, you might see a Comments from Reviewable |
72cee49
to
8daff20
Compare
first commit message lies:
that doesn't happen, afaict. please consider squashing these commits:
the separation in this case does more harm than good. the TL;DR here is that storing a context is generally a bad thing. Reviewed 9 of 73 files at r7, 5 of 61 files at r11, 62 of 62 files at r16, 24 of 24 files at r17, 43 of 43 files at r18, 1 of 1 files at r19, 1 of 1 files at r20. pkg/sql/planner.go, line 179 at r17 (raw file):
🎊 🎉 pkg/sql/sort.go, line 477 at r17 (raw file):
several unnecessary underscores here (resolved in a later commit). seriously, please consider squashing! pkg/sql/trace.go, line 99 at r18 (raw file): However, the commit message simply reads:
and this change is doing a bit more than that. pkg/sql/values.go, line 34 at r17 (raw file):
this needs a comment explaining why it is done (the heap interface, i guess?) heh, this was also resolved in a later commit. another plea for a squash. pkg/sql/values.go, line 234 at r20 (raw file):
while you're here you can s/x // pkg/sql/distsqlrun/flow.go, line 50 at r20 (raw file):
identifier pkg/sql/distsqlrun/flow.go, line 52 at r20 (raw file):
to evaluate what? pkg/sql/distsqlrun/server.go, line 102 at r18 (raw file):
they do? which ones? Comments from Reviewable |
8daff20
to
d19611c
Compare
squashed Thanks for the review! Review status: 98 of 102 files reviewed at latest revision, 9 unresolved discussions, all commit checks successful. pkg/sql/prepare.go, line 85 at r15 (raw file): Previously, andreimatei (Andrei Matei) wrote…
discussed this at length and opinions unfortunately still differ. I'll merge like this, pending further consideration. I was looking at some code, though. Seems to me that
How about we get rid of both pkg/sql/distsqlrun/flow.go, line 50 at r20 (raw file): Previously, tamird (Tamir Duberstein) wrote…
Done. pkg/sql/distsqlrun/flow.go, line 52 at r20 (raw file): Previously, tamird (Tamir Duberstein) wrote…
Done. pkg/sql/distsqlrun/server.go, line 102 at r18 (raw file): Previously, tamird (Tamir Duberstein) wrote…
referenced an issue Comments from Reviewable |
The objective is to be more explicit about context passing: don't store the context in some planNodes and get rid of planner.Ctx(). - a ctx is added to important planNode methods - don't bind a Context in BoundAccount* references cockroachdb#7992 *a BoundAccount is a MemoryAccount bound to a MemoryMonitor. When creating a BoundAcc, we used to bind a context to be used by the Acc throughout its lifetime. This means that the context had to stay "valid" throughout (e.g. the span in the ctx, if any, had to stay open). This is conceptually a problem because it means you can't open an acc in one place and close it in a completely different place (and also diminishes the utility of contexts, as Account operations were not logged in a context corresponding to the function actually performing them). In particular, this was a problem for accounts created for RowContainers and the way how EXPLAIN(TRACE) temporarily hijacks the session's context: a container's account could easily be bound to one of these temporarily hijacked contexts, and closed after the corresponding span had already been Finish()ed. This patch plumbs through a Context to the accounts, through various cascading layers.
Also slightly simplify code.
d19611c
to
a7284aa
Compare
Reviewed 1 of 73 files at r7, 1 of 43 files at r18, 8 of 10 files at r21, 1 of 1 files at r22, 1 of 1 files at r23. Comments from Reviewable |
A BoundAccount is a MemoryAccount bound to a MemoryMonitor. When
creating a BoundAcc, we used to bind a context to be used by the Acc
throughout its lifetime. This means that the context had to stay "valid"
throughout (e.g. the span in the ctx, if any, had to stay open). This is
conceptually a problem because it means you can't open an acc in one
place and close it in a completely different place (and also diminishes
the utility of contexts, as Account operations were not logged in a
context corresponding to the function actually performing them). In
particular, this was a problem for accounts created for RowContainers
and the way how EXPLAIN(TRACE) temporarily hijacks the session's
context: a container's account could easily be bound to one of these
temporarily hijacked contexts, and closed after the corresponding span
had already been Finish()ed.
This patch plumbs through a Context to the accounts, through various
cascading layers.
A bunch of accounts are opened by various planNodes, and there the
question of what context to use I believe is not yet flushed out. So
far, planNodes that already used contexts were using the planner's
current context (which is the session's context). Not all planNodes
maintained a reference to the planner, though. Where a reference
existed, I continued using it. Where it didn't, instead of adding one,
I've bound a context directly to the planNode at ctor time.
cc @RaduBerinde in case you've got any thoughts on how
planNodes
should think about contexts.This change is