On Wed, Jan 28, 2026 at 11:22 PM Andres Freund <[email protected]> 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 <[email protected]> > > 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
v3-0001-Refactor-Move-CheckXidAlive-check-to-table_begins.patch
Description: Binary data
