Re: [PATCH] Add RetrieveInstrumentation hook for CustomScan providers

Hi Tomas,

Just sending a gentle ping on this thread. Please let me know if you
get a chance to take a look at the v3 patch, or if there is anything
else you'd like me to address!

Thanks,
Siddharth

On Thu, Jul 9, 2026 at 1:37 PM Siddharth Kothari <sidkot(at)google(dot)com> wrote:
>
> 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(at)vondra(dot)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
> >