Attached is v7, with a couple adjustments based on the reviews.
The preliminary patches (beginscan flags and WaitReadStream returning if
it had to wait) were already committed, so there are no dependencies for
this patch series.
- The main change is an introduction of read_stream_enable_stats(),
which installs the IOStats pointer into an existing stream. That way we
don't need a new argument for read_stream_begin_relation(), which makes
the patch quite a bit smaller. I like this.
- Reworded a couple comments, the commit message, etc.
I also noticed a new problem. The CI reported failures on FreeBSD,
showing a weird difference in the explain output. For a while I was
really confused, because it seemed like some sort of flakiness.
But it turned out to be because of debug_parallel_query=regress (which
the FreeBSD has enabled, unlike the other configs). That makes all
queries to run as if under a Gather node, but with parallel_aware=false.
But in this case the nodes may not get the instrumentation initialized,
because execParallel.c checks plan->parallel_aware before initializing
the DSM and worker.
So there's no instrumentation, and in that case explain does not show
the stats at all. This is what caused the regression failure on FreeBSD,
because the queries outputing XML/JSON/YAML use a seqscan.
Fixing this for seqscan/tidrangescan sees simple enough - we need to
call the function even with parallel_aware=false, and then do check from
the functions to terminate early if (!parallel_aware). This is how
indexscans already do that, so I modeled it after that. I left it in
separate commits 0004 and 0006, to make it more obvious.
This works, but it's a bit ugly. At least for me it makes the code much
less readable, unfortunately. I'm not sure how else to do this, though.
There's another issue, though. BitmapHeapScan has the same issue, and
needs a similar fix, but I failed to make that work. Apparently there
are some dependencies on having the DSA that I don't understand, which
means I get segfaults if I just "copy" the indexscan approach.
FWIW this is not a new issue, it affects BitmapHeapScan already.
Not the crashes, but not showing the stats sometimes. If the queries in
explain.sql used BHS instead of a seqscan, it'd fail on CI in almost the
same way.
For a while I thought it only affects debug_parallel_query=regress, in
which case maybe the best solution would be to special-case that
somehow. But that's not really true. With a parallel query, the outer
side of the plan will have parallel_aware=false, and so will be missing
the instrumentation.
Attached is a simple reproducer, forcing a parallel join with a BHS on
both sides. Notice that the explain.log (from master) shows "Heap
Blocks" only for the inner side, while the outer side has nothing.
I suppose we should fix that, but considering no one complained about it
so far (at least I'm not aware of any reports) ...
regards
--
Tomas Vondra
