Subject: collect_corrupt_items_vacuum.patch

From: Daniel Shelepanov <[email-masked]>
Date: Mon, 04 Apr 2022 11:51:10 +0300

Hi hackers!
 
I’ve been working on this [ https://www.postgresql.org/message-id/flat/cfcca574-6967-c5ab-7dc3-2c82b6723b99%40mail.ru ] bug. Finally, I’ve come up with the patch you can find attached. Basically what is does is rises a PROC_IN_VACUUM flag and resets it afterwards. I know this seems kinda crunchy and I hope you guys will give me some hints on where to continue. This [ https://www.postgresql.org/message-id/20220218175119.7hwv7ksamfjwijbx%40alap3.anarazel.de ] message contains reproduction script. Thank you very much in advance.
 
Kind regards,
Daniel Shelepanov

diff --git a/contrib/pg_visibility/pg_visibility.c b/contrib/pg_visibility/pg_visibility.c
index 53aaf61cb94..700ebeaa018 100644
--- a/contrib/pg_visibility/pg_visibility.c
+++ b/contrib/pg_visibility/pg_visibility.c
@@ -18,6 +18,7 @@
 #include "funcapi.h"
 #include "miscadmin.h"
 #include "storage/bufmgr.h"
+#include "storage/proc.h"
 #include "storage/procarray.h"
 #include "storage/smgr.h"
 #include "utils/rel.h"
@@ -562,12 +563,19 @@ collect_corrupt_items(Oid relid, bool all_visible, bool all_frozen)
 	Buffer		vmbuffer = InvalidBuffer;
 	BufferAccessStrategy bstrategy = GetAccessStrategy(BAS_BULKREAD);
 	TransactionId OldestXmin = InvalidTransactionId;
+	uint8		oldStatusFlags;
 
 	rel = relation_open(relid, AccessShareLock);
 
 	/* Only some relkinds have a visibility map */
 	check_relation_relkind(rel);
 
+	LWLockAcquire(ProcArrayLock, LW_EXCLUSIVE);
+	oldStatusFlags = MyProc->statusFlags;
+	MyProc->statusFlags |= PROC_IN_VACUUM;
+	ProcGlobal->statusFlags[MyProc->pgxactoff] = MyProc->statusFlags;
+	LWLockRelease(ProcArrayLock);
+
 	if (all_visible)
 		OldestXmin = GetOldestNonRemovableTransactionId(rel);
 
@@ -722,6 +730,11 @@ collect_corrupt_items(Oid relid, bool all_visible, bool all_frozen)
 	items->count = items->next;
 	items->next = 0;
 
+	LWLockAcquire(ProcArrayLock, LW_EXCLUSIVE);
+	MyProc->statusFlags = oldStatusFlags;
+	ProcGlobal->statusFlags[MyProc->pgxactoff] = MyProc->statusFlags;
+	LWLockRelease(ProcArrayLock);
+
 	return items;
 }

---

From: Robert Haas <[email-masked]>
Date: Wed, 27 Jul 2022 17:50:46 -0400

On Mon, Apr 4, 2022 at 4:51 AM Daniel Shelepanov <[email-masked]> wrote:
> I’ve been working on this [https://www.postgresql.org/message-id/flat/cfcca574-6967-c5ab-7dc3-2c82b6723b99%40mail.ru] bug. Finally, I’ve come up with the patch you can find attached. Basically what is does is rises a PROC_IN_VACUUM flag and resets it afterwards. I know this seems kinda crunchy and I hope you guys will give me some hints on where to continue. This [https://www.postgresql.org/message-id/20220218175119.7hwv7ksamfjwijbx%40alap3.anarazel.de] message contains reproduction script. Thank you very much in advance.

I noticed the CommitFest entry for this thread today and decided to
take a look. I think the general issue here can be stated in this way:
suppose a VACUUM computes an all-visible cutoff X, i.e. it thinks all
committed XIDs < X are all-visible. Then, at a later time, pg_visible
computes an all-visible cutoff Y, i.e. it thinks all committed XIDs <
Y are all-visible. If Y < X, pg_check_visible() might falsely report
corruption, because VACUUM might have marked as all-visible some page
containing tuples which pg_check_visibile() thinks aren't really
all-visible.

In reality, the oldest all-visible XID cannot move backward, but
ComputeXidHorizons() lets it move backward, because it's intended for
use by a caller who wants to mark pages all-visible, and it's only
concerned with making sure that the value is old enough to be safe.
And that's a problem for the way that pg_visibility is (mis-)using it.

To say that another way, ComputeXidHorizons() is perfectly fine with
returning a value that is older than the true answer, as long as it
never returns a value that is newer than the new answer. pg_visibility
wants the opposite. Here, a value that is newer than the true value
can't do worse than hide corruption, which is sort of OK, but a value
that's older than the true value can report corruption where none
exists, which is very bad.

I have a feeling, therefore, that this isn't really a complete fix. I
think it might address one way for the horizon reported by
ComputeXidHorizons() to move backward, but not all the ways.

Unfortunately, I am out of time for today to study this... but will
try to find more time on another day.

-- 
Robert Haas
EDB: http://www.enterprisedb.com

---

From: Tom Lane <[email-masked]>
Date: Wed, 27 Jul 2022 17:55:56 -0400

> concerned with making sure that the value is old enough to be safe.

Right.

> exists, which is very bad.

Maybe we need a different function for pg_visibility to call?
If we want ComputeXidHorizons to serve both these purposes, then it
has to always deliver exactly the right answer, which seems like
a definition that will be hard and expensive to achieve.

			regards, tom lane

---

From: Robert Haas <[email-masked]>
Date: Wed, 27 Jul 2022 21:47:19 -0400

> a definition that will be hard and expensive to achieve.

Yeah, I was thinking along similar lines.

I'm also kind of wondering why these calculations use
latestCompletedXid. Is that something we do solely to reduce locking?
The XIDs of running transactions matter, and their snapshots matter,
and the XIDs that could start running in the future matter, but I
don't know why it matters what the latest completed XID is.

---

From: Michael Paquier <[email-masked]>
Date: Wed, 12 Oct 2022 14:14:59 +0900

> don't know why it matters what the latest completed XID is.

Daniel, it seems to me that this thread is waiting for some input from
you, based on the remarks of Tom and Robert.  Are you planning to do
so?  This is marked as a bug fix, so I have moved this item to the
next CF for now.
--
Michael

---

From: Nikita Malakhov <[email-masked]>
Date: Mon, 7 Nov 2022 16:30:32 +0300

Hi hackers!

Daniel is busy with other tasks. I've found this topic and this problem
seems to be actual
or v15 too.
Please correct me if I am wrong. I've checked another discussion related to
pg_visibility [1].
According to discussion: if using latest completed xid is not right for
checking visibility, than
it should be the least running transaction xid? So it must be another
function to be used for
these calculations, not the GetOldestNonRemovableTransactionId that uses
the ComputeXidHorizons.

[1]
https://www.postgresql.org/message-id/flat/c0610352-8433-ab4b-986d-0e803c628efe%40postgrespro.ru

-- 
Regards,
Nikita Malakhov
Postgres Professional
https://postgrespro.ru/

---

From: Nikita Malakhov <[email-masked]>
Date: Wed, 14 Dec 2022 21:56:12 +0300

Hi hackers!

Just to bump this thread, because the problem seems to be still actual:

[1]

-- 
Regards,
Nikita Malakhov
Postgres Professional
https://postgrespro.ru/

---

From: Daniel Gustafsson <[email-masked]>
Date: Tue, 4 Jul 2023 09:21:02 +0200

This patch has been waiting on the author for about a year now, so I will close
it as Returned with Feedback.  Plesae feel free to resubmit to a future CF when
there is renewed interest in working on this.

--
Daniel Gustafsson

---

From: Alexander Korotkov <[email-masked]>
Date: Mon, 6 Nov 2023 11:30:15 +0200

Hi!

> there is renewed interest in working on this.

I'd like to revive this thread.  While the patch proposed definitely
makes things better.  But as pointed out by Robert and Tom, it didn't
allow to avoid all false reports.  The reason is that the way we
currently calculate the oldest xmin, it could move backwards (see
comments to ComputeXidHorizons()).  The attached patch implements own
function to calculate strict oldest xmin, which should be always
greater or equal to any xid horizon calculated before.  I have to do
the following changes in comparison to what ComputeXidHorizons() do.

1. Ignore processes xmin's, because they take into account connection
to other databases which were ignored before.
2. Ignore KnownAssignedXids, because they are not database-aware.
While primary could compute its horizons database-aware.
3. Ignore walsender xmin, because it could go backward if some
replication connections don't use replication slots.

Surely these would significantly sacrifice accuracy. But we have to do
so in order to avoid reporting false errors.

Any thoughts?

------
Regards,
Alexander Korotkov

---

From: Alexander Lakhin <[email-masked]>
Date: Tue, 7 Nov 2023 14:00:00 +0300

Hi Alexander,

> so in order to avoid reporting false errors.

I've reduced the dirty reproducer Daniel Shelepanov posted initially
to the following:
numdbs=10
for ((d=1;d<=$numdbs;d++)); do
   createdb db$d
   psql db$d -c "create extension pg_visibility"
done

for ((i=1;i<=300;i++)); do
echo "iteration $i"
for ((d=1;d<=$numdbs;d++)); do
(
echo "
create table vacuum_test as select 42 i;
vacuum (disable_page_skipping) vacuum_test;
select * from pg_check_visible('vacuum_test');
" | psql db$d -a -q >psql-$d.log 2>&1
) &
done
wait

res=0
for ((d=1;d<=$numdbs;d++)); do
grep -q '0 rows' psql-$d.log || { echo "Error condition in psql-$d.log:"; cat psql-$d.log; res=1; break; }
psql db$d -q -c "drop table vacuum_test"
done
[ $res == 0 ] || break;
done

It looks like the v2 patch doesn't fix the original issue. Maybe I miss
something, but with the patch applied, I see the failure immediately,
though without the patch several iterations are needed to get it.

Best regards,
Alexander

---

From: Alexander Korotkov <[email-masked]>
Date: Tue, 7 Nov 2023 13:38:40 +0200

Hi, Alexander.

> though without the patch several iterations are needed to get it.

That's a bug in the patch.  Thank you for cathing it.  It should start
calculation from latestCompletedXid + 1, not InvalidTransactionId.
Please, check the revised patch.

------
Regards,
Alexander Korotkov

---

From: Alexander Lakhin <[email-masked]>
Date: Tue, 7 Nov 2023 16:00:00 +0300

> Please, check the revised patch.

Thanks for looking at this!
Unfortunately, I still see the failure with the v3, but not on a first
iteration:
...
iteration 316
Error condition in psql-8.log:
create table vacuum_test as select 42 i;
vacuum (disable_page_skipping) vacuum_test;
select * from pg_check_visible('vacuum_test');
  t_ctid
--------
  (0,1)
(1 row)

(I've double-checked that the patch is applied and get_strict_xid_horizon()
is called.)

Best regards,
Alexander

---

From: Alexander Korotkov <[email-masked]>
Date: Mon, 4 Dec 2023 02:23:37 +0200

Hi, Alexander!

> (I've double-checked that the patch is applied and get_strict_xid_horizon()

I managed to reproduce this on a Linux VM.  This problem should arise
because in extension I don't have access to ProcArrayStruct.  So, my
code is iterating the whole PGPROC's array.  I reimplemented the new
horizon calculation function in the core with usage of
ProcArrayStruct.  Now it doesn't fall for me.  Please, recheck.

------
Regards,
Alexander Korotkov

---

From: Alexander Lakhin <[email-masked]>
Date: Mon, 4 Dec 2023 13:00:00 +0300

Hi Alexander,

> ProcArrayStruct.  Now it doesn't fall for me.  Please, recheck.

Yes, v4 works for me as well (thousands of iterations passed).
Thank you!

Though the test passes even without manipulations with the PROC_IN_VACUUM
flag in pg_visibility.c (maybe the test is not good enough to show why
those manipulations are needed).
I also couldn't see where VISHORIZON_DATA_STRICT comes into play...

Best regards,
Alexander