pg.ddx.io pgsql-bugs@postgresql.org mailing list archive
help / color / mirror / Atom feedBUG #18087: pg_visibility: false positives in pg_check_visible
2+ messages / 2 participants
[nested] [flat]
* BUG #18087: pg_visibility: false positives in pg_check_visible
@ 2023-09-05 13:08 PG Bug reporting form <noreply@postgresql.org>
0 siblings, 1 reply; 2+ messages in thread
From: PG Bug reporting form @ 2023-09-05 13:08 UTC (permalink / raw)
To: pgsql-bugs@lists.postgresql.org; +Cc: 1026592243@qq.com
The following bug has been logged on the website:
Bug reference: 18087
Logged by: yajun Hu
Email address: 1026592243@qq.com
PostgreSQL version: 11.21
Operating system: CentOS7 with kernel version 5.10
Description:
hi, community partners.
I found a false positives when use pg_check_visible, I build postgresql
with REL_11_STABLE, run this SQL
select pg_create_logical_replication_slot ('test','pgoutput');
create table t( a int);
insert into t select 1;
insert into t select 1;
insert into t select 1;
vacuum t;
select * from pg_check_visible('t'); -- output ctid, which is false
positives
I analyze this problem, guess in collect_corrupt_items, GetOldestXmin use
rel = NULL
to get alldbs oldestXmin which will influenced by replication slot and other
database
long transaction. But Vacuum get relation's database oldestXmin.
I will put my fix patch behind
Regrads, Yajun Hu
^ permalink raw reply [nested|flat] 2+ messages in thread
* Re: BUG #18087: pg_visibility: false positives in pg_check_visible
@ 2023-09-05 13:12 huyajun <hu_yajun@qq.com>
parent: PG Bug reporting form <noreply@postgresql.org>
0 siblings, 0 replies; 2+ messages in thread
From: huyajun @ 2023-09-05 13:12 UTC (permalink / raw)
To: 我自己的邮箱 <1026592243@qq.com>; pgsql-bugs@lists.postgresql.org
> 2023年9月5日 下午9:08，PG Bug reporting form <noreply@postgresql.org> 写道：
>
> The following bug has been logged on the website:
>
> Bug reference: 18087
> Logged by: yajun Hu
> Email address: 1026592243@qq.com
> PostgreSQL version: 11.21
> Operating system: CentOS7 with kernel version 5.10
> Description:
>
> hi, community partners.
> I found a false positives when use pg_check_visible, I build postgresql
> with REL_11_STABLE, run this SQL
> select pg_create_logical_replication_slot ('test','pgoutput');
> create table t( a int);
> insert into t select 1;
> insert into t select 1;
> insert into t select 1;
> vacuum t;
> select * from pg_check_visible('t'); -- output ctid, which is false
> positives
>
> I analyze this problem, guess in collect_corrupt_items, GetOldestXmin use
> rel = NULL
> to get alldbs oldestXmin which will influenced by replication slot and other
> database
> long transaction. But Vacuum get relation's database oldestXmin.
>
> I will put my fix patch behind
>
> Regrads, Yajun Hu
>
Here are my fix patch, thinks.=
Attachments:
0001-pg_visibility-Fix-wrong-calculation-of-OldestXmin.patch (application/octet-stream, 5.6K) download
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
=
end of thread, other threads:[~2023-09-05 13:12 UTC | newest]
Thread overview: 2+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2023-09-05 13:08 BUG #18087: pg_visibility: false positives in pg_check_visible PG Bug reporting form <noreply@postgresql.org>
2023-09-05 13:12 ` huyajun <hu_yajun@qq.com>
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox