From 598b466e80d9f46aa1472d4f1adb1df90be8c260 Mon Sep 17 00:00:00 2001 From: Fujii Masao Date: Fri, 7 Feb 2020 22:00:21 +0900 Subject: [PATCH] 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 --- src/backend/executor/nodeTidscan.c | 5 ++--- src/backend/utils/adt/tid.c | 4 ++-- src/include/access/tableam.h | 24 +++++++++++++++++++----- src/test/regress/expected/tidscan.out | 16 ++++++++++++++++ src/test/regress/sql/tidscan.sql | 7 +++++++ 5 files changed, 46 insertions(+), 10 deletions(-) diff --git a/src/backend/executor/nodeTidscan.c b/src/backend/executor/nodeTidscan.c index 83ece6bf563..f53cb00d7bf 100644 --- 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 039ddc86a85..2bba09feaad 100644 --- 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 3d73d9ac85b..b5a175dffa6 100644 --- 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 9b5eb04bfd9..13c3c360c25 100644 --- 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 ef05c098420..313e0fb9b67 100644 --- 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;