On Wed, Apr 3, 2024 at 8:39 AM Heikki Linnakangas <hlinn...@iki.fi> wrote:
>
> On 02/04/2024 16:11, Heikki Linnakangas wrote:
> > On 01/04/2024 20:22, Melanie Plageman wrote:
> >> Review for 0003-0006 (I didn't have any new thoughts on 0002). I know
> >> you didn't modify them much/at all, but I noticed some things in my code
> >> that could be better.
> >
> > Ok, here's what I have now. I made a lot of small comment changes here
> > and there, and some minor local refactorings, but nothing major. I lost
> > track of all the individual changes I'm afraid, so I'm afraid you'll
> > have to just diff against the previous version if you want to see what's
> > changed. I hope I didn't break anything.
> >
> > I'm pretty happy with this now. I will skim through it one more time
> > later today or tomorrow, and commit. Please review once more if you have
> > a chance.
> >
> >> This probably doesn't belong here. I noticed spgdoinsert.c had a static
> >> function for sorting OffsetNumbers, but I didn't see anything general
> >> purpose anywhere else.
> >
> > I copied the spgdoinsert.c implementation to vacuumlazy.c as is. Would
> > be nice to have just one copy of this in some common place, but I also
> > wasn't sure where to put it.
>
> One more version, with two small fixes:
>
> 1. I fumbled the offsetnumber-cmp function at the last minute so that it
> didn't compile. Fixed. that
I noticed you didn't make the comment updates I suggested in my
version 13 here [1]. A few of them are outdated references to
heap_page_prune() and one to a now deleted variable name
(all_visible_except_removable).
I applied them to your v13 and attached the diff.
> Off-list, Melanie reported that there is a small regression with the
> benchmark script she posted yesterday, after all, but I'm not able to
> reproduce that.
Actually, I think it was noise.
- Melanie
[1]
https://www.postgresql.org/message-id/CAAKRu_aPqZkThyfr0USaHp-3cN_ruEdAHBKtNQJqXDTjWUz0rw%40mail.gmail.com
From 882e937c122f5e83bc9ba643443c1a27c807d82e Mon Sep 17 00:00:00 2001
From: Melanie Plageman <melanieplage...@gmail.com>
Date: Wed, 3 Apr 2024 10:12:44 -0400
Subject: [PATCH 3/3] comment updates
---
src/backend/access/heap/pruneheap.c | 53 +++++++++++++----------------
src/backend/storage/ipc/procarray.c | 6 ++--
src/include/access/heapam.h | 2 +-
3 files changed, 28 insertions(+), 33 deletions(-)
diff --git a/src/backend/access/heap/pruneheap.c b/src/backend/access/heap/pruneheap.c
index 8ed44ba93d..4a6a4cee4d 100644
--- a/src/backend/access/heap/pruneheap.c
+++ b/src/backend/access/heap/pruneheap.c
@@ -328,7 +328,8 @@ heap_page_prune_opt(Relation relation, Buffer buffer)
*
* presult contains output parameters needed by callers, such as the number of
* tuples removed and the offsets of dead items on the page after pruning.
- * heap_page_prune_and_freeze() is responsible for initializing it.
+ * heap_page_prune_and_freeze() is responsible for initializing it. Required by
+ * all callers.
*
* reason indicates why the pruning is performed. It is included in the WAL
* record for debugging and analysis purposes, but otherwise has no effect.
@@ -393,6 +394,7 @@ heap_page_prune_and_freeze(Relation relation, Buffer buffer,
prstate.pagefrz.freeze_required = false;
if (prstate.freeze)
{
+ Assert(new_relfrozen_xid && new_relmin_mxid);
prstate.pagefrz.FreezePageRelfrozenXid = *new_relfrozen_xid;
prstate.pagefrz.NoFreezePageRelfrozenXid = *new_relfrozen_xid;
prstate.pagefrz.FreezePageRelminMxid = *new_relmin_mxid;
@@ -415,19 +417,19 @@ heap_page_prune_and_freeze(Relation relation, Buffer buffer,
prstate.deadoffsets = presult->deadoffsets;
/*
- * Caller may update the VM after we're done. We keep track of whether
- * the page will be all_visible and all_frozen, once we're done with the
- * pruning and freezing, to help the caller to do that.
+ * Caller may update the VM after we're done. We can keep track of
+ * whether the page will be all-visible and all-frozen after pruning and
+ * freezing to help the caller to do that.
*
* Currently, only VACUUM sets the VM bits. To save the effort, only do
- * only the bookkeeping if the caller needs it. Currently, that's tied to
- * HEAP_PAGE_PRUNE_FREEZE, but it could be a separate flag, if you wanted
- * to update the VM bits without also freezing, or freezing without
+ * the bookkeeping if the caller needs it. Currently, that's tied to
+ * HEAP_PAGE_PRUNE_FREEZE, but it could be a separate flag if you wanted
+ * to update the VM bits without also freezing or freeze without also
* setting the VM bits.
*
* In addition to telling the caller whether it can set the VM bit, we
* also use 'all_visible' and 'all_frozen' for our own decision-making. If
- * the whole page will become frozen, we consider opportunistically
+ * the whole page would become frozen, we consider opportunistically
* freezing tuples. We will not be able to freeze the whole page if there
* are tuples present that are not visible to everyone or if there are
* dead tuples which are not yet removable. However, dead tuples which
@@ -681,16 +683,16 @@ heap_page_prune_and_freeze(Relation relation, Buffer buffer,
else
{
/*
- * Opportunistically freeze the page, if we are generating an FPI
- * anyway, and if doing so means that we can set the page
+ * Opportunistically freeze the page if we are generating an FPI
+ * anyway and if doing so means that we can set the page
* all-frozen afterwards (might not happen until VACUUM's final
* heap pass).
*
* XXX: Previously, we knew if pruning emitted an FPI by checking
* pgWalUsage.wal_fpi before and after pruning. Once the freeze
- * and prune records are combined, this heuristic couldn't be used
- * anymore. The opportunistic freeze heuristic must be improved;
- * however, for now, try to approximate the old logic.
+ * and prune records were combined, this heuristic couldn't be
+ * used anymore. The opportunistic freeze heuristic must be
+ * improved; however, for now, try to approximate the old logic.
*/
if (prstate.all_visible && prstate.all_frozen && prstate.nfrozen > 0)
{
@@ -766,7 +768,7 @@ heap_page_prune_and_freeze(Relation relation, Buffer buffer,
/*
* If that's all we had to do to the page, this is a non-WAL-logged
- * hint. If we will also freeze or prune the page, we will mark the
+ * hint. If we are going to freeze or prune the page, we will mark the
* buffer dirty below.
*/
if (!do_freeze && !do_prune)
@@ -854,12 +856,12 @@ heap_page_prune_and_freeze(Relation relation, Buffer buffer,
* make the choice of whether or not to freeze the page unaffected by the
* short-term presence of LP_DEAD items. These LP_DEAD items were
* effectively assumed to be LP_UNUSED items in the making. It doesn't
- * matter which heap pass (initial pass or final pass) ends up setting the
- * page all-frozen, as long as the ongoing VACUUM does it.
+ * matter which vacuum heap pass (initial pass or final pass) ends up
+ * setting the page all-frozen, as long as the ongoing VACUUM does it.
*
* Now that freezing has been finalized, unset all_visible if there are
* any LP_DEAD items on the page. It needs to reflect the present state
- * of things, as expected by our caller.
+ * of the page, as expected by our caller.
*/
if (prstate.all_visible && prstate.lpdead_items == 0)
{
@@ -1232,14 +1234,7 @@ heap_prune_record_dead(PruneState *prstate, OffsetNumber offnum,
/*
* Deliberately delay unsetting all_visible until later during pruning.
- * Removable dead tuples shouldn't preclude freezing the page. After
- * finishing this first pass of tuple visibility checks, initialize
- * all_visible_except_removable with the current value of all_visible to
- * indicate whether or not the page is all visible except for dead tuples.
- * This will allow us to attempt to freeze the page after pruning. Later
- * during pruning, if we encounter an LP_DEAD item or are setting an item
- * LP_DEAD, we will unset all_visible. As long as we unset it before
- * updating the visibility map, this will be correct.
+ * Removable dead tuples shouldn't preclude freezing the page.
*/
/* Record the dead offset for vacuum */
@@ -1663,10 +1658,10 @@ heap_page_prune_execute(Buffer buffer, bool lp_truncate_only,
else
{
/*
- * When heap_page_prune() was called, mark_unused_now may have
- * been passed as true, which allows would-be LP_DEAD items to be
- * made LP_UNUSED instead. This is only possible if the relation
- * has no indexes. If there are any dead items, then
+ * When heap_page_prune_and_freeze() was called, mark_unused_now
+ * may have been passed as true, which allows would-be LP_DEAD
+ * items to be made LP_UNUSED instead. This is only possible if
+ * the relation has no indexes. If there are any dead items, then
* mark_unused_now was not true and every item being marked
* LP_UNUSED must refer to a heap-only tuple.
*/
diff --git a/src/backend/storage/ipc/procarray.c b/src/backend/storage/ipc/procarray.c
index b3cd248fb6..88a6d504df 100644
--- a/src/backend/storage/ipc/procarray.c
+++ b/src/backend/storage/ipc/procarray.c
@@ -1715,9 +1715,9 @@ TransactionIdIsActive(TransactionId xid)
* Note: the approximate horizons (see definition of GlobalVisState) are
* updated by the computations done here. That's currently required for
* correctness and a small optimization. Without doing so it's possible that
- * heap vacuum's call to heap_page_prune() uses a more conservative horizon
- * than later when deciding which tuples can be removed - which the code
- * doesn't expect (breaking HOT).
+ * heap vacuum's call to heap_page_prune_and_freeze() uses a more conservative
+ * horizon than later when deciding which tuples can be removed - which the
+ * code doesn't expect (breaking HOT).
*/
static void
ComputeXidHorizons(ComputeXidHorizonsResult *h)
diff --git a/src/include/access/heapam.h b/src/include/access/heapam.h
index 536711d98e..a307fb5f24 100644
--- a/src/include/access/heapam.h
+++ b/src/include/access/heapam.h
@@ -238,7 +238,7 @@ typedef struct PruneFreezeResult
OffsetNumber deadoffsets[MaxHeapTuplesPerPage];
} PruneFreezeResult;
-/* 'reason' codes for heap_page_prune() */
+/* 'reason' codes for heap_page_prune_and_freeze() */
typedef enum
{
PRUNE_ON_ACCESS, /* on-access pruning */
--
2.40.1
