From 15f5714a67002de2b4960671a235522013dddc23 Mon Sep 17 00:00:00 2001 From: Dilip Kumar Date: Thu, 29 Jan 2026 14:24:14 +0530 Subject: [PATCH v3] 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. This moves the check to table_beginscan* so it is performed once per scan rather than once per row. Note: The table_tuple_fetch_row_version() do not use the scan descriptor; therefore, the CheckXidAlive check is retained in that function. Reported-by: Andres Freund Author: Dilip Kumar Suggested-by: Andres Freund Suggested-by: Amit Kapila Discussion: https://www.postgresql.org/message-id/tlpltqm5jjwj7mp66dtebwwhppe4ri36vdypux2zoczrc2i3mp%40dhv4v4nikyfg --- src/backend/access/heap/heapam.c | 10 ---- src/backend/access/index/genam.c | 30 +++++------ src/backend/access/table/tableam.c | 18 ++----- src/include/access/tableam.h | 86 ++++++++++++++---------------- 4 files changed, 60 insertions(+), 84 deletions(-) diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.c index f30a56ecf55..ae31efe8c25 100644 --- a/src/backend/access/heap/heapam.c +++ b/src/backend/access/heap/heapam.c @@ -1421,16 +1421,6 @@ heap_getnext(TableScanDesc sscan, ScanDirection direction) (errcode(ERRCODE_FEATURE_NOT_SUPPORTED), errmsg_internal("only heap AM is supported"))); - /* - * We don't expect direct calls to heap_getnext with valid CheckXidAlive - * for catalog or regular tables. See detailed comments in xact.c where - * these variables are declared. Normally we have such a check at tableam - * level API but this is called from many places so we need to ensure it - * here. - */ - if (unlikely(TransactionIdIsValid(CheckXidAlive) && !bsysscan)) - elog(ERROR, "unexpected heap_getnext call during logical decoding"); - /* Note: no locking manipulations needed */ if (scan->rs_base.rs_flags & SO_ALLOW_PAGEMODE) diff --git a/src/backend/access/index/genam.c b/src/backend/access/index/genam.c index a29be6f467b..5e89b86a62c 100644 --- a/src/backend/access/index/genam.c +++ b/src/backend/access/index/genam.c @@ -420,6 +420,14 @@ systable_beginscan(Relation heapRelation, sysscan->snapshot = NULL; } + /* + * If CheckXidAlive is set then set a flag to indicate that system table + * scan is in-progress. See detailed comments in xact.c where these + * variables are declared. + */ + if (TransactionIdIsValid(CheckXidAlive)) + bsysscan = true; + if (irel) { int i; @@ -468,14 +476,6 @@ systable_beginscan(Relation heapRelation, sysscan->iscan = NULL; } - /* - * If CheckXidAlive is set then set a flag to indicate that system table - * scan is in-progress. See detailed comments in xact.c where these - * variables are declared. - */ - if (TransactionIdIsValid(CheckXidAlive)) - bsysscan = true; - return sysscan; } @@ -707,13 +707,6 @@ systable_beginscan_ordered(Relation heapRelation, elog(ERROR, "column is not in index"); } - sysscan->iscan = index_beginscan(heapRelation, indexRelation, - snapshot, NULL, nkeys, 0); - index_rescan(sysscan->iscan, idxkey, nkeys, NULL, 0); - sysscan->scan = NULL; - - pfree(idxkey); - /* * If CheckXidAlive is set then set a flag to indicate that system table * scan is in-progress. See detailed comments in xact.c where these @@ -722,6 +715,13 @@ systable_beginscan_ordered(Relation heapRelation, if (TransactionIdIsValid(CheckXidAlive)) bsysscan = true; + sysscan->iscan = index_beginscan(heapRelation, indexRelation, + snapshot, NULL, nkeys, 0); + index_rescan(sysscan->iscan, idxkey, nkeys, NULL, 0); + sysscan->scan = NULL; + + pfree(idxkey); + return sysscan; } diff --git a/src/backend/access/table/tableam.c b/src/backend/access/table/tableam.c index 87491796523..1334566c47d 100644 --- a/src/backend/access/table/tableam.c +++ b/src/backend/access/table/tableam.c @@ -117,8 +117,8 @@ table_beginscan_catalog(Relation relation, int nkeys, ScanKeyData *key) Oid relid = RelationGetRelid(relation); Snapshot snapshot = RegisterSnapshot(GetCatalogSnapshot(relid)); - return relation->rd_tableam->scan_begin(relation, snapshot, nkeys, key, - NULL, flags); + return table_scan_begin_impl(relation, snapshot, nkeys, key, + NULL, flags); } @@ -184,7 +184,7 @@ table_beginscan_parallel(Relation relation, ParallelTableScanDesc pscan) snapshot = SnapshotAny; } - return relation->rd_tableam->scan_begin(relation, snapshot, 0, NULL, + return table_scan_begin_impl(relation, snapshot, 0, NULL, pscan, flags); } @@ -214,8 +214,8 @@ table_beginscan_parallel_tidrange(Relation relation, snapshot = SnapshotAny; } - sscan = relation->rd_tableam->scan_begin(relation, snapshot, 0, NULL, - pscan, flags); + sscan = table_scan_begin_impl(relation, snapshot, 0, NULL, + pscan, flags); return sscan; } @@ -269,14 +269,6 @@ table_tuple_get_latest_tid(TableScanDesc scan, ItemPointer tid) Relation rel = scan->rs_rd; const TableAmRoutine *tableam = rel->rd_tableam; - /* - * We don't expect direct calls to table_tuple_get_latest_tid 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_get_latest_tid call during logical decoding"); - /* * Since this can be called with user-supplied TID, don't trust the input * too much. diff --git a/src/include/access/tableam.h b/src/include/access/tableam.h index e2ec5289d4d..c7b96b03827 100644 --- a/src/include/access/tableam.h +++ b/src/include/access/tableam.h @@ -868,6 +868,31 @@ extern TupleTableSlot *table_slot_create(Relation relation, List **reglist); * ---------------------------------------------------------------------------- */ +/* + * table_scan_begin_impl + * + * 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. + */ +static TableScanDesc +table_scan_begin_impl(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_impl call during logical decoding"); + + return rel->rd_tableam->scan_begin(rel, snapshot, nkeys, key, pscan, flags); +} + /* * Start a scan of `rel`. Returned tuples pass a visibility test of * `snapshot`, and if nkeys != 0, the results are filtered by those scan keys. @@ -879,7 +904,7 @@ table_beginscan(Relation rel, Snapshot snapshot, uint32 flags = SO_TYPE_SEQSCAN | SO_ALLOW_STRAT | SO_ALLOW_SYNC | SO_ALLOW_PAGEMODE; - return rel->rd_tableam->scan_begin(rel, snapshot, nkeys, key, NULL, flags); + return table_scan_begin_impl(rel, snapshot, nkeys, key, NULL, flags); } /* @@ -908,7 +933,7 @@ table_beginscan_strat(Relation rel, Snapshot snapshot, if (allow_sync) flags |= SO_ALLOW_SYNC; - return rel->rd_tableam->scan_begin(rel, snapshot, nkeys, key, NULL, flags); + return table_scan_begin_impl(rel, snapshot, nkeys, key, NULL, flags); } /* @@ -923,8 +948,7 @@ table_beginscan_bm(Relation rel, Snapshot snapshot, { uint32 flags = SO_TYPE_BITMAPSCAN | SO_ALLOW_PAGEMODE; - return rel->rd_tableam->scan_begin(rel, snapshot, nkeys, key, - NULL, flags); + return table_scan_begin_impl(rel, snapshot, nkeys, key, NULL, flags); } /* @@ -949,7 +973,7 @@ table_beginscan_sampling(Relation rel, Snapshot snapshot, if (allow_pagemode) flags |= SO_ALLOW_PAGEMODE; - return rel->rd_tableam->scan_begin(rel, snapshot, nkeys, key, NULL, flags); + return table_scan_begin_impl(rel, snapshot, nkeys, key, NULL, flags); } /* @@ -962,7 +986,7 @@ table_beginscan_tid(Relation rel, Snapshot snapshot) { uint32 flags = SO_TYPE_TIDSCAN; - return rel->rd_tableam->scan_begin(rel, snapshot, 0, NULL, NULL, flags); + return table_scan_begin_impl(rel, snapshot, 0, NULL, NULL, flags); } /* @@ -975,7 +999,7 @@ table_beginscan_analyze(Relation rel) { uint32 flags = SO_TYPE_ANALYZE; - return rel->rd_tableam->scan_begin(rel, NULL, 0, NULL, NULL, flags); + return table_scan_begin_impl(rel, NULL, 0, NULL, NULL, flags); } /* @@ -1025,14 +1049,6 @@ table_scan_getnextslot(TableScanDesc sscan, ScanDirection direction, TupleTableS Assert(direction == ForwardScanDirection || direction == BackwardScanDirection); - /* - * We don't expect direct calls to table_scan_getnextslot 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_getnextslot call during logical decoding"); - return sscan->rs_rd->rd_tableam->scan_getnextslot(sscan, direction, slot); } @@ -1053,7 +1069,7 @@ table_beginscan_tidrange(Relation rel, Snapshot snapshot, TableScanDesc sscan; uint32 flags = SO_TYPE_TIDRANGESCAN | SO_ALLOW_PAGEMODE; - sscan = rel->rd_tableam->scan_begin(rel, snapshot, 0, NULL, NULL, flags); + sscan = table_scan_begin_impl(rel, snapshot, 0, NULL, NULL, flags); /* Set the range of TIDs to scan */ sscan->rs_rd->rd_tableam->scan_set_tidrange(sscan, mintid, maxtid); @@ -1166,6 +1182,14 @@ table_parallelscan_reinitialize(Relation rel, ParallelTableScanDesc pscan) static inline IndexFetchTableData * table_index_fetch_begin(Relation rel) { + /* + * 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_index_fetch_begin call during logical decoding"); + return rel->rd_tableam->index_fetch_begin(rel); } @@ -1219,14 +1243,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); @@ -1947,14 +1963,6 @@ table_scan_bitmap_next_tuple(TableScanDesc scan, uint64 *lossy_pages, uint64 *exact_pages) { - /* - * We don't expect direct calls to table_scan_bitmap_next_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_scan_bitmap_next_tuple call during logical decoding"); - return scan->rs_rd->rd_tableam->scan_bitmap_next_tuple(scan, slot, recheck, @@ -1975,13 +1983,6 @@ static inline bool table_scan_sample_next_block(TableScanDesc scan, SampleScanState *scanstate) { - /* - * We don't expect direct calls to table_scan_sample_next_block 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_sample_next_block call during logical decoding"); return scan->rs_rd->rd_tableam->scan_sample_next_block(scan, scanstate); } @@ -1998,13 +1999,6 @@ table_scan_sample_next_tuple(TableScanDesc scan, SampleScanState *scanstate, TupleTableSlot *slot) { - /* - * We don't expect direct calls to table_scan_sample_next_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_scan_sample_next_tuple call during logical decoding"); return scan->rs_rd->rd_tableam->scan_sample_next_tuple(scan, scanstate, slot); } -- 2.49.0