sql: almost remove context.Context from eval.Context #85782
Conversation
|
This change is |
yuzefovich
changed the title
There was a problem hiding this comment.
Is the plan to pass a context.Context to eval.Expr separately from the eval.Context? I sort of feel like we're fighting against a gradient here. I'm worried that this is going to lead to a place where folks think that eval.Context can have even longer lifetimes and more flexible uses whereas I'd prefer if we could find a way to work harder to have fewer places where we're allowed to call eval.Expr and were more clear on what exactly that eval.Context's state was. Maybe that's wishful thinking.
Reviewable status:
complete! 0 of 0 LGTMs obtained
Yeah, that's my plan. Not sure if I get there though - it's quite daunting of a task.
Hm, are you implying that the fact that IMO storing a I remember Raphael did an analysis of how this monster |
yuzefovich
force-pushed
the
eval-context
branch
2 times, most recently
from
bf3380b to
f3e6bdd
Compare
There was a problem hiding this comment.
suggests that the latter has shorter lifetime?
I guess my stance is that if we wanted to make it sane for the eval.Context to be a context.Context and embed a context.Context, it ought to have a shorter lifetime than that context.Context. It clearly does not. Given that's I'm more or less happy with us removing the context.Context. It does leave me wanting to rename eval.Context to something else (and, I suppose, tree.SemaContext). I don't know what to call it. eval.Deps?
Reviewable status:
yuzefovich
force-pushed
the
eval-context
branch
8 times, most recently
from
16b5f42 to
2f545ed
Compare
yuzefovich
force-pushed
the
eval-context
branch
4 times, most recently
from
3729f40 to
9a0d4f2
Compare
88080: stmtdiagnostics: save the bundle on a statement timeout r=yuzefovich a=yuzefovich Previously if the traced statement is canceled due to a statement timeout, the statement bundle would be created but would fail on the insertion into the system table. This is suboptimal because we already did all the work to collect the bundle as well as it might be desired to see the partial trace, so this commit makes it so that the bundle is saved correctly. Fixes: #73477. Release note (bug fix): Previously, when a statement bundle was collected for a query that results in an error due to a `statement_timeout`, the bundle would not be saved, and this is now fixed. 88307: sql: retry validation of unique constraint r=yuzefovich a=yuzefovich Previously, when running the validation of unique constraint we would execute the query only once, and if that fails for any reason, we'd return an error. This was suboptimal because we might be performing the validation after having performed a lot of work (e.g. running an import), and the error could be a transient one (e.g. if a node that is used in the distributed validation query goes down). In order to improve the situation this commit adds a retry loop with 5 attempts with an allowlist of error that are swallowed. Fixes: #85711. Release note: None 88929: sql: add tracing spans for each statement executed within a UDF r=mgartner a=mgartner This commit creates a separate tracing span for each statement executed during evaluation of a UDF. Release note: None 88933: opt: add tests for FK delete cascade fast-path with UDF r=mgartner a=mgartner The FK delete cascade fast-path remaps the `DELETE` filter into a filter on child table, avoiding buffering the parent's deleted rows like in the non-fast-path plan. It is possible to plan this fast-path if the filters contain UDF invocations, as long as they are immutable or stable. (Of course, if the user creates a UDF with mislabeled volatility, data corruption is possible. To mitigate this and other corruption from mislabeled UDF volatility in the future, we're considering performing some analysis during `CREATE FUNCTION` to prevent users from incorrectly assigning volatility.) This commit adds tests for FK delete cascades with UDFs and removes an unnecessary TODO. Release note: None 88978: roachtest: tighten boundaries of tpch_concurrency r=yuzefovich a=yuzefovich This should remove one iteration which should keep the test under the default 10 hour timeout. Release note: None 88991: changefeedccl: Release overhead allocation r=miretskiy a=miretskiy To account for the overhead during the encoding stage, we allocate more memory resources than needed (`changefeed.event_memory_multiplier`). Those resources are released when they are amitted by the sink. Some sinks, such as file based sinks, batch many such events (to generate files of target size). This batching, when combined with compression, could result in a situation where maximum of allowed resources, `changefeed.memory.per_changefeed_limit` are all batched, making it impossible to ingest additional events without forcing a sink flush. The only way to avoid premature forced flush, is to increase the memory limit for changefeed -- but doing so is not without the costs (i.e. more events batched, more pressure on Go GC, etc). This PR adjust event resources to match the final size of the data that will be emitted into the sink -- that is, we release the overhead back into the pool, once the event processing is done. Release note: none 89035: ui: fix contention time tooltip on insights page r=ericharmeling a=ericharmeling This commit updates the High Contention Time tooltip on the insights page to accurately reflect the event's contention duration. https://cockroachlabs.slack.com/archives/G01Q9D01NTU/p1664477246961729?thread_ts=1664473625.621409&cid=G01Q9D01NTU https://www.loom.com/share/a8ce63ece586468e870d67118c0d8d5a Release note: None Release justification: bug fix 89036: eval: partial removal of usages of the stored context r=yuzefovich a=yuzefovich This PR contains several commits from #85782 which attempts to remove the stored `context.Context` from `eval.Context` object. I didn't finish that PR since I ran into two very thorny usages, so instead this PR takes some steps towards deprecating the usage of the stored context. Storing a `context.Context` is an anti-pattern that needs to have a justification for applying, and in case of `eval.Context` I think it was introduced that way specifically for scalar functions evaluations (which should use the same `context.Context`) and that was ok, but over time we introduced many more callsites out of convenience. Fixes: #46164 89066: kvserver: add storage checkpoints metric r=erikgrinaker a=pavelkalinnikov When an inconsistency between range's replicas is found, every replica's node leaves behind a storage checkpoint which can be used for finding the cause of the inconsistency. The checkpoints are cheap to make if hard links are used. However, the cost of a checkpoint on a live node increases over time because the state keeps diverging from it. Effectively it becomes a full copy of the old state, and consumes that much of disk capacity. It is important to act on the checkpoints and reclaim the space. This commit adds a store metric which exposes the number of checkpoints found, so that admins/SREs can be alerted and act on it. Touches #21128 Added a test, and also tested manually: tricked one replica to think that there is an inconsistency, this triggered all replicas to store a checkpoint and one crash; the metric on the other 2 replicas increased to 1, and I saw the corresponding directories in storage. Release note: None Co-authored-by: Yahor Yuzefovich <[email protected]> Co-authored-by: Marcus Gartner <[email protected]> Co-authored-by: Yevgeniy Miretskiy <[email protected]> Co-authored-by: Eric Harmeling <[email protected]> Co-authored-by: Pavel Kalinnikov <[email protected]>
yuzefovich
force-pushed
the
eval-context
branch
3 times, most recently
from
07408a6 to
0d55003
Compare
yuzefovich
changed the title
yuzefovich
force-pushed
the
eval-context
branch
from
0d55003 to
85c0077
Compare
yuzefovich
changed the title
|
@ajwerner I'm curious whether I could nerd-snipe you to take a look at this PR (I'll be asking for a proper review from @msirek), so I'm curious whether either of you could see a way around using I started plumbing the |
|
I don't know if there's a way around it without augmenting a bunch of methods to take a What got out of hand when you tried to make the various methods take a context.Context? |
yuzefovich
force-pushed
the
eval-context
branch
from
85c0077 to
0195be7
Compare
yuzefovich
requested review from
msirek
and removed request for
a team,
benbardin and
HonoreDB
|
First commit was mostly an automated replacement, second and third commits will be squashed, fourth and fifth commits will be squashed too. RFAL. |
yuzefovich
force-pushed
the
eval-context
branch
from
6f3b715 to
746d650
Compare
|
@cockroach-dev-inf could someone please take a look at |
|
@msirek could you please take a look at this? I'd like to merge it before going on PTO (i.e. before Friday). |
Will do. Sorry for the delay. |
There was a problem hiding this comment.
Nice work! I just have one question below.
Reviewed 57 of 122 files at r10, 3 of 224 files at r16, 222 of 222 files at r22, 72 of 72 files at r23, 111 of 111 files at r24, 29 of 29 files at r25, 33 of 33 files at r26, 8 of 10 files at r27, 4 of 4 files at r28, all commit messages.
Reviewable status:
pkg/sql/schemachanger/scbuild/internal/scbuildstmt/drop_owned_by.go line 44 at r26 (raw file):
panic(pgerror.New(pgcode.InsufficientPrivilege, "permission denied to drop objects")) } // TODO: confirm the usage of b as context.Context.
Is this something that needs to be done before merging?
pkg/sql/opt/memo/multiplicity_builder_test.go line 473 at r25 (raw file):
func makeOpBuilder(t *testing.T) testOpBuilder { ctx := eval.MakeTestingEvalContext(cluster.MakeTestingClusterSettings())
Rename to evalCtx
yuzefovich
force-pushed
the
eval-context
branch
from
746d650 to
a181b66
Compare
There was a problem hiding this comment.
TFTR!
@knz I see that you were writing something - do you have some feedback on this? I want to merge this by the EOD on Thursday if possible.
Reviewable status:
pkg/sql/schemachanger/scbuild/internal/scbuildstmt/drop_owned_by.go line 44 at r26 (raw file):
Previously, msirek (Mark Sirek) wrote…
Is this something that needs to be done before merging?
No, we're good here, removed the TODO, thanks.
pkg/sql/opt/memo/multiplicity_builder_test.go line 473 at r25 (raw file):
Previously, msirek (Mark Sirek) wrote…
Rename to
evalCtx
Done.
yuzefovich
force-pushed
the
eval-context
branch
2 times, most recently
from
c0ad815 to
d0c1356
Compare
|
Squashed some commits. Will wait for @knz to respond, and if there are no blocking comments, will merge tomorrow (if CI is green). |
This commit plumbs the `context.Context` argument to `Eval` methods explicitly in order to remove the context field from `eval.Context` in the follow-up commits. No additional plumbing is performed, so in many cases `eval.Context.Context` is passed as this new argument. Additionally it renames `ctx *eval.Context` to `evalCtx *eval.Context`. This commit is purely a mechanical change. Release note: None
This commit removes many usages of `eval.Context.Context` in favor of the newly-plumbed parameter or already available one. The only change worth calling out is the plumbing of the context for the optgen-generated `String` implementations - there we use `context.Background()` since the method is used only for testing and debugging purposes. This was needed to go around the difficult (and unnecessary) plumbing into `memo.ScalarFmtInterceptor`. Release note: None
Release note: None
This commit unexports the stored `context.Context` from `eval.Context`, marks it as "deprecated" and provides a setter method for now. There are just two places that still use it - namely `eval.Context.MustGetPlaceholderValue` and `eval.UnwrapDatum` (when unwrapping a placeholder), and refactoring those is extremely difficult since the amount of plumbing required is probably thousands of line changes without an easy way to automate that. In order to not lose progress on the removal achieved in the previous commits we do this deprecation. The next step for the complete removal is to do analysis of the web of callsites of these two methods to see whether we have incorrect layering, and this will be done separately. Release note: None
yuzefovich
force-pushed
the
eval-context
branch
from
d0c1356 to
823ea26
Compare
|
bors r+ |
|
Build succeeded: |
This PR contains several commits that take steps towards the removal of the stored
context.Contextfromeval.Context. The largest change is introducing an explicitcontext.Contextargument to thetree.TypedExpr.Evalmethod (this is done in the first commit). The remaining commits remove accesses to the stored context in most cases (via either already available context in scope or small plumbing).This PR doesn't completely remove the stored context because there were two methods
eval.Context.MustGetPlaceholderValueandeval.UnwrapDatum(when unwrapping a placeholder) that still use it. Performing the plumbing for those turned out to be a daunting task, so it was abandoned.In order to not introduce any new usages this PR unexports the stored context, provides the setter method, and marks both as deprecated.
Epic: None