From 715aa13f123860e1b9cdc353966ff489dfd3594d Mon Sep 17 00:00:00 2001
From: huyajun <hu_yajun@qq.com>
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

