From: Andres Freund Date: Thu, 29 Jan 2026 22:27:23 +0000 (-0500) Subject: tableam: Perform CheckXidAlive check once per scan X-Git-Url: http://git.ipfire.org/gitweb.cgi?a=commitdiff_plain;h=87f7b824f20c1c06884ef0711b4d32dbf4461436;p=thirdparty%2Fpostgresql.git tableam: Perform CheckXidAlive check once per scan Previously, the CheckXidAlive check was performed within the table_scan*next* functions. This caused the check to be executed for every fetched tuple, an unnecessary overhead. To fix, move the check to table_beginscan* so it is performed once per scan rather than once per row. Note: table_tuple_fetch_row_version() does not use a scan descriptor; therefore, the CheckXidAlive check is retained in that function. The overhead is unlikely to be relevant for the existing callers. Reported-by: Andres Freund Author: Dilip Kumar Suggested-by: Andres Freund Suggested-by: Amit Kapila Reviewed-by: Andres Freund Discussion: https://www.postgresql.org/message-id/tlpltqm5jjwj7mp66dtebwwhppe4ri36vdypux2zoczrc2i3mp%40dhv4v4nikyfg --- 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..dfda1af412e 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_beginscan_common(relation, snapshot, nkeys, key, + NULL, flags); } @@ -184,8 +184,8 @@ table_beginscan_parallel(Relation relation, ParallelTableScanDesc pscan) snapshot = SnapshotAny; } - return relation->rd_tableam->scan_begin(relation, snapshot, 0, NULL, - pscan, flags); + return table_beginscan_common(relation, snapshot, 0, NULL, + pscan, flags); } TableScanDesc @@ -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_beginscan_common(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..7260b7b3d52 100644 --- a/src/include/access/tableam.h +++ b/src/include/access/tableam.h @@ -868,6 +868,27 @@ extern TupleTableSlot *table_slot_create(Relation relation, List **reglist); * ---------------------------------------------------------------------------- */ +/* + * A wrapper around the Table Access Method scan_begin callback, to centralize + * error checking. All calls to ->scan_begin() should go through this + * function. + */ +static TableScanDesc +table_beginscan_common(Relation rel, Snapshot snapshot, int nkeys, + ScanKeyData *key, ParallelTableScanDesc pscan, + uint32 flags) +{ + /* + * We don't allow scans to be started while CheckXidAlive is set, except + * via systable_beginscan() et al. See detailed comments in xact.c where + * these variables are declared. + */ + if (unlikely(TransactionIdIsValid(CheckXidAlive) && !bsysscan)) + elog(ERROR, "scan started 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 +900,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_beginscan_common(rel, snapshot, nkeys, key, NULL, flags); } /* @@ -908,7 +929,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_beginscan_common(rel, snapshot, nkeys, key, NULL, flags); } /* @@ -923,8 +944,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_beginscan_common(rel, snapshot, nkeys, key, NULL, flags); } /* @@ -949,7 +969,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_beginscan_common(rel, snapshot, nkeys, key, NULL, flags); } /* @@ -962,7 +982,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_beginscan_common(rel, snapshot, 0, NULL, NULL, flags); } /* @@ -975,7 +995,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_beginscan_common(rel, NULL, 0, NULL, NULL, flags); } /* @@ -1025,14 +1045,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 +1065,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_beginscan_common(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 +1178,14 @@ table_parallelscan_reinitialize(Relation rel, ParallelTableScanDesc pscan) static inline IndexFetchTableData * table_index_fetch_begin(Relation rel) { + /* + * We don't allow scans to be started while CheckXidAlive is set, except + * via systable_beginscan() et al. See detailed comments in xact.c where + * these variables are declared. + */ + if (unlikely(TransactionIdIsValid(CheckXidAlive) && !bsysscan)) + elog(ERROR, "scan started during logical decoding"); + return rel->rd_tableam->index_fetch_begin(rel); } @@ -1219,14 +1239,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 +1959,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 +1979,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 +1995,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); }