Hi,
On 2026-01-24 19:36:08 +1300, David Rowley wrote:
> On Sat, 24 Jan 2026 at 19:21, Amit Langote <[email protected]> wrote:
> > On Sat, Jan 24, 2026 at 5:16 AM Andres Freund <[email protected]> wrote:
> > > Or perhaps we could just make it so that the entire if (scandesc ==
> > > NULL)
> > > branch isn't needed?
> >
> > Kind of like ExecProcNodeFirst(), what if we replace the variant
> > selection in ExecInitSeqScan() with just:
>
> I imagined moving it to ExecInitSeqScan() and just avoid doing it when
> we're doing EXPLAIN or we're doing a parallel scan. Something like the
> attached, which is giving me a 4% speedup selecting from a million row
> table with a single int column running a seqscan query with a WHERE
> clause matching no rows.
Yes, i think that'd be better. Nice!
> > > We should change ExecStoreBufferHeapTuple() to return true. Nobody uses
> > > the
> > > current return value. Alternatively we should consider just moving it to
> > > somewhere heapam.c/heapam_handler.c can see the implementations,
> > > they're the
> > > only ones that should use it anyway.
> >
> > Makes sense. Changing ExecStoreBufferHeapTuple() to return true seems
> > like the simpler option, unless I misunderstood.
>
> It's probably too late to change it now, but wouldn't it have been
> better if scan_getnextslot had been coded to return the TupleTableSlot
> rather than bool? That way you could get the sibling call in
> ExecStoreBufferHeapTuple() and in SeqNext().
TupIsNull() is actually more expensive than a bool return value, because it
needs a memory fetch to complete... Except of course if we end up doing both.
SeqNext() shouldn't need a sibling call because it ought to be inlined. But:
> I also noticed my compiler does not inline SeqNext(). Adding a
> pg_attribute_always_inline results in it getting inlined and gives a
> small speedup.
Oh,m that's not good. I think we really had assumed that it would with the 18
changes around this. It does here, but that's probably because I use -O3.
> diff --git a/src/backend/executor/nodeSeqscan.c
> b/src/backend/executor/nodeSeqscan.c
> index b8119face43..87420e60dc9 100644
> --- a/src/backend/executor/nodeSeqscan.c
> +++ b/src/backend/executor/nodeSeqscan.c
> @@ -63,17 +63,6 @@ SeqNext(SeqScanState *node)
> direction = estate->es_direction;
> slot = node->ss.ss_ScanTupleSlot;
>
> - if (scandesc == NULL)
> - {
> - /*
> - * We reach here if the scan is not parallel, or if we're
> serially
> - * executing a scan that was planned to be parallel.
> - */
> - scandesc = table_beginscan(node->ss.ss_currentRelation,
> -
> estate->es_snapshot,
> - 0, NULL);
> - node->ss.ss_currentScanDesc = scandesc;
> - }
What about the "if we're serially executing a scan that was planned to be
parallel." part? I frankly don't really know what precisely was referencing...
Greetings,
Andres Freund
