Re: [PATCH] Add RetrieveInstrumentation hook for CustomScan providers

Hi folks,

Gentle nudge. 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 16, 2026 at 11:33 AM Siddharth Kothari <sidkot(at)google(dot)com> wrote:
>
> 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
> > >