v3heikki-0004-Refactor-heap_page_prune_and_freeze-some-mo.patch

From 978fc7c9c6a3c30f34c8d54a98aad8b5163fd0ab Mon Sep 17 00:00:00 2001
From: Heikki Linnakangas <heikki.linnakangas@iki.fi>
Date: Mon, 18 Mar 2024 00:47:44 +0200
Subject: [PATCH v3heikki 4/4] Refactor heap_page_prune_and_freeze() some more

- Move WAL-logging to separate function

- Refactor the code that executes the actions prune, freeze and
  hint-bit setting actions on the page. There's a little bit of
  repetition, in how pd_prune_xid is set and the PageClearFull() call,
  but I find this easier to follow.

- Instead of checking after-the-fact if MarkBufferDirtyHint()
  generated an FPI, check before entering the critical section if it
  would. There's a small chance that a checkpoint started in between
  and this gives different answer, but it's a very rare and this is a
  crude heuristic anyway.
---
 src/backend/access/heap/pruneheap.c | 433 ++++++++++++++--------------
 1 file changed, 214 insertions(+), 219 deletions(-)

diff --git a/src/backend/access/heap/pruneheap.c b/src/backend/access/heap/pruneheap.c
index 3541628799a..4d7f7f7ea94 100644
--- a/src/backend/access/heap/pruneheap.c
+++ b/src/backend/access/heap/pruneheap.c
@@ -48,6 +48,9 @@ typedef struct
 	OffsetNumber nowdead[MaxHeapTuplesPerPage];
 	OffsetNumber nowunused[MaxHeapTuplesPerPage];
 
+	/* one entry for every tuple that we may freeze */
+	HeapTupleFreeze frozen[MaxHeapTuplesPerPage];
+
 	/*
 	 * marked[i] is true if item i is entered in one of the above arrays.
 	 *
@@ -89,6 +92,8 @@ static void heap_prune_record_dead_or_unused(PruneState *prstate, OffsetNumber o
 static void heap_prune_record_unused(PruneState *prstate, OffsetNumber offnum);
 static void page_verify_redirects(Page page);
 
+static void log_heap_prune_and_freeze(Relation relation, Buffer buffer, PruneState *prstate, PruneFreezeResult *presult);
+
 
 /*
  * Optionally prune and repair fragmentation in the specified page.
@@ -270,14 +275,8 @@ heap_page_prune_and_freeze(Relation relation, Buffer buffer,
 	bool		do_hint;
 	bool		whole_page_freezable;
 	bool		hint_bit_fpi;
-	bool		prune_fpi = false;
 	int64		fpi_before = pgWalUsage.wal_fpi;
 
-	/*
-	 * One entry for every tuple that we may freeze.
-	 */
-	HeapTupleFreeze frozen[MaxHeapTuplesPerPage];
-
 	/*
 	 * Our strategy is to scan the page and make lists of items to change,
 	 * then apply the changes within a critical section.  This keeps as much
@@ -520,11 +519,11 @@ heap_page_prune_and_freeze(Relation relation, Buffer buffer,
 				bool		totally_frozen;
 
 				if ((heap_prepare_freeze_tuple(htup, pagefrz,
-											   &frozen[presult->nfrozen],
+											   &prstate.frozen[presult->nfrozen],
 											   &totally_frozen)))
 				{
 					/* Save prepared freeze plan for later */
-					frozen[presult->nfrozen++].offset = offnum;
+					prstate.frozen[presult->nfrozen++].offset = offnum;
 				}
 
 				/*
@@ -596,13 +595,6 @@ heap_page_prune_and_freeze(Relation relation, Buffer buffer,
 	/* Record number of newly-set-LP_DEAD items for caller */
 	presult->nnewlpdead = prstate.ndead;
 
-	/*
-	 * Only incur overhead of checking if we will do an FPI if we might use
-	 * the information.
-	 */
-	if (do_prune && pagefrz)
-		prune_fpi = XLogCheckBufferNeedsBackup(buffer);
-
 	/*
 	 * Even if we don't prune anything, if we found a new value for the
 	 * pd_prune_xid field or the page was marked full, we will update the hint
@@ -626,9 +618,24 @@ heap_page_prune_and_freeze(Relation relation, Buffer buffer,
 	 * opportunistic freeze heuristic must be improved; however, for now, try
 	 * to approximate it.
 	 */
-	do_freeze = pagefrz &&
-		(pagefrz->freeze_required ||
-		 (whole_page_freezable && presult->nfrozen > 0 && (prune_fpi || hint_bit_fpi)));
+	do_freeze = false;
+	if (pagefrz)
+	{
+		if (pagefrz->freeze_required)
+			do_freeze = true;
+		else if (whole_page_freezable && presult->nfrozen > 0)
+		{
+			/*
+			 * Freezing would make the page all-frozen. Have we already
+			 * emitted an FPI, or will do so anyway?
+			 */
+			if (hint_bit_fpi ||
+				((do_prune || do_hint) && XLogCheckBufferNeedsBackup(buffer)))
+			{
+				do_freeze = true;
+			}
+		}
+	}
 
 	/*
 	 * Validate the tuples we are considering freezing. We do this even if
@@ -636,85 +643,10 @@ heap_page_prune_and_freeze(Relation relation, Buffer buffer,
 	 * still may emit an FPI while setting the page hint bit later. But we
 	 * want to avoid doing the pre-freeze checks in a critical section.
 	 */
-	if (pagefrz &&
-		(pagefrz->freeze_required ||
-		 (whole_page_freezable && presult->nfrozen > 0)))
-		heap_pre_freeze_checks(buffer, frozen, presult->nfrozen);
-
-	/*
-	 * If we are going to modify the page contents anyway, we will have to
-	 * update more than hint bits.
-	 */
-	if (do_freeze || do_prune)
-		do_hint = false;
-
-	START_CRIT_SECTION();
-
-	/*
-	 * Update the page's pd_prune_xid field to either zero, or the lowest XID
-	 * of any soon-prunable tuple.
-	 */
-	((PageHeader) page)->pd_prune_xid = prstate.new_prune_xid;
-
-	/*
-	 * If pruning, freezing, or updating the hint bit, clear the "page is
-	 * full" flag if it is set since there's no point in repeating the
-	 * prune/defrag process until something else happens to the page.
-	 */
-	if (do_prune || do_freeze || do_hint)
-		PageClearFull(page);
-
-	/*
-	 * Apply the planned item changes, then repair page fragmentation, and
-	 * update the page's hint bit about whether it has free line pointers.
-	 */
-	if (do_prune)
-	{
-		heap_page_prune_execute(buffer,
-								prstate.redirected, prstate.nredirected,
-								prstate.nowdead, prstate.ndead,
-								prstate.nowunused, prstate.nunused);
-	}
-
-	/*
-	 * If we aren't pruning or freezing anything, but we updated pd_prune_xid,
-	 * this is a non-WAL-logged hint.
-	 */
-	if (do_hint)
-	{
-		MarkBufferDirtyHint(buffer, true);
-
-		/*
-		 * We may have decided not to opportunistically freeze above because
-		 * pruning would not emit an FPI. Now, however, if checksums are
-		 * enabled, setting the hint bit may have emitted an FPI. Check again
-		 * if we should freeze.
-		 */
-		hint_bit_fpi = fpi_before != pgWalUsage.wal_fpi;
-
-		if (hint_bit_fpi)
-			do_freeze = pagefrz &&
-				(pagefrz->freeze_required ||
-				 (whole_page_freezable && presult->nfrozen > 0));
-	}
-
 	if (do_freeze)
-	{
-		/*
-		 * We can use frz_conflict_horizon as our cutoff for conflicts when
-		 * the whole page is eligible to become all-frozen in the VM once
-		 * we're done with it.  Otherwise we generate a conservative cutoff by
-		 * stepping back from OldestXmin.
-		 */
-		if (!(presult->all_visible_except_removable && presult->all_frozen))
-		{
-			/* Avoids false conflicts when hot_standby_feedback in use */
-			presult->frz_conflict_horizon = pagefrz->cutoffs->OldestXmin;
-			TransactionIdRetreat(presult->frz_conflict_horizon);
-		}
-		heap_freeze_prepared_tuples(buffer, frozen, presult->nfrozen);
-	}
-	else if ((!pagefrz || !presult->all_frozen || presult->nfrozen > 0))
+		heap_pre_freeze_checks(buffer, prstate.frozen, presult->nfrozen);
+
+	if (!do_freeze && (!pagefrz || !presult->all_frozen || presult->nfrozen > 0))
 	{
 		/*
 		 * If we will neither freeze tuples on the page nor set the page all
@@ -725,162 +657,97 @@ heap_page_prune_and_freeze(Relation relation, Buffer buffer,
 		presult->nfrozen = 0;	/* avoid miscounts in instrumentation */
 	}
 
-	if (do_prune || do_freeze)
-		MarkBufferDirty(buffer);
+	START_CRIT_SECTION();
 
-	/*
-	 * Emit a WAL XLOG_HEAP2_PRUNE record showing what we did
-	 */
-	if ((do_prune || do_freeze) && RelationNeedsWAL(relation))
+	if (do_hint)
 	{
-		xl_heap_prune xlrec;
-		XLogRecPtr	recptr;
-		xlhp_freeze freeze;
-		xlhp_prune_items redirect,
-					dead,
-					unused;
-
-		int			nplans = 0;
-		xl_heap_freeze_plan plans[MaxHeapTuplesPerPage];
-		OffsetNumber frz_offsets[MaxHeapTuplesPerPage];
-
-		xlrec.flags = 0;
-
-		if (RelationIsAccessibleInLogicalDecoding(relation))
-			xlrec.flags |= XLHP_IS_CATALOG_REL;
-
 		/*
-		 * The snapshotConflictHorizon for the whole record should be the most
-		 * conservative of all the horizons calculated for any of the possible
-		 * modifications. If this record will prune tuples, any transactions
-		 * on the standby older than the youngest xmax of the most recently
-		 * removed tuple this record will prune will conflict. If this record
-		 * will freeze tuples, any transactions on the standby with xids older
-		 * than the youngest tuple this record will freeze will conflict.
+		 * Update the page's pd_prune_xid field to either zero, or the lowest XID
+		 * of any soon-prunable tuple.
 		 */
-		if (do_freeze)
-			xlrec.snapshotConflictHorizon = Max(prstate.snapshotConflictHorizon,
-												presult->frz_conflict_horizon);
-		else
-			xlrec.snapshotConflictHorizon = prstate.snapshotConflictHorizon;
+		((PageHeader) page)->pd_prune_xid = prstate.new_prune_xid;
 
 		/*
-		 * Prepare deduplicated representation for use in WAL record
-		 * Destructively sorts tuples array in-place.
+		 * Clear the "page is full" flag if it is set since there's no point
+		 * in repeating the prune/defrag process until something else happens
+		 * to the page.
 		 */
-		if (do_freeze)
-			nplans = heap_log_freeze_plan(frozen,
-										  presult->nfrozen, plans,
-										  frz_offsets);
-		if (nplans > 0)
-			xlrec.flags |= XLHP_HAS_FREEZE_PLANS;
-
-		XLogBeginInsert();
-		XLogRegisterData((char *) &xlrec, SizeOfHeapPrune);
-
-		XLogRegisterBuffer(0, buffer, REGBUF_STANDARD);
+		PageClearFull(page);
 
 		/*
-		 * The OffsetNumber arrays are not actually in the buffer, but we
-		 * pretend that they are.  When XLogInsert stores the whole buffer,
-		 * the offset arrays need not be stored too.
+		 * We only needed to update pd_prune_xid and clear the page-is-full
+		 * hint bit, this is a non-WAL-logged hint. If we will also freeze or
+		 * prune the page, we will mark the buffer dirty below.
 		 */
-		if (nplans > 0)
-		{
-			freeze = (xlhp_freeze)
-			{
-				.nplans = nplans
-			};
-
-			XLogRegisterBufData(0, (char *) &freeze, offsetof(xlhp_freeze, plans));
-
-			XLogRegisterBufData(0, (char *) plans,
-								sizeof(xl_heap_freeze_plan) * freeze.nplans);
-		}
-
-
-		if (prstate.nredirected > 0)
-		{
-			xlrec.flags |= XLHP_HAS_REDIRECTIONS;
-
-			redirect = (xlhp_prune_items)
-			{
-				.ntargets = prstate.nredirected
-			};
-
-			XLogRegisterBufData(0, (char *) &redirect,
-								offsetof(xlhp_prune_items, data));
-
-			XLogRegisterBufData(0, (char *) prstate.redirected,
-								sizeof(OffsetNumber[2]) * prstate.nredirected);
-		}
+		if (!do_freeze && !do_prune)
+			MarkBufferDirtyHint(buffer, true);
+	}
 
-		if (prstate.ndead > 0)
+	if (do_prune || do_freeze)
+	{
+		/*
+		 * Apply the planned item changes, then repair page fragmentation, and
+		 * update the page's hint bit about whether it has free line pointers.
+		 */
+		if (do_prune)
 		{
-			xlrec.flags |= XLHP_HAS_DEAD_ITEMS;
-
-			dead = (xlhp_prune_items)
-			{
-				.ntargets = prstate.ndead
-			};
-
-			XLogRegisterBufData(0, (char *) &dead,
-								offsetof(xlhp_prune_items, data));
-
-			XLogRegisterBufData(0, (char *) prstate.nowdead,
-								sizeof(OffsetNumber) * dead.ntargets);
+			heap_page_prune_execute(buffer,
+									prstate.redirected, prstate.nredirected,
+									prstate.nowdead, prstate.ndead,
+									prstate.nowunused, prstate.nunused);
 		}
 
-		if (prstate.nunused > 0)
+		if (do_freeze)
 		{
-			xlrec.flags |= XLHP_HAS_NOW_UNUSED_ITEMS;
-
-			unused = (xlhp_prune_items)
+			/*
+			 * We can use frz_conflict_horizon as our cutoff for conflicts when
+			 * the whole page is eligible to become all-frozen in the VM once
+			 * we're done with it.  Otherwise we generate a conservative cutoff by
+			 * stepping back from OldestXmin.
+			 */
+			if (!(presult->all_visible_except_removable && presult->all_frozen))
 			{
-				.ntargets = prstate.nunused
-			};
-
-			XLogRegisterBufData(0, (char *) &unused,
-								offsetof(xlhp_prune_items, data));
-
-			XLogRegisterBufData(0, (char *) prstate.nowunused,
-								sizeof(OffsetNumber) * unused.ntargets);
+				/* Avoids false conflicts when hot_standby_feedback in use */
+				presult->frz_conflict_horizon = pagefrz->cutoffs->OldestXmin;
+				TransactionIdRetreat(presult->frz_conflict_horizon);
+			}
+			heap_freeze_prepared_tuples(buffer, prstate.frozen, presult->nfrozen);
 		}
 
-		if (nplans > 0)
-			XLogRegisterBufData(0, (char *) frz_offsets,
-								sizeof(OffsetNumber) * presult->nfrozen);
-
-		recptr = XLogInsert(RM_HEAP2_ID, XLOG_HEAP2_PRUNE);
+		MarkBufferDirty(buffer);
 
-		PageSetLSN(BufferGetPage(buffer), recptr);
+		/*
+		 * Emit a WAL XLOG_HEAP2_PRUNE record showing what we did
+		 */
+		if (RelationNeedsWAL(relation))
+			log_heap_prune_and_freeze(relation, buffer, &prstate, presult);
 	}
 
 	END_CRIT_SECTION();
 
-	/* Caller won't update new_relfrozenxid and new_relminmxid */
-	if (!pagefrz)
-		return;
-
 	/*
+	 * Let caller know how it can updaterelfrozenxid and relminmxid
+	 *
 	 * If we will freeze tuples on the page or, even if we don't freeze tuples
 	 * on the page, if we will set the page all-frozen in the visibility map,
 	 * we can advance relfrozenxid and relminmxid to the values in
 	 * pagefrz->FreezePageRelfrozenXid and pagefrz->FreezePageRelminMxid.
 	 */
-	if (presult->all_frozen || presult->nfrozen > 0)
+	if (pagefrz)
 	{
-		presult->new_relfrozenxid = pagefrz->FreezePageRelfrozenXid;
-		presult->new_relminmxid = pagefrz->FreezePageRelminMxid;
-	}
-	else
-	{
-		presult->new_relfrozenxid = pagefrz->NoFreezePageRelfrozenXid;
-		presult->new_relminmxid = pagefrz->NoFreezePageRelminMxid;
+		if (presult->all_frozen || presult->nfrozen > 0)
+		{
+			presult->new_relfrozenxid = pagefrz->FreezePageRelfrozenXid;
+			presult->new_relminmxid = pagefrz->FreezePageRelminMxid;
+		}
+		else
+		{
+			presult->new_relfrozenxid = pagefrz->NoFreezePageRelfrozenXid;
+			presult->new_relminmxid = pagefrz->NoFreezePageRelminMxid;
+		}
 	}
 }
 
-
 /*
  * Perform visibility checks for heap pruning.
  */
@@ -1634,3 +1501,131 @@ heap_get_root_tuples(Page page, OffsetNumber *root_offsets)
 		}
 	}
 }
+
+
+static void
+log_heap_prune_and_freeze(Relation relation, Buffer buffer, PruneState *prstate, PruneFreezeResult *presult)
+{
+	xl_heap_prune xlrec;
+	XLogRecPtr	recptr;
+	xlhp_freeze freeze;
+	xlhp_prune_items redirect,
+		dead,
+		unused;
+
+	int			nplans = 0;
+	xl_heap_freeze_plan plans[MaxHeapTuplesPerPage];
+	OffsetNumber frz_offsets[MaxHeapTuplesPerPage];
+	bool		do_freeze = (presult->nfrozen > 0);
+
+	xlrec.flags = 0;
+
+	if (RelationIsAccessibleInLogicalDecoding(relation))
+		xlrec.flags |= XLHP_IS_CATALOG_REL;
+
+	/*
+	 * The snapshotConflictHorizon for the whole record should be the most
+	 * conservative of all the horizons calculated for any of the possible
+	 * modifications. If this record will prune tuples, any transactions
+	 * on the standby older than the youngest xmax of the most recently
+	 * removed tuple this record will prune will conflict. If this record
+	 * will freeze tuples, any transactions on the standby with xids older
+	 * than the youngest tuple this record will freeze will conflict.
+	 */
+	if (do_freeze)
+		xlrec.snapshotConflictHorizon = Max(prstate->snapshotConflictHorizon,
+											presult->frz_conflict_horizon);
+	else
+		xlrec.snapshotConflictHorizon = prstate->snapshotConflictHorizon;
+
+	/*
+	 * Prepare deduplicated representation for use in WAL record
+	 * Destructively sorts tuples array in-place.
+	 */
+	if (do_freeze)
+		nplans = heap_log_freeze_plan(prstate->frozen,
+									  presult->nfrozen, plans,
+									  frz_offsets);
+	if (nplans > 0)
+		xlrec.flags |= XLHP_HAS_FREEZE_PLANS;
+
+	XLogBeginInsert();
+	XLogRegisterData((char *) &xlrec, SizeOfHeapPrune);
+
+	XLogRegisterBuffer(0, buffer, REGBUF_STANDARD);
+
+	/*
+	 * The OffsetNumber arrays are not actually in the buffer, but we
+	 * pretend that they are.  When XLogInsert stores the whole buffer,
+	 * the offset arrays need not be stored too.
+	 */
+	if (nplans > 0)
+	{
+		freeze = (xlhp_freeze)
+			{
+				.nplans = nplans
+			};
+
+		XLogRegisterBufData(0, (char *) &freeze, offsetof(xlhp_freeze, plans));
+
+		XLogRegisterBufData(0, (char *) plans,
+							sizeof(xl_heap_freeze_plan) * freeze.nplans);
+	}
+
+
+	if (prstate->nredirected > 0)
+	{
+		xlrec.flags |= XLHP_HAS_REDIRECTIONS;
+
+		redirect = (xlhp_prune_items)
+			{
+				.ntargets = prstate->nredirected
+			};
+
+		XLogRegisterBufData(0, (char *) &redirect,
+							offsetof(xlhp_prune_items, data));
+
+		XLogRegisterBufData(0, (char *) prstate->redirected,
+							sizeof(OffsetNumber[2]) * prstate->nredirected);
+	}
+
+	if (prstate->ndead > 0)
+	{
+		xlrec.flags |= XLHP_HAS_DEAD_ITEMS;
+
+		dead = (xlhp_prune_items)
+			{
+				.ntargets = prstate->ndead
+			};
+
+		XLogRegisterBufData(0, (char *) &dead,
+							offsetof(xlhp_prune_items, data));
+
+		XLogRegisterBufData(0, (char *) prstate->nowdead,
+							sizeof(OffsetNumber) * dead.ntargets);
+	}
+
+	if (prstate->nunused > 0)
+	{
+		xlrec.flags |= XLHP_HAS_NOW_UNUSED_ITEMS;
+
+		unused = (xlhp_prune_items)
+			{
+				.ntargets = prstate->nunused
+			};
+
+		XLogRegisterBufData(0, (char *) &unused,
+							offsetof(xlhp_prune_items, data));
+
+		XLogRegisterBufData(0, (char *) prstate->nowunused,
+							sizeof(OffsetNumber) * unused.ntargets);
+	}
+
+	if (nplans > 0)
+		XLogRegisterBufData(0, (char *) frz_offsets,
+							sizeof(OffsetNumber) * presult->nfrozen);
+
+	recptr = XLogInsert(RM_HEAP2_ID, XLOG_HEAP2_PRUNE);
+
+	PageSetLSN(BufferGetPage(buffer), recptr);
+}
-- 
2.39.2