Re: [PATCH] Add RetrieveInstrumentation hook for CustomScan providers

Hi Siddharth,

Sorry for the long silence. Too much other work :-(

I did a review of the v4 patch today. I haven't found any outright
correctness bug - it modifies the right places, has the right overall
structure, etc. But I think there's a couple loose ends / gaps that need
to be addressed to make this committable.

1) I don't think the new ExecCustomScan callbacks can be this simple.
It's probably right to model this after the parallel query callbacks, so
consider this example:

void
ExecCustomScanInitializeDSM(CustomScanState *node, ...)
{
const CustomExecMethods *methods = node->methods;

if (methods->InitializeDSMCustomScan)
{
int plan_node_id = node->ss.ps.plan->plan_node_id;
void *coordinate;

coordinate = shm_toc_allocate(pcxt->toc, node->pscan_len);
methods->InitializeDSMCustomScan(node, pcxt, coordinate);
shm_toc_insert(pcxt->toc, plan_node_id, coordinate);
}
}

Compare this to the new instrumentation callback doing the same thing:

void
ExecCustomScanInstrumentInitDSM(CustomScanState *node, ...)
{
const CustomExecMethods *methods = node->methods;

if (methods->InitializeDSMCustomScanInstrumentation)
methods->InitializeDSMCustomScanInstrumentation(node, pcxt);
}

Clearly, it does much less. It's up to the custom scan implementation to
do all the shmem setup (shm_toc_alloc, and similarly for the other
callbacks). But why should it be done there? That code is not in a good
position to do that. For example, what key should it use for the new
DSM? It does not really know which keys are already used, e.g. it can't
use the "natural" plan_node_id as that's already used.

A duplicate key likely triggers a crash or some sort of weird errors.

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

2) AFAIK the callbacks are called even when instrumentation was not
requested, which seems a bit weird. Consider what the existing scan
nodes (like nodeSeqScan.c) do:

if ((estate->es_instrument & INSTRUMENT_IO) == 0)
return;

I suppose the CustomScan should do something similar (possibly in a
slightly different way than SeqScan).

3) I'm not sure if the documentation should have more details about what
the callbacks should do (e.g. that "Retrieve" runs at the end of a
query, not on every rescan, that it needs to copy data out, etc.).

4) There need to be some tests. We actually already have CustomScan
tests in src/test/modules/test_extensible - seems natural to just expand
those to also implement and test the new callbacks. It should also print
the collected instrumentation data in EXPLAIN. This will probably
provide some ideas what needs to be mentioned in the docs.

5) The tests may also reveal that some bits are missing, or are not
possible to implement. For example, the EXPLAIN probably won't be able
to print per-worker stats because ExplainOpenWorker/ExplainCloseWorker
are static in explain.c. I'm not sure if we should export them, or if we
need a new callback in the CustomScan (next to the one we already have
for regular EXPLAIN).

6) I realized the same issue (lack of instrumentation) applies to
ForeignScan too. That's a separate patch, but I wonder if doing them
both would be helpful - it seems like a fairly similar.

7) The naming of the new callbacks in CustomExecMethods seems a bit
inconsistent. Most other callbacks end with "CustomScan" but I realize
that conflicts with the parallel query callbacks, so you added the
"Instrumentation" at the end. But then RetrieveInstrumentationCustomScan
breaks that rule. I guess RetrieveCustomScanInstrumentation would be
more consistent? Or maybe stick with the "CustomScan at the end"
pattern, and put the "Instrumentation" before it in all the callbacks?

That's all I have for now. Thanks for the patch, sorry about the delay.

regards

--
Tomas Vondra