tableam: Avoid relying on relation size to determine validity of tids.

Instead add a tableam callback to do so. To avoid adding per
validation overhead, pass a scan to tuple_tid_valid. In heap's case
we'd otherwise incurred a RelationGetNumberOfBlocks() call for each
tid - which'd have added noticable overhead to nodeTidscan.c.

Author: Andres Freund
Reviewed-By: Ashwin Agrawal
Discussion: https://postgr.es/m/20190515185447.gno2jtqxyktylyvs@alap3.anarazel.de

diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.c
index ec9853603fde49736e16ea3bdef691efca4d24b0..d8d4f3b1f5a64c755b45289a8380c81e0ed78388 100644 (file)
--- a/src/backend/access/heap/heapam.c
+++ b/src/backend/access/heap/heapam.c
@@ -1654,8 +1654,8 @@ heap_hot_search_buffer(ItemPointer tid, Relation relation, Buffer buffer,
 /*
  * heap_get_latest_tid -  get the latest tid of a specified tuple
  *
- * Actually, this gets the latest version that is visible according to
- * the passed snapshot.  You can pass SnapshotDirty to get the very latest,
+ * Actually, this gets the latest version that is visible according to the
+ * scan's snapshot.  Create a scan using SnapshotDirty to get the very latest,
  * possibly uncommitted version.
  *
  * *tid is both an input and an output parameter: it is updated to
@@ -1663,28 +1663,20 @@ heap_hot_search_buffer(ItemPointer tid, Relation relation, Buffer buffer,
  * if no version of the row passes the snapshot test.
  */
 void
-heap_get_latest_tid(Relation relation,
-                   Snapshot snapshot,
+heap_get_latest_tid(TableScanDesc sscan,
                    ItemPointer tid)
 {
-   BlockNumber blk;
+   Relation relation = sscan->rs_rd;
+   Snapshot snapshot = sscan->rs_snapshot;
    ItemPointerData ctid;
    TransactionId priorXmax;
 
-   /* this is to avoid Assert failures on bad input */
-   if (!ItemPointerIsValid(tid))
-       return;
-
    /*
-    * Since this can be called with user-supplied TID, don't trust the input
-    * too much.  (RelationGetNumberOfBlocks is an expensive check, so we
-    * don't check t_ctid links again this way.  Note that it would not do to
-    * call it just once and save the result, either.)
+    * table_get_latest_tid verified that the passed in tid is valid.  Assume
+    * that t_ctid links are valid however - there shouldn't be invalid ones
+    * in the table.
     */
-   blk = ItemPointerGetBlockNumber(tid);
-   if (blk >= RelationGetNumberOfBlocks(relation))
-       elog(ERROR, "block number %u is out of range for relation \"%s\"",
-            blk, RelationGetRelationName(relation));
+   Assert(ItemPointerIsValid(tid));
 
    /*
     * Loop to chase down t_ctid links.  At top of loop, ctid is the tuple we

diff --git a/src/backend/access/heap/heapam_handler.c b/src/backend/access/heap/heapam_handler.c
index 9aa468295ae9246e2e0c43fb45314a67ff9b0833..35553c7c92d93dee63b93520cac90d12eff95e39 100644 (file)
--- a/src/backend/access/heap/heapam_handler.c
+++ b/src/backend/access/heap/heapam_handler.c
@@ -204,6 +204,15 @@ heapam_fetch_row_version(Relation relation,
    return false;
 }
 
+static bool
+heapam_tuple_tid_valid(TableScanDesc scan, ItemPointer tid)
+{
+   HeapScanDesc hscan = (HeapScanDesc) scan;
+
+   return ItemPointerIsValid(tid) &&
+       ItemPointerGetBlockNumber(tid) < hscan->rs_nblocks;
+}
+
 static bool
 heapam_tuple_satisfies_snapshot(Relation rel, TupleTableSlot *slot,
                                Snapshot snapshot)
@@ -2568,6 +2577,7 @@ static const TableAmRoutine heapam_methods = {
 
    .tuple_fetch_row_version = heapam_fetch_row_version,
    .tuple_get_latest_tid = heap_get_latest_tid,
+   .tuple_tid_valid = heapam_tuple_tid_valid,
    .tuple_satisfies_snapshot = heapam_tuple_satisfies_snapshot,
    .compute_xid_horizon_for_tuples = heap_compute_xid_horizon_for_tuples,
 

diff --git a/src/backend/access/table/tableam.c b/src/backend/access/table/tableam.c
index baba1ea699bfca830901763f5cf4dc851aef7b7f..6e46befdfd9a85ef40313d788a9f9ab302cc6ad5 100644 (file)
--- a/src/backend/access/table/tableam.c
+++ b/src/backend/access/table/tableam.c
@@ -213,6 +213,33 @@ table_index_fetch_tuple_check(Relation rel,
 }
 
 
+/* ------------------------------------------------------------------------
+ * Functions for non-modifying operations on individual tuples
+ * ------------------------------------------------------------------------
+ */
+
+void
+table_get_latest_tid(TableScanDesc scan, ItemPointer tid)
+{
+   Relation rel = scan->rs_rd;
+   const TableAmRoutine *tableam = rel->rd_tableam;
+
+   /*
+    * Since this can be called with user-supplied TID, don't trust the input
+    * too much.
+    */
+   if (!tableam->tuple_tid_valid(scan, tid))
+       ereport(ERROR,
+               (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+                errmsg("tid (%u, %u) is not valid for relation for relation \"%s\"",
+                       ItemPointerGetBlockNumberNoCheck(tid),
+                       ItemPointerGetOffsetNumberNoCheck(tid),
+                       RelationGetRelationName(rel))));
+
+   return tableam->tuple_get_latest_tid(scan, tid);
+}
+
+
 /* ----------------------------------------------------------------------------
  * Functions to make modifications a bit simpler.
  * ----------------------------------------------------------------------------

diff --git a/src/backend/executor/nodeTidscan.c b/src/backend/executor/nodeTidscan.c
index 156be56a57d3637a3f3ef79a386a7a4d6ea27975..93335864a1ec18f0162452628ecbf6580a0f4979 100644 (file)
--- a/src/backend/executor/nodeTidscan.c
+++ b/src/backend/executor/nodeTidscan.c
@@ -129,19 +129,23 @@ static void
 TidListEval(TidScanState *tidstate)
 {
    ExprContext *econtext = tidstate->ss.ps.ps_ExprContext;
-   BlockNumber nblocks;
+   TableScanDesc scan;
    ItemPointerData *tidList;
    int         numAllocTids;
    int         numTids;
    ListCell   *l;
 
    /*
-    * We silently discard any TIDs that are out of range at the time of scan
-    * start.  (Since we hold at least AccessShareLock on the table, it won't
-    * be possible for someone to truncate away the blocks we intend to
-    * visit.)
+    * Start scan on-demand - initializing a scan isn't free (e.g. heap stats
+    * the size of the table), so it makes sense to delay that until needed -
+    * the node might never get executed.
     */
-   nblocks = RelationGetNumberOfBlocks(tidstate->ss.ss_currentRelation);
+   if (tidstate->ss.ss_currentScanDesc == NULL)
+       tidstate->ss.ss_currentScanDesc =
+           table_beginscan(tidstate->ss.ss_currentRelation,
+                           tidstate->ss.ps.state->es_snapshot,
+                           0, NULL);
+   scan = tidstate->ss.ss_currentScanDesc;
 
    /*
     * We initialize the array with enough slots for the case that all quals
@@ -165,19 +169,27 @@ TidListEval(TidScanState *tidstate)
                DatumGetPointer(ExecEvalExprSwitchContext(tidexpr->exprstate,
                                                          econtext,
                                                          &isNull));
-           if (!isNull &&
-               ItemPointerIsValid(itemptr) &&
-               ItemPointerGetBlockNumber(itemptr) < nblocks)
+           if (isNull)
+               continue;
+
+           /*
+            * We silently discard any TIDs that the AM considers invalid
+            * (E.g. for heap, they could be out of range at the time of scan
+            * start.  Since we hold at least AccessShareLock on the table, it
+            * won't be possible for someone to truncate away the blocks we
+            * intend to visit.).
+            */
+           if (!table_tuple_tid_valid(scan, itemptr))
+               continue;
+
+           if (numTids >= numAllocTids)
            {
-               if (numTids >= numAllocTids)
-               {
-                   numAllocTids *= 2;
-                   tidList = (ItemPointerData *)
-                       repalloc(tidList,
-                                numAllocTids * sizeof(ItemPointerData));
-               }
-               tidList[numTids++] = *itemptr;
+               numAllocTids *= 2;
+               tidList = (ItemPointerData *)
+                   repalloc(tidList,
+                            numAllocTids * sizeof(ItemPointerData));
            }
+           tidList[numTids++] = *itemptr;
        }
        else if (tidexpr->exprstate && tidexpr->isarray)
        {
@@ -206,13 +218,15 @@ TidListEval(TidScanState *tidstate)
            }
            for (i = 0; i < ndatums; i++)
            {
-               if (!ipnulls[i])
-               {
-                   itemptr = (ItemPointer) DatumGetPointer(ipdatums[i]);
-                   if (ItemPointerIsValid(itemptr) &&
-                       ItemPointerGetBlockNumber(itemptr) < nblocks)
-                       tidList[numTids++] = *itemptr;
-               }
+               if (ipnulls[i])
+                   continue;
+
+               itemptr = (ItemPointer) DatumGetPointer(ipdatums[i]);
+
+               if (!table_tuple_tid_valid(scan, itemptr))
+                   continue;
+
+               tidList[numTids++] = *itemptr;
            }
            pfree(ipdatums);
            pfree(ipnulls);
@@ -306,6 +320,7 @@ TidNext(TidScanState *node)
    EState     *estate;
    ScanDirection direction;
    Snapshot    snapshot;
+   TableScanDesc scan;
    Relation    heapRelation;
    TupleTableSlot *slot;
    ItemPointerData *tidList;
@@ -327,6 +342,7 @@ TidNext(TidScanState *node)
    if (node->tss_TidList == NULL)
        TidListEval(node);
 
+   scan = node->ss.ss_currentScanDesc;
    tidList = node->tss_TidList;
    numTids = node->tss_NumTids;
 
@@ -365,7 +381,7 @@ TidNext(TidScanState *node)
         * current according to our snapshot.
         */
        if (node->tss_isCurrentOf)
-           table_get_latest_tid(heapRelation, snapshot, &tid);
+           table_get_latest_tid(scan, &tid);
 
        if (table_fetch_row_version(heapRelation, &tid, snapshot, slot))
            return slot;
@@ -442,6 +458,10 @@ ExecReScanTidScan(TidScanState *node)
    node->tss_NumTids = 0;
    node->tss_TidPtr = -1;
 
+   /* not really necessary, but seems good form */
+   if (node->ss.ss_currentScanDesc)
+       table_rescan(node->ss.ss_currentScanDesc, NULL);
+
    ExecScanReScan(&node->ss);
 }
 
@@ -455,6 +475,9 @@ ExecReScanTidScan(TidScanState *node)
 void
 ExecEndTidScan(TidScanState *node)
 {
+   if (node->ss.ss_currentScanDesc)
+       table_endscan(node->ss.ss_currentScanDesc);
+
    /*
     * Free the exprcontext
     */

diff --git a/src/backend/utils/adt/tid.c b/src/backend/utils/adt/tid.c
index 6ab26d8ea8b4c4b0a570b11a59e924a45e949659..1aab30b6aab175919c6e9dfdd0a19a06209e8a77 100644 (file)
--- a/src/backend/utils/adt/tid.c
+++ b/src/backend/utils/adt/tid.c
@@ -358,6 +358,7 @@ currtid_byreloid(PG_FUNCTION_ARGS)
    Relation    rel;
    AclResult   aclresult;
    Snapshot    snapshot;
+   TableScanDesc scan;
 
    result = (ItemPointer) palloc(sizeof(ItemPointerData));
    if (!reloid)
@@ -380,7 +381,9 @@ currtid_byreloid(PG_FUNCTION_ARGS)
    ItemPointerCopy(tid, result);
 
    snapshot = RegisterSnapshot(GetLatestSnapshot());
-   table_get_latest_tid(rel, snapshot, result);
+   scan = table_beginscan(rel, snapshot, 0, NULL);
+   table_get_latest_tid(scan, result);
+   table_endscan(scan);
    UnregisterSnapshot(snapshot);
 
    table_close(rel, AccessShareLock);
@@ -398,6 +401,7 @@ currtid_byrelname(PG_FUNCTION_ARGS)
    Relation    rel;
    AclResult   aclresult;
    Snapshot    snapshot;
+   TableScanDesc scan;
 
    relrv = makeRangeVarFromNameList(textToQualifiedNameList(relname));
    rel = table_openrv(relrv, AccessShareLock);
@@ -415,7 +419,9 @@ currtid_byrelname(PG_FUNCTION_ARGS)
    ItemPointerCopy(tid, result);
 
    snapshot = RegisterSnapshot(GetLatestSnapshot());
-   table_get_latest_tid(rel, snapshot, result);
+   scan = table_beginscan(rel, snapshot, 0, NULL);
+   table_get_latest_tid(scan, result);
+   table_endscan(scan);
    UnregisterSnapshot(snapshot);
 
    table_close(rel, AccessShareLock);

diff --git a/src/include/access/heapam.h b/src/include/access/heapam.h
index 77e5e603b032e4644fb3271f5202461288fe4ccb..6b8c7020c8c3abdff48c6eeda663fc65c67617a6 100644 (file)
--- a/src/include/access/heapam.h
+++ b/src/include/access/heapam.h
@@ -134,8 +134,7 @@ extern bool heap_hot_search_buffer(ItemPointer tid, Relation relation,
                       Buffer buffer, Snapshot snapshot, HeapTuple heapTuple,
                       bool *all_dead, bool first_call);
 
-extern void heap_get_latest_tid(Relation relation, Snapshot snapshot,
-                   ItemPointer tid);
+extern void heap_get_latest_tid(TableScanDesc scan, ItemPointer tid);
 extern void setLastTid(const ItemPointer tid);
 
 extern BulkInsertState GetBulkInsertState(void);

diff --git a/src/include/access/tableam.h b/src/include/access/tableam.h
index c5d64602036a31eb121f8ad9de61492c98a4c1fb..8fbeb020337f2b2248caa09693efe861a515886f 100644 (file)
--- a/src/include/access/tableam.h
+++ b/src/include/access/tableam.h
@@ -308,12 +308,17 @@ typedef struct TableAmRoutine
                                            Snapshot snapshot,
                                            TupleTableSlot *slot);
 
+   /*
+    * Is tid valid for a scan of this relation.
+    */
+   bool        (*tuple_tid_valid) (TableScanDesc scan,
+                                   ItemPointer tid);
+
    /*
     * Return the latest version of the tuple at `tid`, by updating `tid` to
     * point at the newest version.
     */
-   void        (*tuple_get_latest_tid) (Relation rel,
-                                        Snapshot snapshot,
+   void        (*tuple_get_latest_tid) (TableScanDesc scan,
                                         ItemPointer tid);
 
    /*
@@ -548,10 +553,10 @@ typedef struct TableAmRoutine
    /*
     * See table_relation_size().
     *
-    * Note that currently a few callers use the MAIN_FORKNUM size to vet the
-    * validity of tids (e.g. nodeTidscans.c), and others use it to figure out
-    * the range of potentially interesting blocks (brin, analyze). The
-    * abstraction around this will need to be improved in the near future.
+    * Note that currently a few callers use the MAIN_FORKNUM size to figure
+    * out the range of potentially interesting blocks (brin, analyze). It's
+    * probable that we'll need to revise the interface for those at some
+    * point.
     */
    uint64      (*relation_size) (Relation rel, ForkNumber forkNumber);
 
@@ -986,15 +991,25 @@ table_fetch_row_version(Relation rel,
 }
 
 /*
- * Return the latest version of the tuple at `tid`, by updating `tid` to
- * point at the newest version.
+ * Verify that `tid` is a potentially valid tuple identifier. That doesn't
+ * mean that the pointed to row needs to exist or be visible, but that
+ * attempting to fetch the row (e.g. with table_get_latest_tid() or
+ * table_fetch_row_version()) should not error out if called with that tid.
+ *
+ * `scan` needs to have been started via table_beginscan().
  */
-static inline void
-table_get_latest_tid(Relation rel, Snapshot snapshot, ItemPointer tid)
+static inline bool
+table_tuple_tid_valid(TableScanDesc scan, ItemPointer tid)
 {
-   rel->rd_tableam->tuple_get_latest_tid(rel, snapshot, tid);
+   return scan->rs_rd->rd_tableam->tuple_tid_valid(scan, tid);
 }
 
+/*
+ * Return the latest version of the tuple at `tid`, by updating `tid` to
+ * point at the newest version.
+ */
+extern void table_get_latest_tid(TableScanDesc scan, ItemPointer tid);
+
 /*
  * Return true iff tuple in slot satisfies the snapshot.
  *