colflow: fix premature stats collection and improve tracking#62255
colflow: fix premature stats collection and improve tracking#62255craig[bot] merged 1 commit intocockroachdb:masterfrom
Conversation
This commit achieves two goals: 1. previously, we could call `finishVectorizedStatsCollectors` prematurely in some edge cases (e.g. when the right side of the hash join is empty, we could collect the stats from the left input tree too early). This was caused by the fact that we accumulate the vectorized stats collectors into a queue which is handled by the outboxes or the root materializer. This commit begins sharing the responsibility of the collecting stats with the hash router too. Only the hash router needs updating to achieve the fix because it is currently the only component that splits an input stream. The change is also sound because when the hash router exits, all stats collectors in its input tree should have the final info, and the hash router can collect correct stats. Those stats are added as another metadata object to be returned by the last to exit router output. 2. `vectorizedStatsCollectorsQueue` on the flow creator is removed in favor of more precise tracking of which stats collectors belong to which tree. The motivation behind this changes is that the follow-up commits will unify the retrieval of stats and draining of the metadata sources so that the former always occurred right before the latter. Also this commits renames `metadataSourcesQueue` to `metadataSources` and changes the order of arguments in a few functions. Release note (bug fix): Previously, CockroachDB when collecting execution statistics (e.g. when running EXPLAIN ANALYZE) could collect them prematurely which would result in incorrect stats. This is now fixed.
|
This change is |
|
This PR is a slightly updated version of #62179 which additionally achieves the second goal mentioned in the commit message. To provide more context why I decided to go down this route: I began implementing Alfonso's suggestion to introduce an invariant checker that all stats are collected before Essentially in order to get the stats invariants checker working, we need to have the same behavior for the stats collectors as we added in 2d4ad44 for the metadata sources (the materializers for wrapped processors acquiring the ownership), so I think it'll be beneficial to generalize that. To that end #62221 will attempt the following:
It will track a root of the tree along with "meta" objects within that tree the ownership of which hasn't been claimed yet
I think such design will lead to much cleaner and more maintainable code overall. However, the changes are pretty invasive, so all of them wouldn't be backportable, so I extracted the first step together with the fix into this PR. Please let me know your thoughts. |
asubiotto
left a comment
There was a problem hiding this comment.
Reviewed 25 of 25 files at r1.
Reviewable status:complete! 1 of 0 LGTMs obtained (waiting on @yuzefovich)
pkg/sql/colflow/vectorized_flow.go, line 1023 at r1 (raw file):
return err } s.vectorizedStatsCollectorsQueue = s.vectorizedStatsCollectorsQueue[:0]
Can we remove this queue?
yuzefovich
left a comment
There was a problem hiding this comment.
TFTR!
bors r+
Reviewable status:
pkg/sql/colflow/vectorized_flow.go, line 1023 at r1 (raw file):
Previously, asubiotto (Alfonso Subiotto Marqués) wrote…
Can we remove this queue?
It is removed in this commit, yes. If your question is whether it is safe to do so, then I believe it is because we're extracting the tracking of the stats collectors to be more explicit, and not as a "queue".
|
Build failed (retrying...): |
|
Build succeeded: |
This commit achieves two goals:
previously, we could call
finishVectorizedStatsCollectorsprematurelyin some edge cases (e.g. when the right side of the hash join is empty,
we could collect the stats from the left input tree too early). This was
caused by the fact that we accumulate the vectorized stats collectors
into a queue which is handled by the outboxes or the root materializer.
This commit begins sharing the responsibility of the collecting stats
with the hash router too. Only the hash router needs updating to achieve
the fix because it is currently the only component that splits an input
stream. The change is also sound because when the hash router exits, all
stats collectors in its input tree should have the final info, and the
hash router can collect correct stats. Those stats are added as another
metadata object to be returned by the last to exit router output.
vectorizedStatsCollectorsQueueon the flow creator is removed infavor of more precise tracking of which stats collectors belong to which
tree. The motivation behind this changes is that the follow-up commits
will unify the retrieval of stats and draining of the metadata sources so
that the former always occurred right before the latter.
Also this commits renames
metadataSourcesQueuetometadataSourcesandchanges the order of arguments in a few functions.
Fixes: #56928.
Release note (bug fix): Previously, CockroachDB when collecting
execution statistics (e.g. when running EXPLAIN ANALYZE) could collect
them prematurely which would result in incorrect stats. This is now
fixed.