On 11/13/26 15:58, Siddharth Kothari wrote:
> Hi Tomas,
> Thanks for the detailed review, and no worries about the delay.
>
>> 1) I don't think the new ExecCustomScan callbacks can be this simple.
>> ... I suggest to make this more like the parallel query callbacks, so
>> that the core "owns" the allocation, and the CustomScan merely
>> estimates it, initializes it (after allocation, etc.).
>
> Makes sense. Updated it accordingly.The wrappers in nodeCustom.c now
> mirror ExecCustomScanInitializeDS.
> The estimate callback returns a Size, and the executor does
> shm_toc_estimate_chunk/keys, shm_toc_allocate,
> shm_toc_insert and shm_toc_lookup itself, passing the chunk to the
> provider as "coordinate". The size is carried between the two phases in
> a new CustomScanState.pinstrument_len, like pscan_len.
>
I didn't do a proper review, but seems OK.
> For the key I added PARALLEL_KEY_NODE_INSTRUMENTATION(plan_node_id) in
> execParallel.h, defined as 0xE100000000000000 + plan_node_id. That keeps
> it clear of plan_node_id (used for the parallel-aware chunk) and of the
> executor's fixed 0xE0.. keys, and the same macro serves ForeignScan.
> It lives in the header rather than in execParallel.c only because
> nodeCustom.c and nodeForeignscan.c both need it; happy to move or
> rename it if you prefer.
>
Why not to simply copy what the other scan nodes do?
shm_toc_insert(pcxt->toc,
node->ss.ps.plan->plan_node_id +
PARALLEL_KEY_SCAN_INSTRUMENT_OFFSET,
node->sinstrument);
>> 2) AFAIK the callbacks are called even when instrumentation was not
>> requested, which seems a bit weird.
>
> Done. The wrappers now return early unless the node has instrumentation
> and there are workers, i.e. "if (!node->ss.ps.instrument ||
> pcxt->nworkers == 0)", which is what nodeSort.c, nodeAgg.c and
> nodeMemoize.c do for their shared instrumentation. I went with that
> rather than SeqScan's INSTRUMENT_IO test since the executor cannot know
> which kind of stats a given provider collects.
>
Yeah. I'd probably keep the checks separate, to make it look more like
the other scans. Something like
if (!node->ss.ps.instrument || pcxt->nworkers == 0 ||
node->pinstrument_len == 0)
return;
if (!methods->InitializeDSMInstrumentationCustomScan)
return;
instead of one fused check. But that's a detail. BTW, why do we need the
pinstrument_len check?
>> 3) I'm not sure if the documentation should have more details about
>> what the callbacks should do
>
> Updated the documentation.
>
Haven't looked yet.
>> 4) There need to be some tests.
>
> Added tests.
>
Haven't looked yet.
>> 5) ... the EXPLAIN probably won't be able to print per-worker stats
>> because ExplainOpenWorker/ExplainCloseWorker are static in explain.c.
>
> Yep, the test needed them. I exported the two functions
> (declared in explain.h since that is where they are defined) rather
> than adding another callback; the provider's ExplainCustomScan then
> looks exactly like show_sort_info. Output of existing providers that do
> not use any of this is unchanged.
>
OK. I wonder if exporting the functions is the right solution. Maybe we
should instead provide explain the information in a structured way, so
that explain.c can do the rest. But I guess explain.c does not know what
the statistics are.
>> 6) I realized the same issue (lack of instrumentation) applies to
>> ForeignScan too.
>
> Took the liberty of adding a patch for ForeignScan as well (0002). It
> is the same change
> for FdwRoutine, sharing the key macro and the gating.
>
> I started on a test for it and realized it needs a bit more thought.
> There is no test FDW that is parallel-safe. file_fdw is, so a test
> would mean implementing the callbacks there and printing per-worker
> counts in its EXPLAIN output, which is a small user-visible feature for
> file_fdw rather than just a test. The alternative is a dummy
> parallel-safe FDW under src/test/modules - which would need a non
> trivial amount of scaffolding. I'm happy to do either as a
> follow-up once we agree which one; in the meantime the executor side is
> identical to the CustomScan paths that test_extensible covers.
>
OK. I was not really expecting / suggesting you do the work, but I won't
complain. Haven't looked yet.
>> 7) The naming of the new callbacks in CustomExecMethods seems a bit
>> inconsistent.
>
> Done. All four now end in "CustomScan":
> EstimateDSMInstrumentationCustomScan,
> InitializeDSMInstrumentationCustomScan,
> InitializeWorkerInstrumentationCustomScan,
> RetrieveInstrumentationCustomScan.
>
OK. The naming is somewhat "unnatural", but I don't have a better
option, and the naming convention was mostly set by the existing functions.
> Have attached two v5 patches, one for CustomScan and another for ForeignScan
> that pass make check and the test_extensible suite.
>
Thank. I'll try to take a closer look sometime next week.
regards
--
Tomas Vondra
