Re: unnecessary executor overheads around seqscans

On Mon, Jan 26, 2026 at 5:27 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>
> On Sat, Jan 24, 2026 at 9:01 PM Andres Freund <andres(at)anarazel(dot)de> wrote:
> >
> > On 2026-01-24 15:23:44 +0530, Amit Kapila wrote:
> > > On Sat, Jan 24, 2026 at 1:46 AM Andres Freund <andres(at)anarazel(dot)de> wrote:
> > > >
> > > > - The checkXidAlive checks that have been added to table_scan_getnextslot()
> > > > show up noticeably and in every loop iteration, despite afaict never being reachable
> > > >
> > > > It's not obvious to me that this should
> > > > a) be in table_scan_getnextslot(), rather than in beginscan - how could it
> > > > change in the middle of a scan? That would require a wrapper around
> > > > rd_tableam->scan_begin(), but that seems like it might be good anyway.
> > > > b) not just be an assertion?
> > > >
> > >
> > > IIRC, the main reason for having this precautionary check in the API
> > > is to ensure that during logical decoding we never access the table AM
> > > or
> > > heap APIs directly when scanning catalog tables. This restriction
> > > exists because we only check for concurrent aborts inside the
> > > systable_* APIs.
> >
> > 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.

--
Regards,
Dilip Kumar
Google