On 28/03/2024 03:53, Melanie Plageman wrote:
On Thu, Mar 28, 2024 at 01:04:04AM +0200, Heikki Linnakangas wrote:
One change with this is that live_tuples and many of the other fields are
now again updated, even if the caller doesn't need them. It was hard to skip
them in a way that would save any cycles, with the other refactorings.
I am worried we are writing checks that are going to have to come out of
SELECT queries' bank accounts, but I'll do some benchmarking when we're
all done with major refactoring.
Sounds good, thanks.
+ *
+ * Whether we arrive at the dead HOT tuple first here
or while
+ * following a chain below affects whether preceding
RECENTLY_DEAD
+ * tuples in the chain can be removed or not. Imagine
that you
+ * have a chain with two tuples: RECENTLY_DEAD -> DEAD.
If we
+ * reach the RECENTLY_DEAD tuple first, the
chain-following logic
+ * will find the DEAD tuple and conclude that both
tuples are in
+ * fact dead and can be removed. But if we reach the
DEAD tuple
+ * at the end of the chain first, when we reach the
RECENTLY_DEAD
+ * tuple later, we will not follow the chain because
the DEAD
+ * TUPLE is already 'marked', and will not remove the
+ * RECENTLY_DEAD tuple. This is not a correctness
issue, and the
+ * RECENTLY_DEAD tuple will be removed by a later
VACUUM.
*/
if (!HeapTupleHeaderIsHotUpdated(htup))
Is this intentional? Like would it be correct to remove the
RECENTLY_DEAD tuple during the current vacuum?
Yes, it would be correct. And if we happen to visit the items in
different order, the RECENTLY_DEAD tuple first, it will get removed.
(This is just based on me reading the code, I'm not sure if I've missed
something. Would be nice to construct a test case for that and step
through it with a debugger to really see what happens. But this is not a
new issue, doesn't need to be part of this patch)
From c2ee7508456d0e76de985f9997a6840450e342a8 Mon Sep 17 00:00:00 2001
From: Heikki Linnakangas <heikki.linnakan...@iki.fi>
Date: Thu, 28 Mar 2024 00:45:26 +0200
Subject: [PATCH v8 22/22] WIP
- Got rid of all_visible_except_removable again. We're back to the
roughly the same mechanism as on 'master', where the all_visible
doesn't include LP_DEAD items, but at the end of
heap_page_prune_and_freeze() when we return to the caller, we clear
it if there were any LP_DEAD items. I considered calling the
variable 'all_visible_except_lp_dead', which would be more accurate,
but it's also very long.
not longer than all_visible_except_removable. I would be happy to keep
it more exact, but I'm also okay with just all_visible.
Ok, let's make it 'all_visible_except_lp_dead' for clarity.
What's this "cutoffs TODO"?
+ * cutoffs TODO
+ *
* presult contains output parameters needed by callers such as the number of
* tuples removed and the number of line pointers newly marked LP_DEAD.
* heap_page_prune_and_freeze() is responsible for initializing it.
All the other arguments are documented in the comment, except 'cutoffs'.
@@ -728,10 +832,12 @@ htsv_get_valid_status(int status)
* DEAD, our visibility test is just too coarse to detect it.
*
* In general, pruning must never leave behind a DEAD tuple that still has
- * tuple storage. VACUUM isn't prepared to deal with that case. That's why
+ * tuple storage. VACUUM isn't prepared to deal with that case (FIXME: it no
longer cares, right?).
+ * That's why
* VACUUM prunes the same heap page a second time (without dropping its lock
* in the interim) when it sees a newly DEAD tuple that we initially saw as
- * in-progress. Retrying pruning like this can only happen when an inserting
+ * in-progress (FIXME: Really? Does it still do that?).
Yea, I think all that is no longer true. I missed this comment back
then.
Committed a patch to remove it.
@@ -981,7 +1069,18 @@ heap_prune_chain(Buffer buffer, OffsetNumber rootoffnum,
* RECENTLY_DEAD tuples just in case there's a DEAD one after
them;
* but we can't advance past anything else. We have to make
sure that
* we don't miss any DEAD tuples, since DEAD tuples that still
have
- * tuple storage after pruning will confuse VACUUM.
+ * tuple storage after pruning will confuse VACUUM (FIXME: not
anymore
+ * I think?).
Meaning, it won't confuse vacuum anymore or there won't be DEAD tuples
with storage after pruning anymore?
I meant that it won't confuse VACUUM anymore. lazy_scan_prune() doesn't
loop through the items on the page checking their visibility anymore.
Hmm, one confusion remains though: In the 2nd phase of vacuum, we remove
all the dead line pointers that we have now removed from the indexes.
When we do that, we assume them all to be dead line pointers, without
storage, rather than normal tuples that happen to be HEAPTUPLE_DEAD. So
it's important that if pruning would leave behind HEAPTUPLE_DEAD tuples,
they are not included in 'deadoffsets'.
In any case, let's just make sure that pruning doesn't leave
HEAPTUPLE_DEAD tuples. There's no reason it should.
@@ -1224,10 +1327,21 @@ heap_prune_record_live_or_recently_dead(Page page,
PruneState *prstate, OffsetNu
* ensure the math works out. The assumption that the transaction will
* commit and update the counters after we report is a bit shaky; but it
* is what acquire_sample_rows() does, so we do the same to be
consistent.
+ *
+ * HEAPTUPLE_DEAD are handled by the other heap_prune_record_*()
+ * subroutines. They don't count dead items like acquire_sample_rows()
+ * does, because we assume that all dead items will become LP_UNUSED
+ * before VACUUM finishes. This difference is only superficial. VACUUM
+ * effectively agrees with ANALYZE about DEAD items, in the end. VACUUM
+ * won't remember LP_DEAD items, but only because they're not supposed
to
+ * be left behind when it is done. (Cases where we bypass index
vacuuming
+ * will violate this optimistic assumption, but the overall impact of
that
+ * should be negligible.) FIXME: I don't understand that last sentence
in
+ * parens. It was copied from elsewhere.
If we bypass index vacuuming, there will be LP_DEAD items left behind
when we are done because we didn't do index vacuuming and then reaping
of those dead items. All of these comments are kind of a copypasta,
though.
Ah, gotcha, makes sense now. I didn't remember that we sometimes by pass
index vacuuming if there are very few dead items. I thought this talked
about the case that there are no indexes, but that case is OK.
--
Heikki Linnakangas
Neon (https://neon.tech)
