Re: unnecessary executor overheads around seqscans - Mailing list pgsql-hackers

On Wed, Jan 28, 2026 at 11:22 PM Andres Freund <andres@anarazel.de> wrote:
>
> Hi,
>
> Thanks for the updated patch! I found one issue below.  Unless somebody sees a
> reason not to, I'm planning to apply this after that is fixed.
>
>
> On 2026-01-28 09:53:48 +0530, Dilip Kumar wrote:
> > From 5347d920fe590ad3b250624d9cb50cc685ccd6d9 Mon Sep 17 00:00:00 2001
> > From: Dilip Kumar <dilipkumarb@google.com>
> > Date: Tue, 27 Jan 2026 16:20:59 +0530
> > Subject: [PATCH v2] Refactor: Move CheckXidAlive check to table_beginscan for
> >  better performance
> >
> > Previously, the CheckXidAlive validation was performed within each table_scan_*
> > function. This caused the check to be executed repeatedly for every tuple
> > fetched, creating unnecessary overhead.
> >
> > Move the check to table_beginscan* so it is performed once per scan rather than
> > once per row.
> >
> > Author: Dilip Kumar
> > Reported-by: Andres Freund
> > Suggested-by: Andres Freund, Amit Kapila
> > ---
>
> Very minor suggestion for the future, to make it easier for committers: Our
> project style these days is to include email addresses, as used by the people
> on the thread, in these tags, and to only include one person per tag,
> instead repeating the tag to represent multiple people.

Okay, noted, I have changed it.

>
> > @@ -1219,14 +1235,6 @@ table_index_fetch_tuple(struct IndexFetchTableData *scan,
> >                                               TupleTableSlot *slot,
> >                                               bool *call_again, bool *all_dead)
> >  {
> > -     /*
> > -      * We don't expect direct calls to table_index_fetch_tuple 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_index_fetch_tuple call during logical decoding");
> > -
> >       return scan->rel->rd_tableam->index_fetch_tuple(scan, tid, snapshot,
> >                                                                                                       slot,
call_again,
> >                                                                                                       all_dead);
> > @@ -1265,14 +1273,6 @@ table_tuple_fetch_row_version(Relation rel,
> >                                                         Snapshot snapshot,
> >                                                         TupleTableSlot *slot)
> >  {
> > -     /*
> > -      * We don't expect direct calls to table_tuple_fetch_row_version 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_tuple_fetch_row_version call during logical decoding");
> > -
> >       return rel->rd_tableam->tuple_fetch_row_version(rel, tid, snapshot, slot);
> >  }
> >
>
> These two cases aren't covered by the CheckXidAlive check in
> table_scan_begin_impl(), as they don't use TableScanDesc.
>
> table_tuple_fetch_row_version() doesn't use a scan, so we probably can't get
> rid of the check - I think that's ok, the callers seem unlikely to be
> bottlenecked by the test.

Yeah, I missed this, thanks for pointing this out, fixed.

> For table_index_fetch_tuple(), the check should be moved to
> table_index_fetch_begin(). That's worth doing, as table_index_fetch_tuple()
> can be performance critical (e.g. in an ordered index scan).

Yeah we can do that, fixed.


--
Regards,
Dilip Kumar
Google