Subject: Re: Removing more vacuumlazy.c special cases, relfrozenxid optimizations

From: Andres Freund <[email-masked]>
Date: Fri, 18 Feb 2022 14:11:17 -0800

Hi,

On 2022-02-18 13:09:45 -0800, Peter Geoghegan wrote:
> 0001 is tricky in the sense that there are a lot of fine details, and
> if you get any one of them wrong the result might be a subtle bug. For
> example, the heap_tuple_needs_freeze() code path is only used when we
> cannot get a cleanup lock, which is rare -- and some of the branches
> within the function are relatively rare themselves. The obvious
> concern is: What if some detail of how we track the new relfrozenxid
> value (and new relminmxid value) in this seldom-hit codepath is just
> wrong, in whatever way we didn't think of?

I think it'd be good to add a few isolationtest cases for the
can't-get-cleanup-lock paths. I think it shouldn't be hard using cursors. The
slightly harder part is verifying that VACUUM did something reasonable, but
that still should be doable?

Greetings,

Andres Freund

---

From: Peter Geoghegan <[email-masked]>
Date: Fri, 18 Feb 2022 17:00:47 -0800

> that still should be doable?

We could even just extend existing, related tests, from vacuum-reltuples.spec.

Another testing strategy occurs to me: we could stress-test the
implementation by simulating an environment where the no-cleanup-lock
path is hit an unusually large number of times, possibly a fixed
percentage of the time (like 1%, 5%), say by making vacuumlazy.c's
ConditionalLockBufferForCleanup() call return false randomly. Now that
we have lazy_scan_noprune for the no-cleanup-lock path (which is as
similar to the regular lazy_scan_prune path as possible), I wouldn't
expect this ConditionalLockBufferForCleanup() testing gizmo to be too
disruptive.

-- 
Peter Geoghegan

---

From: Peter Geoghegan <[email-masked]>
Date: Sat, 19 Feb 2022 15:08:41 -0800

> expect this ConditionalLockBufferForCleanup() testing gizmo to be too

I tried this out, using the attached patch. It was quite interesting,
even when run against HEAD. I think that I might have found a bug on
HEAD, though I'm not really sure.

If you modify the patch to simulate conditions under which
ConditionalLockBufferForCleanup() fails about 2% of the time, you get
much better coverage of lazy_scan_noprune/heap_tuple_needs_freeze,
without it being so aggressive as to make "make check-world" fail --
which is exactly what I expected. If you are much more aggressive
about it, and make it 50% instead (which you can get just by using the
patch as written), then some tests will fail, mostly for reasons that
aren't surprising or interesting (e.g. plan changes). This is also
what I'd have guessed would happen.

However, it gets more interesting. One thing that I did not expect to
happen at all also happened (with the current 50% rate of simulated
ConditionalLockBufferForCleanup() failure from the patch): if I run
"make check" from the pg_surgery directory, then the Postgres backend
gets stuck in an infinite loop inside lazy_scan_prune, which has been
a symptom of several tricky bugs in the past year (not every time, but
usually). Specifically, the VACUUM statement launched by the SQL
command "vacuum freeze htab2;" from the file
contrib/pg_surgery/sql/heap_surgery.sql, at line 54 leads to this
misbehavior.

This is a temp table, which is a choice made by the tests specifically
because they need to "use a temp table so that vacuum behavior doesn't
depend on global xmin". This is convenient way of avoiding spurious
regression tests failures (e.g. from autoanalyze), and relies on the
GlobalVisTempRels behavior established by Andres' 2020 bugfix commit
94bc27b5.

It's quite possible that this is nothing more than a bug in my
adversarial gizmo patch -- since I don't think that
ConditionalLockBufferForCleanup() can ever fail with a temp buffer
(though even that's not completely clear right now). Even if the
behavior that I saw does not indicate a bug on HEAD, it still seems
informative. At the very least, it wouldn't hurt to Assert() that the
target table isn't a temp table inside lazy_scan_noprune, documenting
our assumptions around temp tables and
ConditionalLockBufferForCleanup().

I haven't actually tried to debug the issue just yet, so take all this
with a grain of salt.

-- 
Peter Geoghegan
From 3f01281af3ba81b35777cb7d717f76e001fd3e10 Mon Sep 17 00:00:00 2001
From: Peter Geoghegan <[email-masked]>
Date: Sat, 19 Feb 2022 14:07:35 -0800
Subject: [PATCH] Add adversarial ConditionalLockBufferForCleanup() gizmo to
 vacuumlazy.c.

---
 src/backend/access/heap/vacuumlazy.c | 36 +++++++++++++++++++++++++++-
 1 file changed, 35 insertions(+), 1 deletion(-)

diff --git a/src/backend/access/heap/vacuumlazy.c b/src/backend/access/heap/vacuumlazy.c
index 242511a23..31c6b360e 100644
--- a/src/backend/access/heap/vacuumlazy.c
+++ b/src/backend/access/heap/vacuumlazy.c
@@ -50,6 +50,7 @@
 #include "commands/dbcommands.h"
 #include "commands/progress.h"
 #include "commands/vacuum.h"
+#include "common/pg_prng.h"
 #include "executor/instrument.h"
 #include "miscadmin.h"
 #include "optimizer/paths.h"
@@ -748,6 +749,39 @@ heap_vacuum_rel(Relation rel, VacuumParams *params,
 	}
 }
 
+/*
+ * Adversarial gizmo, simulates excessive failure to get cleanup locks
+ */
+static inline bool
+lazy_conditionallockbufferforcleanup(Buffer buffer)
+{
+	/*
+	 * Artificially fail to get a cleanup lock 50% of the time.
+	 *
+	 * XXX: What about temp tables?  We simulate not getting a cleanup lock
+	 * there, but is that choice actually reasonable?
+	 */
+	if (pg_prng_uint32(&pg_global_prng_state) <= (PG_UINT32_MAX / 2))
+		return false;
+
+#if 0
+	/*
+	 * 50% is very very aggressive, while 2% - 5% is still basically
+	 * adversarial but in many ways less annoying.
+	 *
+	 * This version (which injects a failure to get a cleanup lock 2% of the
+	 * time) seems to pass the regression tests, even with my parallel make
+	 * check-world recipe.  Expected query plans don't seem to shift on
+	 * account of unexpected index bloat (nor are there any problems of a
+	 * similar nature) with this variant of the gizmo.
+	 */
+	if (pg_prng_uint32(&pg_global_prng_state) <= (PG_UINT32_MAX / 50))
+		return false;
+#endif
+
+	return ConditionalLockBufferForCleanup(buffer);
+}
+
 /*
  *	lazy_scan_heap() -- workhorse function for VACUUM
  *
@@ -1093,7 +1127,7 @@ lazy_scan_heap(LVRelState *vacrel, int nworkers)
 		 * a cleanup lock right away, we may be able to settle for reduced
 		 * processing using lazy_scan_noprune.
 		 */
-		if (!ConditionalLockBufferForCleanup(buf))
+		if (!lazy_conditionallockbufferforcleanup(buf))
 		{
 			bool		hastup,
 						recordfreespace;
-- 
2.30.2

---

From: Andres Freund <[email-masked]>
Date: Sat, 19 Feb 2022 16:12:31 -0800

Hi,

(On phone, so crappy formatting and no source access)

>GlobalVisTempRels behavior established by Andres' 2020 bugfix commit

We don't have a blocking path for cleanup locks of temporary buffers IIRC (normally not reachable). So I wouldn't be surprised if a cleanup lock failing would cause some odd behavior.

>ConditionalLockBufferForCleanup().

Definitely worth looking into more.

This reminds me of a recent thing I noticed in the aio patch. Spgist can end up busy looping when buffers are locked, instead of blocking. Not actually related, of course.

Andres
-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.

---

From: Peter Geoghegan <[email-masked]>
Date: Sat, 19 Feb 2022 16:22:23 -0800

On Sat, Feb 19, 2022 at 3:08 PM Peter Geoghegan <[email-masked]> wrote:
> informative.

This very much looks like a bug in pg_surgery itself now -- attached
is a draft fix.

The temp table thing was a red herring. I found I could get exactly
the same kind of failure when htab2 was a permanent table (which was
how it originally appeared, before commit 0811f766fd made it into a
temp table due to test flappiness issues). The relevant "vacuum freeze
htab2" happens at a point after the test has already deliberately
corrupted one of its tuples using heap_force_kill().  It's not that we
aren't careful enough about the corruption at some point in
vacuumlazy.c, which was my second theory. But I quickly discarded that
idea, and came up with a third theory: the relevant heap_surgery.c
path does the relevant ItemIdSetDead() to kill items, without also
defragmenting the page to remove the tuples with storage, which is
wrong.

This meant that we depended on pruning happening (in this case during
VACUUM) and defragmenting the page in passing. But there is no reason
to not defragment the page within pg_surgery (at least no obvious
reason), since we have a cleanup lock anyway.

Theoretically you could blame this on lazy_scan_noprune instead, since
it thinks it can collect LP_DEAD items while assuming that they have
no storage, but that doesn't make much sense to me. There has never
been any way of setting a heap item to LP_DEAD without also
defragmenting the page.  Since that's exactly what it means to prune a
heap page. (Actually, the same used to be true about heap vacuuming,
which worked more like heap pruning before Postgres 14, but that
doesn't seem important.)

-- 
Peter Geoghegan
From 81f01ca623b115647ee78a1b09bbb4458fb35dab Mon Sep 17 00:00:00 2001
From: Peter Geoghegan <[email-masked]>
Date: Sat, 19 Feb 2022 16:13:48 -0800
Subject: [PATCH 2/2] Fix for pg_surgery's heap_force_kill() function.

---
 contrib/pg_surgery/heap_surgery.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/contrib/pg_surgery/heap_surgery.c b/contrib/pg_surgery/heap_surgery.c
index 3e641aa64..a3a193ba5 100644
--- a/contrib/pg_surgery/heap_surgery.c
+++ b/contrib/pg_surgery/heap_surgery.c
@@ -311,7 +311,8 @@ heap_force_common(FunctionCallInfo fcinfo, HeapTupleForceOption heap_force_opt)
 		 */
 		if (did_modify_page)
 		{
-			/* Mark buffer dirty before we write WAL. */
+			/* Defragment and mark buffer dirty before we write WAL. */
+			PageRepairFragmentation(page);
 			MarkBufferDirty(buf);
 
 			/* XLOG stuff */
-- 
2.30.2

---

From: Peter Geoghegan <[email-masked]>
Date: Sat, 19 Feb 2022 17:22:33 -0800

> This very much looks like a bug in pg_surgery itself now -- attached

Wait, that's not it either. I jumped the gun -- this isn't sufficient
(though the patch I posted might not be a bad idea anyway).

Looks like pg_surgery isn't processing HOT chains as whole units,
which it really should (at least in the context of killing items via
the heap_force_kill() function). Killing a root item in a HOT chain is
just hazardous -- disconnected/orphaned heap-only tuples are liable to
cause chaos, and should be avoided everywhere (including during
pruning, and within pg_surgery).

It's likely that the hardening I already planned on adding to pruning
[1] (as follow-up work to recent bugfix commit 18b87b201f) will
prevent lazy_scan_prune from getting stuck like this, whatever the
cause happens to be. The actual page image I see lazy_scan_prune choke
on (i.e. exhibit the same infinite loop unpleasantness we've seen
before on) is not in a consistent state at all (its tuples consist of
tuples from a single HOT chain, and the HOT chain is totally
inconsistent on account of having an LP_DEAD line pointer root item).
pg_surgery could in principle do the right thing here by always
treating HOT chains as whole units.

Leaving behind disconnected/orphaned heap-only tuples is pretty much
pointless anyway, since they'll never be accessible by index scans.
Even after a REINDEX, since there is no root item from the heap page
to go in the index. (A dump and restore might work better, though.)

[1] https://postgr.es/m/[email-masked]
-- 
Peter Geoghegan

---

From: Andres Freund <[email-masked]>
Date: Sat, 19 Feb 2022 17:54:16 -0800

Hi,

> pruning, and within pg_surgery).

How does that cause the endless loop?

It doesn't do so on HEAD + 0001-Add-adversarial-ConditionalLockBuff[...] for
me. So something needs have changed with your patch?

> cause happens to be.

Yea, we should pick that up again. Not just for robustness or
performance. Also because it's just a lot easier to understand.

> to go in the index. (A dump and restore might work better, though.)

Given that heap_surgery's raison d'etre is correcting corruption etc, I think
it makes sense for it to do as minimal work as possible. Iterating through a
HOT chain would be a problem if you e.g. tried to repair a page with HOT
corruption.

Greetings,

Andres Freund

---

From: Peter Geoghegan <[email-masked]>
Date: Sat, 19 Feb 2022 18:16:54 -0800

> How does that cause the endless loop?

Attached is the page image itself, dumped via gdb (and gzip'd). This
was on recent HEAD (commit 8f388f6f, actually), plus
0001-Add-adversarial-ConditionalLockBuff[...]. No other changes. No
defragmenting in pg_surgery, nothing like that.

> me. So something needs have changed with your patch?

It doesn't always happen -- only about half the time on my machine.
Maybe it's timing sensitive?

We hit the "goto retry" on offnum 2, which is the first tuple with
storage (you can see "the ghost" of the tuple from the LP_DEAD item at
offnum 1, since the page isn't defragmented in pg_surgery). I think
that this happens because the heap-only tuple at offnum 2 is fully
DEAD to lazy_scan_prune, but hasn't been recognized as such by
heap_page_prune. There is no way that they'll ever "agree" on the
tuple being DEAD right now, because pruning still doesn't assume that
an orphaned heap-only tuple is fully DEAD.

We can either do that, or we can throw an error concerning corruption
when heap_page_prune notices orphaned tuples. Neither seems
particularly appealing. But it definitely makes no sense to allow
lazy_scan_prune to spin in a futile attempt to reach agreement with
heap_page_prune about a DEAD tuple really being DEAD.

> HOT chain would be a problem if you e.g. tried to repair a page with HOT

I guess that's also true. There is at least a legitimate argument to
be made for not leaving behind any orphaned heap-only tuples. The
interface is a TID, and so the user may already believe that they're
killing the heap-only, not just the root item (since ctid suggests
that the TID of a heap-only tuple is the TID of the root item, which
is kind of misleading).

Anyway, we can decide on what to do in heap_surgery later, once the
main issue is under control. My point was mostly just that orphaned
heap-only tuples are definitely not okay, in general. They are the
least worst option when corruption has already happened, maybe -- but
maybe not.

-- 
Peter Geoghegan

---

From: Andres Freund <[email-masked]>
Date: Sat, 19 Feb 2022 19:01:28 -0800

Hi,

> Maybe it's timing sensitive?

Ah, I'd only run the tests three times or so, without it happening. Trying a
few more times repro'd it.

It's kind of surprising that this needs this
0001-Add-adversarial-ConditionalLockBuff to break. I suspect it's a question
of hint bits changing due to lazy_scan_noprune(), which then makes
HeapTupleHeaderIsHotUpdated() have a different return value, preventing the
"If the tuple is DEAD and doesn't chain to anything else"
path from being taken.

> heap_page_prune about a DEAD tuple really being DEAD.

Yea, this sucks. I think we should go for the rewrite of the
heap_prune_chain() logic. The current approach is just never going to be
robust.

Greetings,

Andres Freund

---

From: Peter Geoghegan <[email-masked]>
Date: Sat, 19 Feb 2022 19:07:39 -0800

> heap_prune_chain() logic. The current approach is just never going to be

No, it just isn't robust enough. But it's not that hard to fix. My
patch really wasn't invasive.

I confirmed that HeapTupleSatisfiesVacuum() and
heap_prune_satisfies_vacuum() agree that the heap-only tuple at offnum
2 is HEAPTUPLE_DEAD -- they are in agreement, as expected (so no
reason to think that there is a new bug involved). The problem here is
indeed just that heap_prune_chain() can't "get to" the tuple, given
its current design.

For anybody else that doesn't follow what we're talking about:

The "doesn't chain to anything else" code at the start of
heap_prune_chain() won't get to the heap-only tuple at offnum 2, since
the tuple is itself HeapTupleHeaderIsHotUpdated() -- the expectation
is that it'll be processed later on, once we locate the HOT chain's
root item. Since, of course, the "root item" was already LP_DEAD
before we even reached heap_page_prune() (on account of the pg_surgery
corruption), there is no possible way that that can happen later on.
And so we cannot find the same heap-only tuple and mark it LP_UNUSED
(which is how we always deal with HEAPTUPLE_DEAD heap-only tuples)
during pruning.

-- 
Peter Geoghegan

---

From: Peter Geoghegan <[email-masked]>
Date: Sat, 19 Feb 2022 19:18:25 -0800

> "If the tuple is DEAD and doesn't chain to anything else"

That makes sense as an explanation. Goes to show just how fragile the
"DEAD and doesn't chain to anything else" logic at the top of
heap_prune_chain really is.

-- 
Peter Geoghegan

---

From: Andres Freund <[email-masked]>
Date: Sat, 19 Feb 2022 19:28:33 -0800

Hi,

> patch really wasn't invasive.

I think we're in agreement there. We might think at some point about
backpatching too, but I'd rather have it stew in HEAD for a bit first.

> indeed just that heap_prune_chain() can't "get to" the tuple, given

Right.

The reason that the "adversarial" patch makes a different is solely that it
changes the heap_surgery test to actually kill an item, which it doesn't
intend:

create temp table htab2(a int);
insert into htab2 values (100);
update htab2 set a = 200;
vacuum htab2;

-- redirected TIDs should be skipped
select heap_force_kill('htab2'::regclass, ARRAY['(0, 1)']::tid[]);

If the vacuum can get the cleanup lock due to the adversarial patch, the
heap_force_kill() doesn't do anything, because the first item is a
redirect. However if it *can't* get a cleanup lock, heap_force_kill() instead
targets the root item. Triggering the endless loop.

Hm. I think this might be a mild regression in 14. In < 14 we'd just skip the
tuple in lazy_scan_heap(), but now we have an uninterruptible endless
loop.

We'd do completely bogus stuff later in < 14 though, I think we'd just leave
it in place despite being older than relfrozenxid, which obviously has its own
set of issues.

Greetings,

Andres Freund

---

From: Peter Geoghegan <[email-masked]>
Date: Sat, 19 Feb 2022 19:31:21 -0800

> that the TID of a heap-only tuple is the TID of the root item, which

Actually, I would say that heap_surgery's raison d'etre is making
weird errors related to corruption of this or that TID go away, so
that the user can cut their losses. That's how it's advertised.

Let's assume that we don't want to make VACUUM/pruning just treat
orphaned heap-only tuples as DEAD, regardless of their true HTSV-wise
status -- let's say that we want to err in the direction of doing
nothing at all with the page. Now we have to have a weird error in
VACUUM instead (not great, but better than just spinning between
lazy_scan_prune and heap_prune_page). And we've just created natural
demand for heap_surgery to deal with the problem by deleting whole HOT
chains (not just root items).

If we allow VACUUM to treat orphaned heap-only tuples as DEAD right
away, then we might as well do the same thing in heap_surgery, since
there is little chance that the user will get to the heap-only tuples
before VACUUM does (not something to rely on, at any rate).

Either way, I think we probably end up needing to teach heap_surgery
to kill entire HOT chains as a group, given a TID.

-- 
Peter Geoghegan

---

From: Peter Geoghegan <[email-masked]>
Date: Sat, 19 Feb 2022 19:40:20 -0800

> targets the root item. Triggering the endless loop.

But it shouldn't matter if the root item is an LP_REDIRECT or a normal
(not heap-only) tuple with storage. Either way it's the root of a HOT
chain.

The fact that pg_surgery treats LP_REDIRECT items differently from the
other kind of root items is just arbitrary. It seems to have more to
do with freezing tuples than killing tuples.

--
Peter Geoghegan

---

From: Andres Freund <[email-masked]>
Date: Sat, 19 Feb 2022 19:47:32 -0800

Hi,

> that the user can cut their losses. That's how it's advertised.

I'm not that sure those are that different... Imagine some corruption leading
to two hot chains ending in the same tid, which our fancy new secure pruning
algorithm might detect.

Either way, I'm a bit surprised about the logic to not allow killing redirect
items? What if you have a redirect pointing to an unused item?

> status

I don't think that'd ever be a good idea. Those tuples are visible to a
seqscan after all.

> -- let's say that we want to err in the direction of doing
> lazy_scan_prune and heap_prune_page).

Non DEAD orphaned versions shouldn't cause a problem in lazy_scan_prune(). The
problem here is a DEAD orphaned HOT tuples, and those we should be able to
delete with the new page pruning logic, right?

I think it might be worth getting rid of the need for the retry approach by
reusing the same HTSV status array between heap_prune_page and
lazy_scan_prune. Then the only legitimate reason for seeing a DEAD item in
lazy_scan_prune() would be some form of corruption.  And it'd be a pretty
decent performance boost, HTSV ain't cheap.

Greetings,

Andres Freund

---

From: Peter Geoghegan <[email-masked]>
Date: Sat, 19 Feb 2022 19:56:53 -0800

> to two hot chains ending in the same tid, which our fancy new secure pruning

I suppose that's possible, but it doesn't seem all that likely to ever
happen, what with the xmin -> xmax cross-tuple matching stuff.

> items? What if you have a redirect pointing to an unused item?

Again, I simply think it boils down to having to treat HOT chains as a
whole unit when killing TIDs.

> I don't think that'd ever be a good idea. Those tuples are visible to a

I agree (I don't hate it completely, but it seems mostly bad). This is
what leads me to the conclusion that pg_surgery has to be able to do
this instead. Surely it's not okay to have something that makes VACUUM
always end in error, that cannot even be fixed by pg_surgery.

> delete with the new page pruning logic, right?

Right. But what good does that really do? The problematic page had a
third tuple (at offnum 3) that was LIVE. If we could have done
something about the problematic tuple at offnum 2 (which is where we
got stuck), then we'd still be left with a very unpleasant choice
about what happens to the third tuple.

> decent performance boost, HTSV ain't cheap.

I guess it doesn't actually matter if we leave an aborted DEAD tuple
behind, that we could have pruned away, but didn't. The important
thing is to be consistent at the level of the page.

-- 
Peter Geoghegan

---

From: Andres Freund <[email-masked]>
Date: Sat, 19 Feb 2022 20:21:47 -0800

Hi, 

>about what happens to the third tuple.

Why does anything need to happen to it from vacuum's POV?  It'll not be a problem for freezing etc. Until it's deleted vacuum doesn't need to care.

Probably worth a WARNING, and amcheck definitely needs to detect it, but otherwise I think it's fine to just continue.

>thing is to be consistent at the level of the page.

That's not ok, because it opens up dangers of being interpreted differently after wraparound etc.

But I don't see any cases where it would happen with the new pruning logic in your patch and sharing the HTSV status array?

Andres

---

From: Peter Geoghegan <[email-masked]>
Date: Sat, 19 Feb 2022 20:28:21 -0800

> Probably worth a WARNING, and amcheck definitely needs to detect it, but otherwise I think it's fine to just continue.

Maybe that's true, but it's just really weird to imagine not having an
LP_REDIRECT that points to the LIVE item here, without throwing an
error. Seems kind of iffy, to say the least.

> But I don't see any cases where it would happen with the new pruning logic in your patch and sharing the HTSV status array?

Right. Fundamentally, there isn't any reason why it should matter that
VACUUM reached the heap page just before (rather than concurrent with
or just after) some xact that inserted or updated on the page aborts.
Just as long as we have a consistent idea about what's going on at the
level of the whole page (or maybe the level of each HOT chain, but the
whole page level seems simpler to me).

-- 
Peter Geoghegan

---

From: Robert Haas <[email-masked]>
Date: Sun, 20 Feb 2022 10:03:17 -0500

> HOT chain would be a problem if you e.g. tried to repair a page with HOT

Yeah, I agree. I don't have time to respond to all of these emails
thoroughly right now, but I think it's really important that
pg_surgery do the exact surgery the user requested, and not any other
work. I don't think that page defragmentation should EVER be REQUIRED
as a condition of other work. If other code is relying on that, I'd
say it's busted. I'm a little more uncertain about the case where we
kill the root tuple of a HOT chain, because I can see that this might
leave the page a state where sequential scans see the tuple at the end
of the chain and index scans don't. I'm not sure whether that should
be the responsibility of pg_surgery itself to avoid, or whether that's
your problem as a user of it -- although I lean mildly toward the
latter view, at the moment. But in any case surely the pruning code
can't just decide to go into an infinite loop if that happens. Code
that manipulates the states of data pages needs to be as robust
against arbitrary on-disk states as we can reasonably make it, because
pages get garbled on disk all the time.

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