Fix bug in Tid scan.

Commit 147e3722f7 changed Tid scan so that it calls table_beginscan()
and uses the scan option for seq scan. This change caused two issues.

(1) The change caused Tid scan to take a predicate lock on the entire
       relation in serializable transaction even when relation-level
       lock is not necessary. This could lead to an unexpected
       serialization error.

(2) The change caused Tid scan to increment the number of seq_scan
       in pg_stat_*_tables views even though it's not seq scan. This
       could confuse the users.

This commit adds the scan option for Tid scan and makes Tid scan
use it, to avoid those issues.

Back-patch to v12, where the bug was introduced.

Author: Tatsuhito Kasahara
Reviewed-by: Kyotaro Horiguchi, Masahiko Sawada, Fujii Masao
Discussion: https://postgr.es/m/CAP0=ZVKy+gTbFmB6X_UW0pP3WaeJ-fkUWHoD-pExS=at3CY76g@mail.gmail.com

diff --git a/src/backend/executor/nodeTidscan.c b/src/backend/executor/nodeTidscan.c
index 83ece6bf563e71b6e1911fedd921b08130fe5205..f53cb00d7bfdfdf2cfb6fbce3f617379608ef457 100644 (file)
--- a/src/backend/executor/nodeTidscan.c
+++ b/src/backend/executor/nodeTidscan.c
@@ -142,9 +142,8 @@ TidListEval(TidScanState *tidstate)
     */
    if (tidstate->ss.ss_currentScanDesc == NULL)
        tidstate->ss.ss_currentScanDesc =
-           table_beginscan(tidstate->ss.ss_currentRelation,
-                           tidstate->ss.ps.state->es_snapshot,
-                           0, NULL);
+           table_beginscan_tid(tidstate->ss.ss_currentRelation,
+                           tidstate->ss.ps.state->es_snapshot);
    scan = tidstate->ss.ss_currentScanDesc;
 
    /*

diff --git a/src/backend/utils/adt/tid.c b/src/backend/utils/adt/tid.c
index 039ddc86a857112410a7bcb4feb19614e3d08d19..2bba09feaad14eef7e8e658acc9ba6e7fe6b9231 100644 (file)
--- a/src/backend/utils/adt/tid.c
+++ b/src/backend/utils/adt/tid.c
@@ -381,7 +381,7 @@ currtid_byreloid(PG_FUNCTION_ARGS)
    ItemPointerCopy(tid, result);
 
    snapshot = RegisterSnapshot(GetLatestSnapshot());
-   scan = table_beginscan(rel, snapshot, 0, NULL);
+   scan = table_beginscan_tid(rel, snapshot);
    table_tuple_get_latest_tid(scan, result);
    table_endscan(scan);
    UnregisterSnapshot(snapshot);
@@ -419,7 +419,7 @@ currtid_byrelname(PG_FUNCTION_ARGS)
    ItemPointerCopy(tid, result);
 
    snapshot = RegisterSnapshot(GetLatestSnapshot());
-   scan = table_beginscan(rel, snapshot, 0, NULL);
+   scan = table_beginscan_tid(rel, snapshot);
    table_tuple_get_latest_tid(scan, result);
    table_endscan(scan);
    UnregisterSnapshot(snapshot);

diff --git a/src/include/access/tableam.h b/src/include/access/tableam.h
index 3d73d9ac85b292f17a39c1fdf78ee4066b598123..b5a175dffa6931c57d77cf2e7d0b934586430b7d 100644 (file)
--- a/src/include/access/tableam.h
+++ b/src/include/access/tableam.h
@@ -47,18 +47,19 @@ typedef enum ScanOptions
    SO_TYPE_SEQSCAN = 1 << 0,
    SO_TYPE_BITMAPSCAN = 1 << 1,
    SO_TYPE_SAMPLESCAN = 1 << 2,
-   SO_TYPE_ANALYZE = 1 << 3,
+   SO_TYPE_TIDSCAN = 1 << 3,
+   SO_TYPE_ANALYZE = 1 << 4,
 
    /* several of SO_ALLOW_* may be specified */
    /* allow or disallow use of access strategy */
-   SO_ALLOW_STRAT = 1 << 4,
+   SO_ALLOW_STRAT = 1 << 5,
    /* report location to syncscan logic? */
-   SO_ALLOW_SYNC = 1 << 5,
+   SO_ALLOW_SYNC = 1 << 6,
    /* verify visibility page-at-a-time? */
-   SO_ALLOW_PAGEMODE = 1 << 6,
+   SO_ALLOW_PAGEMODE = 1 << 7,
 
    /* unregister snapshot at scan end? */
-   SO_TEMP_SNAPSHOT = 1 << 7
+   SO_TEMP_SNAPSHOT = 1 << 8
 } ScanOptions;
 
 /*
@@ -811,6 +812,19 @@ table_beginscan_sampling(Relation rel, Snapshot snapshot,
    return rel->rd_tableam->scan_begin(rel, snapshot, nkeys, key, NULL, flags);
 }
 
+/*
+ * table_beginscan_tid is an alternative entry point for setting up a
+ * TableScanDesc for a Tid scan. As with bitmap scans, it's worth using
+ * the same data structure although the behavior is rather different.
+ */
+static inline TableScanDesc
+table_beginscan_tid(Relation rel, Snapshot snapshot)
+{
+   uint32      flags = SO_TYPE_TIDSCAN;
+
+   return rel->rd_tableam->scan_begin(rel, snapshot, 0, NULL, NULL, flags);
+}
+
 /*
  * table_beginscan_analyze is an alternative entry point for setting up a
  * TableScanDesc for an ANALYZE scan.  As with bitmap scans, it's worth using

diff --git a/src/test/regress/expected/tidscan.out b/src/test/regress/expected/tidscan.out
index 9b5eb04bfd9c11a038c12d7fe72dbdf0b6d0e46c..13c3c360c2576d3ff4a2a94c0098f9400b628d72 100644 (file)
--- a/src/test/regress/expected/tidscan.out
+++ b/src/test/regress/expected/tidscan.out
@@ -277,4 +277,20 @@ SELECT count(*) FROM tenk1 t1 JOIN tenk1 t2 ON t1.ctid = t2.ctid;
 (1 row)
 
 RESET enable_hashjoin;
+-- check predicate lock on CTID
+BEGIN ISOLATION LEVEL SERIALIZABLE;
+SELECT * FROM tidscan WHERE ctid = '(0,1)';
+ id 
+----
+  1
+(1 row)
+
+-- locktype should be 'tuple'
+SELECT locktype, mode FROM pg_locks WHERE pid = pg_backend_pid() AND mode = 'SIReadLock';
+ locktype |    mode    
+----------+------------
+ tuple    | SIReadLock
+(1 row)
+
+ROLLBACK;
 DROP TABLE tidscan;

diff --git a/src/test/regress/sql/tidscan.sql b/src/test/regress/sql/tidscan.sql
index ef05c0984207919626a14d9ab5581f76d73bca8c..313e0fb9b679a1eacaaab6937956b7e14f6d0597 100644 (file)
--- a/src/test/regress/sql/tidscan.sql
+++ b/src/test/regress/sql/tidscan.sql
@@ -94,4 +94,11 @@ SELECT count(*) FROM tenk1 t1 JOIN tenk1 t2 ON t1.ctid = t2.ctid;
 SELECT count(*) FROM tenk1 t1 JOIN tenk1 t2 ON t1.ctid = t2.ctid;
 RESET enable_hashjoin;
 
+-- check predicate lock on CTID
+BEGIN ISOLATION LEVEL SERIALIZABLE;
+SELECT * FROM tidscan WHERE ctid = '(0,1)';
+-- locktype should be 'tuple'
+SELECT locktype, mode FROM pg_locks WHERE pid = pg_backend_pid() AND mode = 'SIReadLock';
+ROLLBACK;
+
 DROP TABLE tidscan;