Re: [PATCH] Add RetrieveInstrumentation hook for CustomScan providers - Mailing list pgsql-hackers

Hi Tomas,

Thanks for the review, and no worries.
I completely agree with both of your suggestions.

I added the new missing callbacks:

  ExecCustomScanInstrumentEstimate
  ExecCustomScanInstrumentInitDSM
  ExecCustomScanInstrumentInitWorker
along with the existing
  ExecCustomScanRetrieveInstrumentation

and extracted the custom scan instrumentation tracking out of the
parallel_aware guard block in execParallel.c. The core execution
orchestrator will now correctly invoke these instrumentation callbacks
even when a CustomScan node is part of a parallel chunk but not
strictly parallel_aware itself.

I also updated the Custom scan documentation to properly document the
new callbacks.

I've attached the v3 patch with these changes included. Let me know if
everything looks good or if there is anything else I should adjust.

Regards,
Siddharth



On Thu, Jul 9, 2026 at 12:10 AM Tomas Vondra <tomas@vondra.me> wrote:
>
> Hi Siddharth,
>
> On 7/8/26 18:52, Siddharth Kothari wrote:
> > Hi hackers,
> >
> > Adding Melanie and Tomas to this thread. I noticed you both recently
> > committed similar changes to handle shared memory instrumentation
> > separation and retrieval for IndexScan/IndexOnlyScan (dd78e69cfc
> > <https://github.com/postgres/postgres/commit/
> > dd78e69cfc337f93cfc0303ddf660262d7f1237e>) and SeqScan (3b1117d6e2
> > <https://github.com/postgres/postgres/
> > commit/3b1117d6e2e47d86cdbd978b79434c630cb0ef52>).
> >
> > My patch implements the exact same pattern for CustomScan states. It
> > adds an optional RetrieveInstrumentationCustomScan hook so that
> > extensions can aggregate their worker metrics before the DSM gets
> > unlinked, bringing custom scans to parity with the recent core scan
> > instrumentation improvements.
> >
> > Could you please help review?
> >
> > The CommitFest entry is also updated here: https://
> > commitfest.postgresql.org/patch/6524/ <https://
> > commitfest.postgresql.org/patch/6524/>
> >
>
> Thanks for the patch, and sorry for not responding earlier. The last
> couple months were incredibly busy, both because of the last commitfest
> and personal reasons.
>
> I think the patch looks generally OK, except for two things:
>
> 1) It needs to add the new callback to doc/src/sgml/custom-scan.sgml,
> with similar documentation as for the other callbacks.
>
> 2) While looking at execParallel.c it occurred to me the CustomScan may
> have the same issue as described in [1] (and a couple messages after
> that). The scan may in the parallel part of a plan, but not necessarily
> parallel-aware. But a lot of the initialization is gated by
>
>     if (planstate->plan->parallel_aware)
>         ...
>
> All the other scans initialize some of the instrumentation always, in
> blocks like this:
>
>     /* even when not parallel-aware, for EXPLAIN ANALYZE */
>
> And BHS had this issue until 9c18b47e610, but we didn't do the same
> thing for CustomScan. I guess we should.
>
> Of course, it's not the fault of this patch, it's just something I
> noticed while looking at the code. OTOH it probably also makes sense for
> CustomScan to handle the instrumentation just like the other scans, i.e.
> by allocating a separate DSM for the instrumentation. AFAICS that'll
> require a couple more optional callbacks in CustomExecMethods:
>
>   ExecCustomScanInstrumentEstimate
>   ExecCustomScanInstrumentInitDSM
>   ExecCustomScanInstrumentInitWorker
>
> But I think that's OK.
>
>
> [1]
> https://www.postgresql.org/message-id/3bdbc70d-ad44-494a-8aab-868b5066fe8b%40vondra.me
>
>
> regards
>
> --
> Tomas Vondra
>