From 715aa13f123860e1b9cdc353966ff489dfd3594d Mon Sep 17 00:00:00 2001 From: huyajun Date: Tue, 5 Sep 2023 17:25:33 +0800 Subject: [PATCH] pg_visibility: Fix wrong calculation of OldestXmin Vacuum get rel's OldestXmin but pg_visibility not. 1. other db procArray will impact calculation 2. replication slot will impact calculation So OldestXmin may be smaller than vacuum calculation which cound lead to false positives --- contrib/pg_visibility/Makefile | 4 ++++ contrib/pg_visibility/expected/pg_visibility.out | 12 ++++++++++++ contrib/pg_visibility/pg_visibility.c | 15 +++++++++++---- contrib/pg_visibility/pg_visibility.conf | 1 + contrib/pg_visibility/sql/pg_visibility.sql | 3 +++ 5 files changed, 31 insertions(+), 4 deletions(-) create mode 100644 contrib/pg_visibility/pg_visibility.conf diff --git a/contrib/pg_visibility/Makefile b/contrib/pg_visibility/Makefile index 21d787ddf7..366160a40e 100644 --- a/contrib/pg_visibility/Makefile +++ b/contrib/pg_visibility/Makefile @@ -8,7 +8,11 @@ DATA = pg_visibility--1.1.sql pg_visibility--1.1--1.2.sql \ pg_visibility--1.0--1.1.sql PGFILEDESC = "pg_visibility - page visibility information" +REGRESS_OPTS = --temp-config $(top_srcdir)/contrib/pg_visibility/pg_visibility.conf REGRESS = pg_visibility +# Disabled because these tests require "wal_level=logical", +# which typical installcheck users do not have. +NO_INSTALLCHECK = 1 ifdef USE_PGXS PG_CONFIG = pg_config diff --git a/contrib/pg_visibility/expected/pg_visibility.out b/contrib/pg_visibility/expected/pg_visibility.out index cc1edeba73..4acec4e004 100644 --- a/contrib/pg_visibility/expected/pg_visibility.out +++ b/contrib/pg_visibility/expected/pg_visibility.out @@ -66,6 +66,13 @@ select pg_check_frozen('test_foreign_table'); ERROR: "test_foreign_table" is not a table, materialized view, or TOAST table select pg_truncate_visibility_map('test_foreign_table'); ERROR: "test_foreign_table" is not a table, materialized view, or TOAST table +-- create logical slot to check get correct OldestXmin. +SELECT 'create_logical_slot' FROM pg_create_logical_replication_slot('pg_visibility_test','pgoutput'); + ?column? +--------------------- + create_logical_slot +(1 row) + -- check some of the allowed relkinds create table regular_table (a int); insert into regular_table values (1), (2); @@ -125,6 +132,11 @@ select * from pg_check_frozen('test_partition'); -- hopefully none -------- (0 rows) +select * from pg_check_visible('test_partition'); -- hopefully none + t_ctid +-------- +(0 rows) + select pg_truncate_visibility_map('test_partition'); pg_truncate_visibility_map ---------------------------- diff --git a/contrib/pg_visibility/pg_visibility.c b/contrib/pg_visibility/pg_visibility.c index 8009bb381c..0add768bcc 100644 --- a/contrib/pg_visibility/pg_visibility.c +++ b/contrib/pg_visibility/pg_visibility.c @@ -554,13 +554,17 @@ collect_corrupt_items(Oid relid, bool all_visible, bool all_frozen) BufferAccessStrategy bstrategy = GetAccessStrategy(BAS_BULKREAD); TransactionId OldestXmin = InvalidTransactionId; + rel = relation_open(relid, AccessShareLock); if (all_visible) { - /* Don't pass rel; that will fail in recovery. */ - OldestXmin = GetOldestXmin(NULL, PROCARRAY_FLAGS_VACUUM); + /* Don't pass rel when in recovery, that will fail. */ + if (RecoveryInProgress()) + OldestXmin = GetOldestXmin(NULL, PROCARRAY_FLAGS_VACUUM); + /* Pass rel when in primary to avoid returning spurious results. */ + else + OldestXmin = GetOldestXmin(rel, PROCARRAY_FLAGS_VACUUM); } - rel = relation_open(relid, AccessShareLock); /* Only some relkinds have a visibility map */ check_relation_relkind(rel); @@ -674,7 +678,10 @@ collect_corrupt_items(Oid relid, bool all_visible, bool all_frozen) * a buffer lock. And this shouldn't happen often, so it's * worth being careful so as to avoid false positives. */ - RecomputedOldestXmin = GetOldestXmin(NULL, PROCARRAY_FLAGS_VACUUM); + if (RecoveryInProgress()) + RecomputedOldestXmin = GetOldestXmin(NULL, PROCARRAY_FLAGS_VACUUM); + else + RecomputedOldestXmin = GetOldestXmin(rel, PROCARRAY_FLAGS_VACUUM); if (!TransactionIdPrecedes(OldestXmin, RecomputedOldestXmin)) record_corrupt_item(items, &tuple.t_self); diff --git a/contrib/pg_visibility/pg_visibility.conf b/contrib/pg_visibility/pg_visibility.conf new file mode 100644 index 0000000000..e3d257315f --- /dev/null +++ b/contrib/pg_visibility/pg_visibility.conf @@ -0,0 +1 @@ +wal_level = logical diff --git a/contrib/pg_visibility/sql/pg_visibility.sql b/contrib/pg_visibility/sql/pg_visibility.sql index 6f4c9a42f9..a32c438f9d 100644 --- a/contrib/pg_visibility/sql/pg_visibility.sql +++ b/contrib/pg_visibility/sql/pg_visibility.sql @@ -48,6 +48,8 @@ select pg_visibility_map_summary('test_foreign_table'); select pg_check_frozen('test_foreign_table'); select pg_truncate_visibility_map('test_foreign_table'); +-- create logical slot to check get correct OldestXmin. +SELECT 'create_logical_slot' FROM pg_create_logical_replication_slot('pg_visibility_test','pgoutput'); -- check some of the allowed relkinds create table regular_table (a int); insert into regular_table values (1), (2); @@ -70,6 +72,7 @@ select count(*) > 0 from pg_visibility('test_partition', 0); select count(*) > 0 from pg_visibility_map('test_partition'); select count(*) > 0 from pg_visibility_map_summary('test_partition'); select * from pg_check_frozen('test_partition'); -- hopefully none +select * from pg_check_visible('test_partition'); -- hopefully none select pg_truncate_visibility_map('test_partition'); -- cleanup -- 2.41.0