Re: [PATCH] Add RetrieveInstrumentation hook for CustomScan providers

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.

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.

> 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.

> 3) I'm not sure if the documentation should have more details about
> what the callbacks should do

Updated the documentation.

> 4) There need to be some tests.

Added tests.

> 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.

> 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.

> 7) The naming of the new callbacks in CustomExecMethods seems a bit
> inconsistent.

Done. All four now end in "CustomScan":
EstimateDSMInstrumentationCustomScan,
InitializeDSMInstrumentationCustomScan,
InitializeWorkerInstrumentationCustomScan,
RetrieveInstrumentationCustomScan.

Have attached two v5 patches, one for CustomScan and another for ForeignScan
that pass make check and the test_extensible suite.

Thanks,
Siddharth