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 <andres@anarazel.de>
Author: Dilip Kumar <dilipbalaut@gmail.com>
Suggested-by: Andres Freund <andres@anarazel.de>
Suggested-by: Amit Kapila <akapila@postgresql.org>
Reviewed-by: Andres Freund <andres@anarazel.de>
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 f30a56ecf5539778645d42b75740595321b66714..ae31efe8c255cbeb3390967af3b94a14a82262ee 100644 (file)
--- 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 a29be6f467be784a7ef0e17547456abf4eb4adcf..5e89b86a62c428d4fdb0b38fadb6ade5d2351abd 100644 (file)
--- 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 87491796523e894f8a7b4f27027ed25e057b1d88..dfda1af412ec3b21c09c751e94dbd2382c685fe8 100644 (file)
--- 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 e2ec5289d4da35c5436a73e2a365179be82159e8..7260b7b3d52d71fd449155ec2e0bf8428387ec22 100644 (file)
--- 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);
 }