Re: unnecessary executor overheads around seqscans

[Dilip Kumar]

On Wed, Jan 28, 2026 at 12:54 AM Andres Freund <and...@anarazel.de> wrote:
>
> Hi,
>
> On 2026-01-27 16:40:55 +0530, Dilip Kumar wrote:
> > > > I know why the check exists - but why does it have to be in
> > > > table_scan_getnextslot(), which is executed very frequently, rather than
> > > > table_beginscan*(), which is executed much less frequently.
> > > >
> > >
> > > I thought about this point and couldn't think of any reason why this
> > > check can't be in table_beginscan*(). I think your idea of having a
> > > wrapper around scan_begin() to handle this check is a good one.
> >
> > Here is the patch. I've used table_scan_begin_wrapper() to wrap the
> > scan_begin() callback for now. If you have a better naming preference
> > to avoid the 'wrapper' suffix, please let me know.
>
> I'd probably just go for _internal(), _impl() or such.

Thanks

>
> > diff --git a/src/backend/access/table/tableam.c 
> > b/src/backend/access/table/tableam.c
> > index 87491796523..15a92c052d3 100644
> > --- a/src/backend/access/table/tableam.c
> > +++ b/src/backend/access/table/tableam.c
> > [...]
> > +/*
> > + * table_scan_begin_wrapper
> > + *
> > + * A wrapper around the Table Access Method scan_begin callback. This 
> > handles
> > + * centralized error checking—specifically ensuring we aren't performing
> > + * table scan while CheckXidAlive is valid.  This state is reserved for
> > + * specific logical decoding operations where direct relation scanning is
> > + * prohibited.
> > + */
> > +TableScanDesc
> > +table_scan_begin_wrapper(Relation rel, Snapshot snapshot, int nkeys,
> > +                                              ScanKeyData *key, 
> > ParallelTableScanDesc pscan,
> > +                                              uint32 flags)
> > +{
> > +     /*
> > +      * We don't expect direct calls to this function with valid 
> > CheckXidAlive
> > +      * for catalog or regular tables.  See detailed comments in xact.c 
> > where
> > +      * these variables are declared.
> > +      */
> > +     if (unlikely(TransactionIdIsValid(CheckXidAlive) && !bsysscan))
> > +             elog(ERROR, "unexpected table_scan_begin_wrapper call during 
> > logical decoding");
> > +
> > +     return rel->rd_tableam->scan_begin(rel, snapshot, nkeys, key, pscan, 
> > flags);
> > +}
>
> Given that some of the callers are in inline functions in tableam.h, I would
> implement the wrapper there. I doubt it'll make a meaningful difference, but
> it doesn't seem worth "risking" that.

Yeah, it makes sense, changed.

-- 
Regards,
Dilip Kumar
Google

v2-0001-Refactor-Move-CheckXidAlive-check-to-table_begins.patch
Description: Binary data