Re: unnecessary executor overheads around seqscans

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(at)google(dot)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.

> @@ -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.

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).

Greetings,

Andres Freund