| From: | Heikki Linnakangas <hlinnaka(at)iki(dot)fi> |
|---|---|
| To: | Andres Freund <andres(at)anarazel(dot)de> |
| Cc: | pgsql-hackers(at)postgresql(dot)org, Amit Langote <amitlangote09(at)gmail(dot)com>, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>, David Rowley <dgrowleyml(at)gmail(dot)com> |
| Subject: | Re: unnecessary executor overheads around seqscans |
| Date: | 2026-01-27 10:16:15 |
| Message-ID: | 0dc565fa-bcc0-4600-9cef-263822b08132@iki.fi |
| Views: | Whole Thread | Raw Message | Download mbox | Resend email |
| Thread: | |
| Lists: | pgsql-hackers |
On 27/01/2026 01:25, Andres Freund wrote:
> On 2026-01-26 17:23:16 +0200, Heikki Linnakangas wrote:
>> Perhaps we should turn the ExecScanExtended() function inside out. Instead
>> of passing SeqNext as a callback to ExecScanExtended(), we would have a
>> function like this (for illustration purposes only, doesn't compile):
>
> That would be one approach, would require structural changes in a fair number
> of places though :/.
ExecScanExtended is only used in execScan.c and nodeSeqScan.c, so not
that many places. Even replacing ExecScan() completely would be isolated
to src/backend/executor.
> A slightly simpler approach could be for ExecScanExtended to pass in these
> parameters as arguments to the callbacks. For things like estate, direction
> and scanslot, that makes plenty sense. It's a bit more problematic for the
> scan descriptor, due to the "lazy start" we have in a few places.
Yep, it's less flexible. We have to know beforehand which variables the
function might want to avoid re-fetching, and add them all as parameters.
> I very briefly prototyped that (relying on the fact that all callers cast to
> the callback type and that passing unused arguments just works even if the
> function definition doesn't expect them), and that seems to do the trick.
This can be a very hot codepath so if we can shave a few % off, I'm
willing to hold my nose. But if we can do it more elegantly, even better...
> What shows up more visibly afterwards is that we set ExecScanExtended() sets
> econtext->ecxt_scantuple in every iteration, despite that not changing in
> almost all cases (I think for FDWs it could, fdwhandler.sgml just says that
> the scanslot "should" be used).
Huh, that's just a single store instruction. This really is a hot path.
Or is it just that the first instruction after the function call causes
some kind of a stall or data dependency, i.e. if that was removed, it'd
just move to the next instruction instead?
I wonder about the access to (econtext)->ecxt_per_tuple_memory. That
never changes, but we're re-fetching that too on every iteration.
InstrCountFiltered1() also fetches node->instrument, with a NULL check,
on every iteration. Maybe keep a counter in a local variable and only
call InstrCountFiltered1() when exiting the loop.
I guess these don't show up in a profiler or you would've latched on
them already..
- Heikki
| From | Date | Subject | |
|---|---|---|---|
| Next Message | Joel Jacobson | 2026-01-27 10:23:43 | Re: Time to add FIDO2 support? |
| Previous Message | Joel Jacobson | 2026-01-27 09:35:36 | Re: Time to add FIDO2 support? |