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 <[email protected]> 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 <[email protected]> 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 > >
