public inbox for [email protected]  
help / color / mirror / Atom feed
Re: Minimal logical decoding on standbys
11+ messages / 3 participants
[nested] [flat]

* Re: Minimal logical decoding on standbys
@ 2022-02-25 09:34  Drouvot, Bertrand <[email protected]>
  0 siblings, 1 reply; 11+ messages in thread

From: Drouvot, Bertrand @ 2022-02-25 09:34 UTC (permalink / raw)
  To: Andres Freund <[email protected]>; Robert Haas <[email protected]>; +Cc: Alvaro Herrera <[email protected]>; Ibrar Ahmed <[email protected]>; Amit Khandekar <[email protected]>; [email protected]; tushar <[email protected]>; Rahila Syed <[email protected]>; pgsql-hackers

Hi,

On 10/28/21 11:07 PM, Andres Freund wrote:
> Hi,
>
> On 2021-10-28 16:24:22 -0400, Robert Haas wrote:
>> On Wed, Oct 27, 2021 at 2:56 AM Drouvot, Bertrand <[email protected]> wrote:
>>> So you have in mind to check for XLogLogicalInfoActive() first, and if true, then open the relation and call
>>> RelationIsAccessibleInLogicalDecoding()?
>> I think 0001 is utterly unacceptable. We cannot add calls to
>> table_open() in low-level functions like this. Suppose for example
>> that _bt_getbuf() calls _bt_log_reuse_page() which with 0001 applied
>> would call get_rel_logical_catalog(). _bt_getbuf() will have acquired
>> a buffer lock on the page. The idea that it's safe to call
>> table_open() while holding a buffer lock cannot be taken seriously.
> Yes - that's pretty clearly a deadlock hazard. It shouldn't too hard to fix, I
> think. Possibly a bit more verbose than nice, but...
>
> Alternatively we could propagate the information whether a relcache entry is
> for a catalog from the table to the index. Then we'd not need to change the
> btree code to pass the table down.

+1 for the idea of propagating to the index. If that sounds good to you 
too, I can try to have a look at it.

Thanks Robert and Andres for the feedbacks you have done on the various 
sub-patches.

I've now in mind to work sub patch by sub patch (starting with 0001 
then) and move to the next one once we agree that the current one is 
"ready".

I think that could help us to get this new feature moving forward more 
"easily", what do you think?

Thanks

Bertrand







^ permalink  raw  reply  [nested|flat] 11+ messages in thread

* Re: Minimal logical decoding on standbys
@ 2022-06-30 08:49  Drouvot, Bertrand <[email protected]>
  parent: Drouvot, Bertrand <[email protected]>
  0 siblings, 1 reply; 11+ messages in thread

From: Drouvot, Bertrand @ 2022-06-30 08:49 UTC (permalink / raw)
  To: Andres Freund <[email protected]>; Robert Haas <[email protected]>; +Cc: Alvaro Herrera <[email protected]>; Ibrar Ahmed <[email protected]>; Amit Khandekar <[email protected]>; [email protected]; tushar <[email protected]>; Rahila Syed <[email protected]>; pgsql-hackers

Hi,

On 2/25/22 10:34 AM, Drouvot, Bertrand wrote:
> Hi,
>
> On 10/28/21 11:07 PM, Andres Freund wrote:
>> Hi,
>>
>> On 2021-10-28 16:24:22 -0400, Robert Haas wrote:
>>> On Wed, Oct 27, 2021 at 2:56 AM Drouvot, Bertrand 
>>> <[email protected]> wrote:
>>>> So you have in mind to check for XLogLogicalInfoActive() first, and 
>>>> if true, then open the relation and call
>>>> RelationIsAccessibleInLogicalDecoding()?
>>> I think 0001 is utterly unacceptable. We cannot add calls to
>>> table_open() in low-level functions like this. Suppose for example
>>> that _bt_getbuf() calls _bt_log_reuse_page() which with 0001 applied
>>> would call get_rel_logical_catalog(). _bt_getbuf() will have acquired
>>> a buffer lock on the page. The idea that it's safe to call
>>> table_open() while holding a buffer lock cannot be taken seriously.
>> Yes - that's pretty clearly a deadlock hazard. It shouldn't too hard 
>> to fix, I
>> think. Possibly a bit more verbose than nice, but...
>>
>> Alternatively we could propagate the information whether a relcache 
>> entry is
>> for a catalog from the table to the index. Then we'd not need to 
>> change the
>> btree code to pass the table down.
>
> +1 for the idea of propagating to the index. If that sounds good to 
> you too, I can try to have a look at it.
>
> Thanks Robert and Andres for the feedbacks you have done on the 
> various sub-patches.
>
> I've now in mind to work sub patch by sub patch (starting with 0001 
> then) and move to the next one once we agree that the current one is 
> "ready".
>
> I think that could help us to get this new feature moving forward more 
> "easily", what do you think?
>
> Thanks
>
> Bertrand
>
I'm going to re-create a CF entry for it, as:

- It seems there is a clear interest for the feature (given the time 
already spend on it and the number of people that worked on)

- I've in mind to resume working on it

- It would give more visibility in case others want to jump in

Hope that makes sense,

Thanks,

Bertrand






^ permalink  raw  reply  [nested|flat] 11+ messages in thread

* Re: Minimal logical decoding on standbys
@ 2022-07-01 20:03  Ibrar Ahmed <[email protected]>
  parent: Drouvot, Bertrand <[email protected]>
  0 siblings, 1 reply; 11+ messages in thread

From: Ibrar Ahmed @ 2022-07-01 20:03 UTC (permalink / raw)
  To: Drouvot, Bertrand <[email protected]>; +Cc: Andres Freund <[email protected]>; Robert Haas <[email protected]>; Alvaro Herrera <[email protected]>; Amit Khandekar <[email protected]>; [email protected]; tushar <[email protected]>; Rahila Syed <[email protected]>; pgsql-hackers

On Thu, Jun 30, 2022 at 1:49 PM Drouvot, Bertrand <[email protected]>
wrote:

> Hi,
>
> On 2/25/22 10:34 AM, Drouvot, Bertrand wrote:
> > Hi,
> >
> > On 10/28/21 11:07 PM, Andres Freund wrote:
> >> Hi,
> >>
> >> On 2021-10-28 16:24:22 -0400, Robert Haas wrote:
> >>> On Wed, Oct 27, 2021 at 2:56 AM Drouvot, Bertrand
> >>> <[email protected]> wrote:
> >>>> So you have in mind to check for XLogLogicalInfoActive() first, and
> >>>> if true, then open the relation and call
> >>>> RelationIsAccessibleInLogicalDecoding()?
> >>> I think 0001 is utterly unacceptable. We cannot add calls to
> >>> table_open() in low-level functions like this. Suppose for example
> >>> that _bt_getbuf() calls _bt_log_reuse_page() which with 0001 applied
> >>> would call get_rel_logical_catalog(). _bt_getbuf() will have acquired
> >>> a buffer lock on the page. The idea that it's safe to call
> >>> table_open() while holding a buffer lock cannot be taken seriously.
> >> Yes - that's pretty clearly a deadlock hazard. It shouldn't too hard
> >> to fix, I
> >> think. Possibly a bit more verbose than nice, but...
> >>
> >> Alternatively we could propagate the information whether a relcache
> >> entry is
> >> for a catalog from the table to the index. Then we'd not need to
> >> change the
> >> btree code to pass the table down.
> >
> > +1 for the idea of propagating to the index. If that sounds good to
> > you too, I can try to have a look at it.
> >
> > Thanks Robert and Andres for the feedbacks you have done on the
> > various sub-patches.
> >
> > I've now in mind to work sub patch by sub patch (starting with 0001
> > then) and move to the next one once we agree that the current one is
> > "ready".
> >
> > I think that could help us to get this new feature moving forward more
> > "easily", what do you think?
> >
> > Thanks
> >
> > Bertrand
> >
> I'm going to re-create a CF entry for it, as:
>
> - It seems there is a clear interest for the feature (given the time
> already spend on it and the number of people that worked on)
>
> - I've in mind to resume working on it
>
> I have already done some research on that, I can definitely look at it.


> - It would give more visibility in case others want to jump in
>
> Hope that makes sense,
>
> Thanks,
>
> Bertrand
>
>

-- 
Ibrar Ahmed


^ permalink  raw  reply  [nested|flat] 11+ messages in thread

* Re: Minimal logical decoding on standbys
@ 2022-07-04 13:12  Drouvot, Bertrand <[email protected]>
  parent: Ibrar Ahmed <[email protected]>
  0 siblings, 1 reply; 11+ messages in thread

From: Drouvot, Bertrand @ 2022-07-04 13:12 UTC (permalink / raw)
  To: Ibrar Ahmed <[email protected]>; +Cc: Andres Freund <[email protected]>; Robert Haas <[email protected]>; Alvaro Herrera <[email protected]>; Amit Khandekar <[email protected]>; [email protected]; tushar <[email protected]>; Rahila Syed <[email protected]>; pgsql-hackers

Hi,

On 7/1/22 10:03 PM, Ibrar Ahmed wrote:
>
> On Thu, Jun 30, 2022 at 1:49 PM Drouvot, Bertrand 
> <[email protected]> wrote:
>
>     I'm going to re-create a CF entry for it, as:
>
>     - It seems there is a clear interest for the feature (given the time
>     already spend on it and the number of people that worked on)
>
>     - I've in mind to resume working on it
>
> I have already done some research on that, I can definitely look at it.
>
Thanks!

This feature proposal is currently made of 5 sub-patches:

0001: Add info in WAL records in preparation for logical slot conflict 
handling
0002: Handle logical slot conflicts on standby
0003: Allow logical decoding on standby.
0004: New TAP test for logical decoding on standby
0005: Doc changes describing details about logical decoding

I suggest that we focus on one sub-patch at a time.

I'll start with 0001 and come back with a rebase addressing Andres and 
Robert's previous comments.

Sounds good to you?

Thanks

-- 
Bertrand Drouvot
Amazon Web Services:https://aws.amazon.com


^ permalink  raw  reply  [nested|flat] 11+ messages in thread

* Re: Minimal logical decoding on standbys
@ 2022-07-04 13:17  Ibrar Ahmed <[email protected]>
  parent: Drouvot, Bertrand <[email protected]>
  0 siblings, 0 replies; 11+ messages in thread

From: Ibrar Ahmed @ 2022-07-04 13:17 UTC (permalink / raw)
  To: Drouvot, Bertrand <[email protected]>; +Cc: Andres Freund <[email protected]>; Robert Haas <[email protected]>; Alvaro Herrera <[email protected]>; Amit Khandekar <[email protected]>; [email protected]; tushar <[email protected]>; Rahila Syed <[email protected]>; pgsql-hackers

On Mon, Jul 4, 2022 at 6:12 PM Drouvot, Bertrand <[email protected]>
wrote:

> Hi,
> On 7/1/22 10:03 PM, Ibrar Ahmed wrote:
>
>
> On Thu, Jun 30, 2022 at 1:49 PM Drouvot, Bertrand <[email protected]>
> wrote:
>
>> I'm going to re-create a CF entry for it, as:
>>
>> - It seems there is a clear interest for the feature (given the time
>> already spend on it and the number of people that worked on)
>>
>> - I've in mind to resume working on it
>>
>> I have already done some research on that, I can definitely look at it.
>
> Thanks!
>
> This feature proposal is currently made of 5 sub-patches:
>
> 0001: Add info in WAL records in preparation for logical slot conflict
> handling
> 0002: Handle logical slot conflicts on standby
> 0003: Allow logical decoding on standby.
> 0004: New TAP test for logical decoding on standby
> 0005: Doc changes describing details about logical decoding
>
> I suggest that we focus on one sub-patch at a time.
>
> I'll start with 0001 and come back with a rebase addressing Andres and
> Robert's previous comments.
>
> Sounds good to you?
>
> Thanks
>
> --
> Bertrand Drouvot
> Amazon Web Services: https://aws.amazon.com
>
> That's great I am looking at "0002: Handle logical slot conflicts on
standby".


-- 
Ibrar Ahmed


^ permalink  raw  reply  [nested|flat] 11+ messages in thread

* [PATCH v4 12/19] Remove heap_freeze_execute_prepared()
@ 2024-01-07 22:03  Melanie Plageman <[email protected]>
  0 siblings, 0 replies; 11+ messages in thread

From: Melanie Plageman @ 2024-01-07 22:03 UTC (permalink / raw)

In order to merge freeze and prune records, the execution of tuple
freezing and the WAL logging of the changes to the page must be
separated so that the WAL logging can be combined with prune WAL
logging. This commit makes a helper for the tuple freezing and then
inlines the contents of heap_freeze_execute_prepared() where it is
called in heap_page_prune().
---
 src/backend/access/heap/heapam.c    | 79 +++++------------------------
 src/backend/access/heap/pruneheap.c | 51 +++++++++++++++++--
 src/include/access/heapam.h         | 31 ++++++-----
 3 files changed, 77 insertions(+), 84 deletions(-)

diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.c
index 16e3f2520a4..e47b56e7856 100644
--- a/src/backend/access/heap/heapam.c
+++ b/src/backend/access/heap/heapam.c
@@ -91,9 +91,6 @@ static void compute_new_xmax_infomask(TransactionId xmax, uint16 old_infomask,
 static TM_Result heap_lock_updated_tuple(Relation rel, HeapTuple tuple,
 										 ItemPointer ctid, TransactionId xid,
 										 LockTupleMode mode);
-static int	heap_log_freeze_plan(HeapTupleFreeze *tuples, int ntuples,
-								 xl_heap_freeze_plan *plans_out,
-								 OffsetNumber *offsets_out);
 static void GetMultiXactIdHintBits(MultiXactId multi, uint16 *new_infomask,
 								   uint16 *new_infomask2);
 static TransactionId MultiXactIdGetUpdateXid(TransactionId xmax,
@@ -6343,9 +6340,9 @@ FreezeMultiXactId(MultiXactId multi, uint16 t_infomask,
  * XIDs or MultiXactIds that will need to be processed by a future VACUUM.
  *
  * VACUUM caller must assemble HeapTupleFreeze freeze plan entries for every
- * tuple that we returned true for, and call heap_freeze_execute_prepared to
- * execute freezing.  Caller must initialize pagefrz fields for page as a
- * whole before first call here for each heap page.
+ * tuple that we returned true for, and then execute freezing.  Caller must
+ * initialize pagefrz fields for page as a whole before first call here for
+ * each heap page.
  *
  * VACUUM caller decides on whether or not to freeze the page as a whole.
  * We'll often prepare freeze plans for a page that caller just discards.
@@ -6659,8 +6656,8 @@ heap_execute_freeze_tuple(HeapTupleHeader tuple, HeapTupleFreeze *frz)
 }
 
 /*
-* Perform xmin/xmax XID status sanity checks before calling
-* heap_freeze_execute_prepared().
+* Perform xmin/xmax XID status sanity checks before actually executing freeze
+* plans.
 *
 * heap_prepare_freeze_tuple doesn't perform these checks directly because
 * pg_xact lookups are relatively expensive.  They shouldn't be repeated
@@ -6713,30 +6710,17 @@ heap_pre_freeze_checks(Buffer buffer,
 }
 
 /*
- * heap_freeze_execute_prepared
- *
- * Executes freezing of one or more heap tuples on a page on behalf of caller.
- * Caller passes an array of tuple plans from heap_prepare_freeze_tuple.
- * Caller must set 'offset' in each plan for us.  Note that we destructively
- * sort caller's tuples array in-place, so caller had better be done with it.
- *
- * WAL-logs the changes so that VACUUM can advance the rel's relfrozenxid
- * later on without any risk of unsafe pg_xact lookups, even following a hard
- * crash (or when querying from a standby).  We represent freezing by setting
- * infomask bits in tuple headers, but this shouldn't be thought of as a hint.
- * See section on buffer access rules in src/backend/storage/buffer/README.
+ * Helper which executes freezing of one or more heap tuples on a page on
+ * behalf of caller. Caller passes an array of tuple plans from
+ * heap_prepare_freeze_tuple. Caller must set 'offset' in each plan for us.
+ * Must be called in a critical section that also marks the buffer dirty and,
+ * if needed, emits WAL.
  */
 void
-heap_freeze_execute_prepared(Relation rel, Buffer buffer,
-							 TransactionId snapshotConflictHorizon,
-							 HeapTupleFreeze *tuples, int ntuples)
+heap_freeze_prepared_tuples(Buffer buffer, HeapTupleFreeze *tuples, int ntuples)
 {
 	Page		page = BufferGetPage(buffer);
 
-	Assert(ntuples > 0);
-
-	START_CRIT_SECTION();
-
 	for (int i = 0; i < ntuples; i++)
 	{
 		HeapTupleFreeze *frz = tuples + i;
@@ -6746,45 +6730,6 @@ heap_freeze_execute_prepared(Relation rel, Buffer buffer,
 		htup = (HeapTupleHeader) PageGetItem(page, itemid);
 		heap_execute_freeze_tuple(htup, frz);
 	}
-
-	MarkBufferDirty(buffer);
-
-	/* Now WAL-log freezing if necessary */
-	if (RelationNeedsWAL(rel))
-	{
-		xl_heap_freeze_plan plans[MaxHeapTuplesPerPage];
-		OffsetNumber offsets[MaxHeapTuplesPerPage];
-		int			nplans;
-		xl_heap_freeze_page xlrec;
-		XLogRecPtr	recptr;
-
-		/* Prepare deduplicated representation for use in WAL record */
-		nplans = heap_log_freeze_plan(tuples, ntuples, plans, offsets);
-
-		xlrec.snapshotConflictHorizon = snapshotConflictHorizon;
-		xlrec.isCatalogRel = RelationIsAccessibleInLogicalDecoding(rel);
-		xlrec.nplans = nplans;
-
-		XLogBeginInsert();
-		XLogRegisterData((char *) &xlrec, SizeOfHeapFreezePage);
-
-		/*
-		 * The freeze plan array and offset array are not actually in the
-		 * buffer, but pretend that they are.  When XLogInsert stores the
-		 * whole buffer, the arrays need not be stored too.
-		 */
-		XLogRegisterBuffer(0, buffer, REGBUF_STANDARD);
-		XLogRegisterBufData(0, (char *) plans,
-							nplans * sizeof(xl_heap_freeze_plan));
-		XLogRegisterBufData(0, (char *) offsets,
-							ntuples * sizeof(OffsetNumber));
-
-		recptr = XLogInsert(RM_HEAP2_ID, XLOG_HEAP2_FREEZE_PAGE);
-
-		PageSetLSN(page, recptr);
-	}
-
-	END_CRIT_SECTION();
 }
 
 /*
@@ -6874,7 +6819,7 @@ heap_log_freeze_new_plan(xl_heap_freeze_plan *plan, HeapTupleFreeze *frz)
  * (actually there is one array per freeze plan, but that's not of immediate
  * concern to our caller).
  */
-static int
+int
 heap_log_freeze_plan(HeapTupleFreeze *tuples, int ntuples,
 					 xl_heap_freeze_plan *plans_out,
 					 OffsetNumber *offsets_out)
diff --git a/src/backend/access/heap/pruneheap.c b/src/backend/access/heap/pruneheap.c
index 87f99497865..7bd479cfd4e 100644
--- a/src/backend/access/heap/pruneheap.c
+++ b/src/backend/access/heap/pruneheap.c
@@ -640,10 +640,53 @@ heap_page_prune_and_freeze(Relation relation, Buffer buffer,
 
 	if (do_freeze)
 	{
-		/* Execute all freeze plans for page as a single atomic action */
-		heap_freeze_execute_prepared(relation, buffer,
-									 presult->frz_conflict_horizon,
-									 prstate.frozen, presult->nfrozen);
+		START_CRIT_SECTION();
+
+		Assert(presult->nfrozen > 0);
+
+		heap_freeze_prepared_tuples(buffer, prstate.frozen, presult->nfrozen);
+
+		MarkBufferDirty(buffer);
+
+		/* Now WAL-log freezing if necessary */
+		if (RelationNeedsWAL(relation))
+		{
+			xl_heap_freeze_plan plans[MaxHeapTuplesPerPage];
+			OffsetNumber offsets[MaxHeapTuplesPerPage];
+			int			nplans;
+			xl_heap_freeze_page xlrec;
+			XLogRecPtr	recptr;
+
+			/*
+			 * Prepare deduplicated representation for use in WAL record
+			 * Destructively sorts tuples array in-place.
+			 */
+			nplans = heap_log_freeze_plan(prstate.frozen, presult->nfrozen, plans, offsets);
+
+			xlrec.snapshotConflictHorizon = presult->frz_conflict_horizon;
+			xlrec.isCatalogRel = RelationIsAccessibleInLogicalDecoding(relation);
+			xlrec.nplans = nplans;
+
+			XLogBeginInsert();
+			XLogRegisterData((char *) &xlrec, SizeOfHeapFreezePage);
+
+			/*
+			 * The freeze plan array and offset array are not actually in the
+			 * buffer, but pretend that they are.  When XLogInsert stores the
+			 * whole buffer, the arrays need not be stored too.
+			 */
+			XLogRegisterBuffer(0, buffer, REGBUF_STANDARD);
+			XLogRegisterBufData(0, (char *) plans,
+								nplans * sizeof(xl_heap_freeze_plan));
+			XLogRegisterBufData(0, (char *) offsets,
+								presult->nfrozen * sizeof(OffsetNumber));
+
+			recptr = XLogInsert(RM_HEAP2_ID, XLOG_HEAP2_FREEZE_PAGE);
+
+			PageSetLSN(page, recptr);
+		}
+
+		END_CRIT_SECTION();
 	}
 	else if (!pagefrz || !presult->all_frozen || presult->nfrozen > 0)
 	{
diff --git a/src/include/access/heapam.h b/src/include/access/heapam.h
index 02e33f213e1..321a46185e1 100644
--- a/src/include/access/heapam.h
+++ b/src/include/access/heapam.h
@@ -14,6 +14,7 @@
 #ifndef HEAPAM_H
 #define HEAPAM_H
 
+#include "access/heapam_xlog.h"
 #include "access/relation.h"	/* for backward compatibility */
 #include "access/relscan.h"
 #include "access/sdir.h"
@@ -101,8 +102,8 @@ typedef enum
 } HTSV_Result;
 
 /*
- * heap_prepare_freeze_tuple may request that heap_freeze_execute_prepared
- * check any tuple's to-be-frozen xmin and/or xmax status using pg_xact
+ * heap_prepare_freeze_tuple may request that any tuple's to-be-frozen xmin
+ * and/or xmax status is checked using pg_xact during freezing execution.
  */
 #define		HEAP_FREEZE_CHECK_XMIN_COMMITTED	0x01
 #define		HEAP_FREEZE_CHECK_XMAX_ABORTED		0x02
@@ -154,14 +155,14 @@ typedef struct HeapPageFreeze
 	/*
 	 * "Freeze" NewRelfrozenXid/NewRelminMxid trackers.
 	 *
-	 * Trackers used when heap_freeze_execute_prepared freezes, or when there
-	 * are zero freeze plans for a page.  It is always valid for vacuumlazy.c
-	 * to freeze any page, by definition.  This even includes pages that have
-	 * no tuples with storage to consider in the first place.  That way the
-	 * 'totally_frozen' results from heap_prepare_freeze_tuple can always be
-	 * used in the same way, even when no freeze plans need to be executed to
-	 * "freeze the page".  Only the "freeze" path needs to consider the need
-	 * to set pages all-frozen in the visibility map under this scheme.
+	 * Trackers used when tuples will be frozen, or when there are zero freeze
+	 * plans for a page.  It is always valid for vacuumlazy.c to freeze any
+	 * page, by definition.  This even includes pages that have no tuples with
+	 * storage to consider in the first place.  That way the 'totally_frozen'
+	 * results from heap_prepare_freeze_tuple can always be used in the same
+	 * way, even when no freeze plans need to be executed to "freeze the
+	 * page". Only the "freeze" path needs to consider the need to set pages
+	 * all-frozen in the visibility map under this scheme.
 	 *
 	 * When we freeze a page, we generally freeze all XIDs < OldestXmin, only
 	 * leaving behind XIDs that are ineligible for freezing, if any.  And so
@@ -315,12 +316,16 @@ extern bool heap_prepare_freeze_tuple(HeapTupleHeader tuple,
 
 extern void heap_pre_freeze_checks(Buffer buffer,
 								   HeapTupleFreeze *tuples, int ntuples);
-extern void heap_freeze_execute_prepared(Relation rel, Buffer buffer,
-										 TransactionId snapshotConflictHorizon,
-										 HeapTupleFreeze *tuples, int ntuples);
+
+extern void heap_freeze_prepared_tuples(Buffer buffer,
+										HeapTupleFreeze *tuples, int ntuples);
 extern bool heap_freeze_tuple(HeapTupleHeader tuple,
 							  TransactionId relfrozenxid, TransactionId relminmxid,
 							  TransactionId FreezeLimit, TransactionId MultiXactCutoff);
+
+extern int	heap_log_freeze_plan(HeapTupleFreeze *tuples, int ntuples,
+								 xl_heap_freeze_plan *plans_out,
+								 OffsetNumber *offsets_out);
 extern bool heap_tuple_should_freeze(HeapTupleHeader tuple,
 									 const struct VacuumCutoffs *cutoffs,
 									 TransactionId *NoFreezePageRelfrozenXid,
-- 
2.40.1


--tez7m2a73jtztiij
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
	filename="v4-0013-Merge-prune-and-freeze-records.patch"



^ permalink  raw  reply  [nested|flat] 11+ messages in thread

* [PATCH v4 12/19] Remove heap_freeze_execute_prepared()
@ 2024-01-07 22:03  Melanie Plageman <[email protected]>
  0 siblings, 0 replies; 11+ messages in thread

From: Melanie Plageman @ 2024-01-07 22:03 UTC (permalink / raw)

In order to merge freeze and prune records, the execution of tuple
freezing and the WAL logging of the changes to the page must be
separated so that the WAL logging can be combined with prune WAL
logging. This commit makes a helper for the tuple freezing and then
inlines the contents of heap_freeze_execute_prepared() where it is
called in heap_page_prune().
---
 src/backend/access/heap/heapam.c    | 79 +++++------------------------
 src/backend/access/heap/pruneheap.c | 51 +++++++++++++++++--
 src/include/access/heapam.h         | 31 ++++++-----
 3 files changed, 77 insertions(+), 84 deletions(-)

diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.c
index 16e3f2520a4..e47b56e7856 100644
--- a/src/backend/access/heap/heapam.c
+++ b/src/backend/access/heap/heapam.c
@@ -91,9 +91,6 @@ static void compute_new_xmax_infomask(TransactionId xmax, uint16 old_infomask,
 static TM_Result heap_lock_updated_tuple(Relation rel, HeapTuple tuple,
 										 ItemPointer ctid, TransactionId xid,
 										 LockTupleMode mode);
-static int	heap_log_freeze_plan(HeapTupleFreeze *tuples, int ntuples,
-								 xl_heap_freeze_plan *plans_out,
-								 OffsetNumber *offsets_out);
 static void GetMultiXactIdHintBits(MultiXactId multi, uint16 *new_infomask,
 								   uint16 *new_infomask2);
 static TransactionId MultiXactIdGetUpdateXid(TransactionId xmax,
@@ -6343,9 +6340,9 @@ FreezeMultiXactId(MultiXactId multi, uint16 t_infomask,
  * XIDs or MultiXactIds that will need to be processed by a future VACUUM.
  *
  * VACUUM caller must assemble HeapTupleFreeze freeze plan entries for every
- * tuple that we returned true for, and call heap_freeze_execute_prepared to
- * execute freezing.  Caller must initialize pagefrz fields for page as a
- * whole before first call here for each heap page.
+ * tuple that we returned true for, and then execute freezing.  Caller must
+ * initialize pagefrz fields for page as a whole before first call here for
+ * each heap page.
  *
  * VACUUM caller decides on whether or not to freeze the page as a whole.
  * We'll often prepare freeze plans for a page that caller just discards.
@@ -6659,8 +6656,8 @@ heap_execute_freeze_tuple(HeapTupleHeader tuple, HeapTupleFreeze *frz)
 }
 
 /*
-* Perform xmin/xmax XID status sanity checks before calling
-* heap_freeze_execute_prepared().
+* Perform xmin/xmax XID status sanity checks before actually executing freeze
+* plans.
 *
 * heap_prepare_freeze_tuple doesn't perform these checks directly because
 * pg_xact lookups are relatively expensive.  They shouldn't be repeated
@@ -6713,30 +6710,17 @@ heap_pre_freeze_checks(Buffer buffer,
 }
 
 /*
- * heap_freeze_execute_prepared
- *
- * Executes freezing of one or more heap tuples on a page on behalf of caller.
- * Caller passes an array of tuple plans from heap_prepare_freeze_tuple.
- * Caller must set 'offset' in each plan for us.  Note that we destructively
- * sort caller's tuples array in-place, so caller had better be done with it.
- *
- * WAL-logs the changes so that VACUUM can advance the rel's relfrozenxid
- * later on without any risk of unsafe pg_xact lookups, even following a hard
- * crash (or when querying from a standby).  We represent freezing by setting
- * infomask bits in tuple headers, but this shouldn't be thought of as a hint.
- * See section on buffer access rules in src/backend/storage/buffer/README.
+ * Helper which executes freezing of one or more heap tuples on a page on
+ * behalf of caller. Caller passes an array of tuple plans from
+ * heap_prepare_freeze_tuple. Caller must set 'offset' in each plan for us.
+ * Must be called in a critical section that also marks the buffer dirty and,
+ * if needed, emits WAL.
  */
 void
-heap_freeze_execute_prepared(Relation rel, Buffer buffer,
-							 TransactionId snapshotConflictHorizon,
-							 HeapTupleFreeze *tuples, int ntuples)
+heap_freeze_prepared_tuples(Buffer buffer, HeapTupleFreeze *tuples, int ntuples)
 {
 	Page		page = BufferGetPage(buffer);
 
-	Assert(ntuples > 0);
-
-	START_CRIT_SECTION();
-
 	for (int i = 0; i < ntuples; i++)
 	{
 		HeapTupleFreeze *frz = tuples + i;
@@ -6746,45 +6730,6 @@ heap_freeze_execute_prepared(Relation rel, Buffer buffer,
 		htup = (HeapTupleHeader) PageGetItem(page, itemid);
 		heap_execute_freeze_tuple(htup, frz);
 	}
-
-	MarkBufferDirty(buffer);
-
-	/* Now WAL-log freezing if necessary */
-	if (RelationNeedsWAL(rel))
-	{
-		xl_heap_freeze_plan plans[MaxHeapTuplesPerPage];
-		OffsetNumber offsets[MaxHeapTuplesPerPage];
-		int			nplans;
-		xl_heap_freeze_page xlrec;
-		XLogRecPtr	recptr;
-
-		/* Prepare deduplicated representation for use in WAL record */
-		nplans = heap_log_freeze_plan(tuples, ntuples, plans, offsets);
-
-		xlrec.snapshotConflictHorizon = snapshotConflictHorizon;
-		xlrec.isCatalogRel = RelationIsAccessibleInLogicalDecoding(rel);
-		xlrec.nplans = nplans;
-
-		XLogBeginInsert();
-		XLogRegisterData((char *) &xlrec, SizeOfHeapFreezePage);
-
-		/*
-		 * The freeze plan array and offset array are not actually in the
-		 * buffer, but pretend that they are.  When XLogInsert stores the
-		 * whole buffer, the arrays need not be stored too.
-		 */
-		XLogRegisterBuffer(0, buffer, REGBUF_STANDARD);
-		XLogRegisterBufData(0, (char *) plans,
-							nplans * sizeof(xl_heap_freeze_plan));
-		XLogRegisterBufData(0, (char *) offsets,
-							ntuples * sizeof(OffsetNumber));
-
-		recptr = XLogInsert(RM_HEAP2_ID, XLOG_HEAP2_FREEZE_PAGE);
-
-		PageSetLSN(page, recptr);
-	}
-
-	END_CRIT_SECTION();
 }
 
 /*
@@ -6874,7 +6819,7 @@ heap_log_freeze_new_plan(xl_heap_freeze_plan *plan, HeapTupleFreeze *frz)
  * (actually there is one array per freeze plan, but that's not of immediate
  * concern to our caller).
  */
-static int
+int
 heap_log_freeze_plan(HeapTupleFreeze *tuples, int ntuples,
 					 xl_heap_freeze_plan *plans_out,
 					 OffsetNumber *offsets_out)
diff --git a/src/backend/access/heap/pruneheap.c b/src/backend/access/heap/pruneheap.c
index 87f99497865..7bd479cfd4e 100644
--- a/src/backend/access/heap/pruneheap.c
+++ b/src/backend/access/heap/pruneheap.c
@@ -640,10 +640,53 @@ heap_page_prune_and_freeze(Relation relation, Buffer buffer,
 
 	if (do_freeze)
 	{
-		/* Execute all freeze plans for page as a single atomic action */
-		heap_freeze_execute_prepared(relation, buffer,
-									 presult->frz_conflict_horizon,
-									 prstate.frozen, presult->nfrozen);
+		START_CRIT_SECTION();
+
+		Assert(presult->nfrozen > 0);
+
+		heap_freeze_prepared_tuples(buffer, prstate.frozen, presult->nfrozen);
+
+		MarkBufferDirty(buffer);
+
+		/* Now WAL-log freezing if necessary */
+		if (RelationNeedsWAL(relation))
+		{
+			xl_heap_freeze_plan plans[MaxHeapTuplesPerPage];
+			OffsetNumber offsets[MaxHeapTuplesPerPage];
+			int			nplans;
+			xl_heap_freeze_page xlrec;
+			XLogRecPtr	recptr;
+
+			/*
+			 * Prepare deduplicated representation for use in WAL record
+			 * Destructively sorts tuples array in-place.
+			 */
+			nplans = heap_log_freeze_plan(prstate.frozen, presult->nfrozen, plans, offsets);
+
+			xlrec.snapshotConflictHorizon = presult->frz_conflict_horizon;
+			xlrec.isCatalogRel = RelationIsAccessibleInLogicalDecoding(relation);
+			xlrec.nplans = nplans;
+
+			XLogBeginInsert();
+			XLogRegisterData((char *) &xlrec, SizeOfHeapFreezePage);
+
+			/*
+			 * The freeze plan array and offset array are not actually in the
+			 * buffer, but pretend that they are.  When XLogInsert stores the
+			 * whole buffer, the arrays need not be stored too.
+			 */
+			XLogRegisterBuffer(0, buffer, REGBUF_STANDARD);
+			XLogRegisterBufData(0, (char *) plans,
+								nplans * sizeof(xl_heap_freeze_plan));
+			XLogRegisterBufData(0, (char *) offsets,
+								presult->nfrozen * sizeof(OffsetNumber));
+
+			recptr = XLogInsert(RM_HEAP2_ID, XLOG_HEAP2_FREEZE_PAGE);
+
+			PageSetLSN(page, recptr);
+		}
+
+		END_CRIT_SECTION();
 	}
 	else if (!pagefrz || !presult->all_frozen || presult->nfrozen > 0)
 	{
diff --git a/src/include/access/heapam.h b/src/include/access/heapam.h
index 02e33f213e1..321a46185e1 100644
--- a/src/include/access/heapam.h
+++ b/src/include/access/heapam.h
@@ -14,6 +14,7 @@
 #ifndef HEAPAM_H
 #define HEAPAM_H
 
+#include "access/heapam_xlog.h"
 #include "access/relation.h"	/* for backward compatibility */
 #include "access/relscan.h"
 #include "access/sdir.h"
@@ -101,8 +102,8 @@ typedef enum
 } HTSV_Result;
 
 /*
- * heap_prepare_freeze_tuple may request that heap_freeze_execute_prepared
- * check any tuple's to-be-frozen xmin and/or xmax status using pg_xact
+ * heap_prepare_freeze_tuple may request that any tuple's to-be-frozen xmin
+ * and/or xmax status is checked using pg_xact during freezing execution.
  */
 #define		HEAP_FREEZE_CHECK_XMIN_COMMITTED	0x01
 #define		HEAP_FREEZE_CHECK_XMAX_ABORTED		0x02
@@ -154,14 +155,14 @@ typedef struct HeapPageFreeze
 	/*
 	 * "Freeze" NewRelfrozenXid/NewRelminMxid trackers.
 	 *
-	 * Trackers used when heap_freeze_execute_prepared freezes, or when there
-	 * are zero freeze plans for a page.  It is always valid for vacuumlazy.c
-	 * to freeze any page, by definition.  This even includes pages that have
-	 * no tuples with storage to consider in the first place.  That way the
-	 * 'totally_frozen' results from heap_prepare_freeze_tuple can always be
-	 * used in the same way, even when no freeze plans need to be executed to
-	 * "freeze the page".  Only the "freeze" path needs to consider the need
-	 * to set pages all-frozen in the visibility map under this scheme.
+	 * Trackers used when tuples will be frozen, or when there are zero freeze
+	 * plans for a page.  It is always valid for vacuumlazy.c to freeze any
+	 * page, by definition.  This even includes pages that have no tuples with
+	 * storage to consider in the first place.  That way the 'totally_frozen'
+	 * results from heap_prepare_freeze_tuple can always be used in the same
+	 * way, even when no freeze plans need to be executed to "freeze the
+	 * page". Only the "freeze" path needs to consider the need to set pages
+	 * all-frozen in the visibility map under this scheme.
 	 *
 	 * When we freeze a page, we generally freeze all XIDs < OldestXmin, only
 	 * leaving behind XIDs that are ineligible for freezing, if any.  And so
@@ -315,12 +316,16 @@ extern bool heap_prepare_freeze_tuple(HeapTupleHeader tuple,
 
 extern void heap_pre_freeze_checks(Buffer buffer,
 								   HeapTupleFreeze *tuples, int ntuples);
-extern void heap_freeze_execute_prepared(Relation rel, Buffer buffer,
-										 TransactionId snapshotConflictHorizon,
-										 HeapTupleFreeze *tuples, int ntuples);
+
+extern void heap_freeze_prepared_tuples(Buffer buffer,
+										HeapTupleFreeze *tuples, int ntuples);
 extern bool heap_freeze_tuple(HeapTupleHeader tuple,
 							  TransactionId relfrozenxid, TransactionId relminmxid,
 							  TransactionId FreezeLimit, TransactionId MultiXactCutoff);
+
+extern int	heap_log_freeze_plan(HeapTupleFreeze *tuples, int ntuples,
+								 xl_heap_freeze_plan *plans_out,
+								 OffsetNumber *offsets_out);
 extern bool heap_tuple_should_freeze(HeapTupleHeader tuple,
 									 const struct VacuumCutoffs *cutoffs,
 									 TransactionId *NoFreezePageRelfrozenXid,
-- 
2.40.1


--tez7m2a73jtztiij
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
	filename="v4-0013-Merge-prune-and-freeze-records.patch"



^ permalink  raw  reply  [nested|flat] 11+ messages in thread

* [PATCH v4 12/19] Remove heap_freeze_execute_prepared()
@ 2024-01-07 22:03  Melanie Plageman <[email protected]>
  0 siblings, 0 replies; 11+ messages in thread

From: Melanie Plageman @ 2024-01-07 22:03 UTC (permalink / raw)

In order to merge freeze and prune records, the execution of tuple
freezing and the WAL logging of the changes to the page must be
separated so that the WAL logging can be combined with prune WAL
logging. This commit makes a helper for the tuple freezing and then
inlines the contents of heap_freeze_execute_prepared() where it is
called in heap_page_prune().
---
 src/backend/access/heap/heapam.c    | 79 +++++------------------------
 src/backend/access/heap/pruneheap.c | 51 +++++++++++++++++--
 src/include/access/heapam.h         | 31 ++++++-----
 3 files changed, 77 insertions(+), 84 deletions(-)

diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.c
index 16e3f2520a4..e47b56e7856 100644
--- a/src/backend/access/heap/heapam.c
+++ b/src/backend/access/heap/heapam.c
@@ -91,9 +91,6 @@ static void compute_new_xmax_infomask(TransactionId xmax, uint16 old_infomask,
 static TM_Result heap_lock_updated_tuple(Relation rel, HeapTuple tuple,
 										 ItemPointer ctid, TransactionId xid,
 										 LockTupleMode mode);
-static int	heap_log_freeze_plan(HeapTupleFreeze *tuples, int ntuples,
-								 xl_heap_freeze_plan *plans_out,
-								 OffsetNumber *offsets_out);
 static void GetMultiXactIdHintBits(MultiXactId multi, uint16 *new_infomask,
 								   uint16 *new_infomask2);
 static TransactionId MultiXactIdGetUpdateXid(TransactionId xmax,
@@ -6343,9 +6340,9 @@ FreezeMultiXactId(MultiXactId multi, uint16 t_infomask,
  * XIDs or MultiXactIds that will need to be processed by a future VACUUM.
  *
  * VACUUM caller must assemble HeapTupleFreeze freeze plan entries for every
- * tuple that we returned true for, and call heap_freeze_execute_prepared to
- * execute freezing.  Caller must initialize pagefrz fields for page as a
- * whole before first call here for each heap page.
+ * tuple that we returned true for, and then execute freezing.  Caller must
+ * initialize pagefrz fields for page as a whole before first call here for
+ * each heap page.
  *
  * VACUUM caller decides on whether or not to freeze the page as a whole.
  * We'll often prepare freeze plans for a page that caller just discards.
@@ -6659,8 +6656,8 @@ heap_execute_freeze_tuple(HeapTupleHeader tuple, HeapTupleFreeze *frz)
 }
 
 /*
-* Perform xmin/xmax XID status sanity checks before calling
-* heap_freeze_execute_prepared().
+* Perform xmin/xmax XID status sanity checks before actually executing freeze
+* plans.
 *
 * heap_prepare_freeze_tuple doesn't perform these checks directly because
 * pg_xact lookups are relatively expensive.  They shouldn't be repeated
@@ -6713,30 +6710,17 @@ heap_pre_freeze_checks(Buffer buffer,
 }
 
 /*
- * heap_freeze_execute_prepared
- *
- * Executes freezing of one or more heap tuples on a page on behalf of caller.
- * Caller passes an array of tuple plans from heap_prepare_freeze_tuple.
- * Caller must set 'offset' in each plan for us.  Note that we destructively
- * sort caller's tuples array in-place, so caller had better be done with it.
- *
- * WAL-logs the changes so that VACUUM can advance the rel's relfrozenxid
- * later on without any risk of unsafe pg_xact lookups, even following a hard
- * crash (or when querying from a standby).  We represent freezing by setting
- * infomask bits in tuple headers, but this shouldn't be thought of as a hint.
- * See section on buffer access rules in src/backend/storage/buffer/README.
+ * Helper which executes freezing of one or more heap tuples on a page on
+ * behalf of caller. Caller passes an array of tuple plans from
+ * heap_prepare_freeze_tuple. Caller must set 'offset' in each plan for us.
+ * Must be called in a critical section that also marks the buffer dirty and,
+ * if needed, emits WAL.
  */
 void
-heap_freeze_execute_prepared(Relation rel, Buffer buffer,
-							 TransactionId snapshotConflictHorizon,
-							 HeapTupleFreeze *tuples, int ntuples)
+heap_freeze_prepared_tuples(Buffer buffer, HeapTupleFreeze *tuples, int ntuples)
 {
 	Page		page = BufferGetPage(buffer);
 
-	Assert(ntuples > 0);
-
-	START_CRIT_SECTION();
-
 	for (int i = 0; i < ntuples; i++)
 	{
 		HeapTupleFreeze *frz = tuples + i;
@@ -6746,45 +6730,6 @@ heap_freeze_execute_prepared(Relation rel, Buffer buffer,
 		htup = (HeapTupleHeader) PageGetItem(page, itemid);
 		heap_execute_freeze_tuple(htup, frz);
 	}
-
-	MarkBufferDirty(buffer);
-
-	/* Now WAL-log freezing if necessary */
-	if (RelationNeedsWAL(rel))
-	{
-		xl_heap_freeze_plan plans[MaxHeapTuplesPerPage];
-		OffsetNumber offsets[MaxHeapTuplesPerPage];
-		int			nplans;
-		xl_heap_freeze_page xlrec;
-		XLogRecPtr	recptr;
-
-		/* Prepare deduplicated representation for use in WAL record */
-		nplans = heap_log_freeze_plan(tuples, ntuples, plans, offsets);
-
-		xlrec.snapshotConflictHorizon = snapshotConflictHorizon;
-		xlrec.isCatalogRel = RelationIsAccessibleInLogicalDecoding(rel);
-		xlrec.nplans = nplans;
-
-		XLogBeginInsert();
-		XLogRegisterData((char *) &xlrec, SizeOfHeapFreezePage);
-
-		/*
-		 * The freeze plan array and offset array are not actually in the
-		 * buffer, but pretend that they are.  When XLogInsert stores the
-		 * whole buffer, the arrays need not be stored too.
-		 */
-		XLogRegisterBuffer(0, buffer, REGBUF_STANDARD);
-		XLogRegisterBufData(0, (char *) plans,
-							nplans * sizeof(xl_heap_freeze_plan));
-		XLogRegisterBufData(0, (char *) offsets,
-							ntuples * sizeof(OffsetNumber));
-
-		recptr = XLogInsert(RM_HEAP2_ID, XLOG_HEAP2_FREEZE_PAGE);
-
-		PageSetLSN(page, recptr);
-	}
-
-	END_CRIT_SECTION();
 }
 
 /*
@@ -6874,7 +6819,7 @@ heap_log_freeze_new_plan(xl_heap_freeze_plan *plan, HeapTupleFreeze *frz)
  * (actually there is one array per freeze plan, but that's not of immediate
  * concern to our caller).
  */
-static int
+int
 heap_log_freeze_plan(HeapTupleFreeze *tuples, int ntuples,
 					 xl_heap_freeze_plan *plans_out,
 					 OffsetNumber *offsets_out)
diff --git a/src/backend/access/heap/pruneheap.c b/src/backend/access/heap/pruneheap.c
index 87f99497865..7bd479cfd4e 100644
--- a/src/backend/access/heap/pruneheap.c
+++ b/src/backend/access/heap/pruneheap.c
@@ -640,10 +640,53 @@ heap_page_prune_and_freeze(Relation relation, Buffer buffer,
 
 	if (do_freeze)
 	{
-		/* Execute all freeze plans for page as a single atomic action */
-		heap_freeze_execute_prepared(relation, buffer,
-									 presult->frz_conflict_horizon,
-									 prstate.frozen, presult->nfrozen);
+		START_CRIT_SECTION();
+
+		Assert(presult->nfrozen > 0);
+
+		heap_freeze_prepared_tuples(buffer, prstate.frozen, presult->nfrozen);
+
+		MarkBufferDirty(buffer);
+
+		/* Now WAL-log freezing if necessary */
+		if (RelationNeedsWAL(relation))
+		{
+			xl_heap_freeze_plan plans[MaxHeapTuplesPerPage];
+			OffsetNumber offsets[MaxHeapTuplesPerPage];
+			int			nplans;
+			xl_heap_freeze_page xlrec;
+			XLogRecPtr	recptr;
+
+			/*
+			 * Prepare deduplicated representation for use in WAL record
+			 * Destructively sorts tuples array in-place.
+			 */
+			nplans = heap_log_freeze_plan(prstate.frozen, presult->nfrozen, plans, offsets);
+
+			xlrec.snapshotConflictHorizon = presult->frz_conflict_horizon;
+			xlrec.isCatalogRel = RelationIsAccessibleInLogicalDecoding(relation);
+			xlrec.nplans = nplans;
+
+			XLogBeginInsert();
+			XLogRegisterData((char *) &xlrec, SizeOfHeapFreezePage);
+
+			/*
+			 * The freeze plan array and offset array are not actually in the
+			 * buffer, but pretend that they are.  When XLogInsert stores the
+			 * whole buffer, the arrays need not be stored too.
+			 */
+			XLogRegisterBuffer(0, buffer, REGBUF_STANDARD);
+			XLogRegisterBufData(0, (char *) plans,
+								nplans * sizeof(xl_heap_freeze_plan));
+			XLogRegisterBufData(0, (char *) offsets,
+								presult->nfrozen * sizeof(OffsetNumber));
+
+			recptr = XLogInsert(RM_HEAP2_ID, XLOG_HEAP2_FREEZE_PAGE);
+
+			PageSetLSN(page, recptr);
+		}
+
+		END_CRIT_SECTION();
 	}
 	else if (!pagefrz || !presult->all_frozen || presult->nfrozen > 0)
 	{
diff --git a/src/include/access/heapam.h b/src/include/access/heapam.h
index 02e33f213e1..321a46185e1 100644
--- a/src/include/access/heapam.h
+++ b/src/include/access/heapam.h
@@ -14,6 +14,7 @@
 #ifndef HEAPAM_H
 #define HEAPAM_H
 
+#include "access/heapam_xlog.h"
 #include "access/relation.h"	/* for backward compatibility */
 #include "access/relscan.h"
 #include "access/sdir.h"
@@ -101,8 +102,8 @@ typedef enum
 } HTSV_Result;
 
 /*
- * heap_prepare_freeze_tuple may request that heap_freeze_execute_prepared
- * check any tuple's to-be-frozen xmin and/or xmax status using pg_xact
+ * heap_prepare_freeze_tuple may request that any tuple's to-be-frozen xmin
+ * and/or xmax status is checked using pg_xact during freezing execution.
  */
 #define		HEAP_FREEZE_CHECK_XMIN_COMMITTED	0x01
 #define		HEAP_FREEZE_CHECK_XMAX_ABORTED		0x02
@@ -154,14 +155,14 @@ typedef struct HeapPageFreeze
 	/*
 	 * "Freeze" NewRelfrozenXid/NewRelminMxid trackers.
 	 *
-	 * Trackers used when heap_freeze_execute_prepared freezes, or when there
-	 * are zero freeze plans for a page.  It is always valid for vacuumlazy.c
-	 * to freeze any page, by definition.  This even includes pages that have
-	 * no tuples with storage to consider in the first place.  That way the
-	 * 'totally_frozen' results from heap_prepare_freeze_tuple can always be
-	 * used in the same way, even when no freeze plans need to be executed to
-	 * "freeze the page".  Only the "freeze" path needs to consider the need
-	 * to set pages all-frozen in the visibility map under this scheme.
+	 * Trackers used when tuples will be frozen, or when there are zero freeze
+	 * plans for a page.  It is always valid for vacuumlazy.c to freeze any
+	 * page, by definition.  This even includes pages that have no tuples with
+	 * storage to consider in the first place.  That way the 'totally_frozen'
+	 * results from heap_prepare_freeze_tuple can always be used in the same
+	 * way, even when no freeze plans need to be executed to "freeze the
+	 * page". Only the "freeze" path needs to consider the need to set pages
+	 * all-frozen in the visibility map under this scheme.
 	 *
 	 * When we freeze a page, we generally freeze all XIDs < OldestXmin, only
 	 * leaving behind XIDs that are ineligible for freezing, if any.  And so
@@ -315,12 +316,16 @@ extern bool heap_prepare_freeze_tuple(HeapTupleHeader tuple,
 
 extern void heap_pre_freeze_checks(Buffer buffer,
 								   HeapTupleFreeze *tuples, int ntuples);
-extern void heap_freeze_execute_prepared(Relation rel, Buffer buffer,
-										 TransactionId snapshotConflictHorizon,
-										 HeapTupleFreeze *tuples, int ntuples);
+
+extern void heap_freeze_prepared_tuples(Buffer buffer,
+										HeapTupleFreeze *tuples, int ntuples);
 extern bool heap_freeze_tuple(HeapTupleHeader tuple,
 							  TransactionId relfrozenxid, TransactionId relminmxid,
 							  TransactionId FreezeLimit, TransactionId MultiXactCutoff);
+
+extern int	heap_log_freeze_plan(HeapTupleFreeze *tuples, int ntuples,
+								 xl_heap_freeze_plan *plans_out,
+								 OffsetNumber *offsets_out);
 extern bool heap_tuple_should_freeze(HeapTupleHeader tuple,
 									 const struct VacuumCutoffs *cutoffs,
 									 TransactionId *NoFreezePageRelfrozenXid,
-- 
2.40.1


--tez7m2a73jtztiij
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
	filename="v4-0013-Merge-prune-and-freeze-records.patch"



^ permalink  raw  reply  [nested|flat] 11+ messages in thread

* [PATCH v9 11/21] Remove heap_freeze_execute_prepared()
@ 2024-03-26 13:10  Melanie Plageman <[email protected]>
  0 siblings, 0 replies; 11+ messages in thread

From: Melanie Plageman @ 2024-03-26 13:10 UTC (permalink / raw)

In order to merge freeze and prune records, the execution of tuple
freezing and the WAL logging of the changes to the page must be
separated so that the WAL logging can be combined with prune WAL
logging. This commit makes a helper for the tuple freezing and then
inlines the contents of heap_freeze_execute_prepared() where it is
called in heap_page_prune().
---
 src/backend/access/heap/heapam.c    | 49 +++++++----------------------
 src/backend/access/heap/pruneheap.c | 22 ++++++++++---
 src/include/access/heapam.h         | 28 +++++++++--------
 3 files changed, 44 insertions(+), 55 deletions(-)

diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.c
index b3119de2aa6..41c1c7d286f 100644
--- a/src/backend/access/heap/heapam.c
+++ b/src/backend/access/heap/heapam.c
@@ -6445,9 +6445,9 @@ FreezeMultiXactId(MultiXactId multi, uint16 t_infomask,
  * XIDs or MultiXactIds that will need to be processed by a future VACUUM.
  *
  * VACUUM caller must assemble HeapTupleFreeze freeze plan entries for every
- * tuple that we returned true for, and call heap_freeze_execute_prepared to
- * execute freezing.  Caller must initialize pagefrz fields for page as a
- * whole before first call here for each heap page.
+ * tuple that we returned true for, and then execute freezing.  Caller must
+ * initialize pagefrz fields for page as a whole before first call here for
+ * each heap page.
  *
  * VACUUM caller decides on whether or not to freeze the page as a whole.
  * We'll often prepare freeze plans for a page that caller just discards.
@@ -6762,8 +6762,8 @@ heap_execute_freeze_tuple(HeapTupleHeader tuple, HeapTupleFreeze *frz)
 }
 
 /*
-* Perform xmin/xmax XID status sanity checks before calling
-* heap_freeze_execute_prepared().
+* Perform xmin/xmax XID status sanity checks before actually executing freeze
+* plans.
 *
 * heap_prepare_freeze_tuple doesn't perform these checks directly because
 * pg_xact lookups are relatively expensive.  They shouldn't be repeated
@@ -6816,30 +6816,17 @@ heap_pre_freeze_checks(Buffer buffer,
 }
 
 /*
- * heap_freeze_execute_prepared
- *
- * Executes freezing of one or more heap tuples on a page on behalf of caller.
- * Caller passes an array of tuple plans from heap_prepare_freeze_tuple.
- * Caller must set 'offset' in each plan for us.  Note that we destructively
- * sort caller's tuples array in-place, so caller had better be done with it.
- *
- * WAL-logs the changes so that VACUUM can advance the rel's relfrozenxid
- * later on without any risk of unsafe pg_xact lookups, even following a hard
- * crash (or when querying from a standby).  We represent freezing by setting
- * infomask bits in tuple headers, but this shouldn't be thought of as a hint.
- * See section on buffer access rules in src/backend/storage/buffer/README.
+ * Helper which executes freezing of one or more heap tuples on a page on
+ * behalf of caller. Caller passes an array of tuple plans from
+ * heap_prepare_freeze_tuple. Caller must set 'offset' in each plan for us.
+ * Must be called in a critical section that also marks the buffer dirty and,
+ * if needed, emits WAL.
  */
 void
-heap_freeze_execute_prepared(Relation rel, Buffer buffer,
-							 TransactionId snapshotConflictHorizon,
-							 HeapTupleFreeze *tuples, int ntuples)
+heap_freeze_prepared_tuples(Buffer buffer, HeapTupleFreeze *tuples, int ntuples)
 {
 	Page		page = BufferGetPage(buffer);
 
-	Assert(ntuples > 0);
-
-	START_CRIT_SECTION();
-
 	for (int i = 0; i < ntuples; i++)
 	{
 		HeapTupleFreeze *frz = tuples + i;
@@ -6851,20 +6838,6 @@ heap_freeze_execute_prepared(Relation rel, Buffer buffer,
 	}
 
 	MarkBufferDirty(buffer);
-
-	/* Now WAL-log freezing if necessary */
-	if (RelationNeedsWAL(rel))
-	{
-		log_heap_prune_and_freeze(rel, buffer, snapshotConflictHorizon,
-								  false,	/* no cleanup lock required */
-								  PRUNE_VACUUM_SCAN,
-								  tuples, ntuples,
-								  NULL, 0,	/* redirected */
-								  NULL, 0,	/* dead */
-								  NULL, 0); /* unused */
-	}
-
-	END_CRIT_SECTION();
 }
 
 /*
diff --git a/src/backend/access/heap/pruneheap.c b/src/backend/access/heap/pruneheap.c
index 13db348b2c1..5d8c881c2fc 100644
--- a/src/backend/access/heap/pruneheap.c
+++ b/src/backend/access/heap/pruneheap.c
@@ -635,10 +635,24 @@ heap_page_prune_and_freeze(Relation relation, Buffer buffer,
 
 	if (do_freeze)
 	{
-		/* Execute all freeze plans for page as a single atomic action */
-		heap_freeze_execute_prepared(relation, buffer,
-									 frz_conflict_horizon,
-									 prstate.frozen, presult->nfrozen);
+		START_CRIT_SECTION();
+
+		Assert(presult->nfrozen > 0);
+
+		heap_freeze_prepared_tuples(buffer, prstate.frozen, presult->nfrozen);
+
+		MarkBufferDirty(buffer);
+
+		/* Now WAL-log freezing if necessary */
+		if (RelationNeedsWAL(relation))
+			log_heap_prune_and_freeze(relation, buffer,
+									  frz_conflict_horizon, false, reason,
+									  prstate.frozen, presult->nfrozen,
+									  NULL, 0,	/* redirected */
+									  NULL, 0,	/* dead */
+									  NULL, 0); /* unused */
+
+		END_CRIT_SECTION();
 	}
 	else if (!pagefrz || !presult->all_frozen || presult->nfrozen > 0)
 	{
diff --git a/src/include/access/heapam.h b/src/include/access/heapam.h
index cc3b3346bc4..897f3bc50c9 100644
--- a/src/include/access/heapam.h
+++ b/src/include/access/heapam.h
@@ -14,6 +14,7 @@
 #ifndef HEAPAM_H
 #define HEAPAM_H
 
+#include "access/heapam_xlog.h"
 #include "access/relation.h"	/* for backward compatibility */
 #include "access/relscan.h"
 #include "access/sdir.h"
@@ -101,8 +102,8 @@ typedef enum
 } HTSV_Result;
 
 /*
- * heap_prepare_freeze_tuple may request that heap_freeze_execute_prepared
- * check any tuple's to-be-frozen xmin and/or xmax status using pg_xact
+ * heap_prepare_freeze_tuple may request that any tuple's to-be-frozen xmin
+ * and/or xmax status is checked using pg_xact during freezing execution.
  */
 #define		HEAP_FREEZE_CHECK_XMIN_COMMITTED	0x01
 #define		HEAP_FREEZE_CHECK_XMAX_ABORTED		0x02
@@ -154,14 +155,14 @@ typedef struct HeapPageFreeze
 	/*
 	 * "Freeze" NewRelfrozenXid/NewRelminMxid trackers.
 	 *
-	 * Trackers used when heap_freeze_execute_prepared freezes, or when there
-	 * are zero freeze plans for a page.  It is always valid for vacuumlazy.c
-	 * to freeze any page, by definition.  This even includes pages that have
-	 * no tuples with storage to consider in the first place.  That way the
-	 * 'totally_frozen' results from heap_prepare_freeze_tuple can always be
-	 * used in the same way, even when no freeze plans need to be executed to
-	 * "freeze the page".  Only the "freeze" path needs to consider the need
-	 * to set pages all-frozen in the visibility map under this scheme.
+	 * Trackers used when tuples will be frozen, or when there are zero freeze
+	 * plans for a page.  It is always valid for vacuumlazy.c to freeze any
+	 * page, by definition.  This even includes pages that have no tuples with
+	 * storage to consider in the first place.  That way the 'totally_frozen'
+	 * results from heap_prepare_freeze_tuple can always be used in the same
+	 * way, even when no freeze plans need to be executed to "freeze the
+	 * page". Only the "freeze" path needs to consider the need to set pages
+	 * all-frozen in the visibility map under this scheme.
 	 *
 	 * When we freeze a page, we generally freeze all XIDs < OldestXmin, only
 	 * leaving behind XIDs that are ineligible for freezing, if any.  And so
@@ -345,12 +346,13 @@ extern bool heap_prepare_freeze_tuple(HeapTupleHeader tuple,
 
 extern void heap_pre_freeze_checks(Buffer buffer,
 								   HeapTupleFreeze *tuples, int ntuples);
-extern void heap_freeze_execute_prepared(Relation rel, Buffer buffer,
-										 TransactionId snapshotConflictHorizon,
-										 HeapTupleFreeze *tuples, int ntuples);
+
+extern void heap_freeze_prepared_tuples(Buffer buffer,
+										HeapTupleFreeze *tuples, int ntuples);
 extern bool heap_freeze_tuple(HeapTupleHeader tuple,
 							  TransactionId relfrozenxid, TransactionId relminmxid,
 							  TransactionId FreezeLimit, TransactionId MultiXactCutoff);
+
 extern bool heap_tuple_should_freeze(HeapTupleHeader tuple,
 									 const struct VacuumCutoffs *cutoffs,
 									 TransactionId *NoFreezePageRelfrozenXid,
-- 
2.40.1


--caj67xgx3lukmr5f
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
	filename="v9-0012-Merge-prune-and-freeze-records.patch"



^ permalink  raw  reply  [nested|flat] 11+ messages in thread

* [PATCH v7 11/16] Remove heap_freeze_execute_prepared()
@ 2024-03-26 13:10  Melanie Plageman <[email protected]>
  0 siblings, 0 replies; 11+ messages in thread

From: Melanie Plageman @ 2024-03-26 13:10 UTC (permalink / raw)

In order to merge freeze and prune records, the execution of tuple
freezing and the WAL logging of the changes to the page must be
separated so that the WAL logging can be combined with prune WAL
logging. This commit makes a helper for the tuple freezing and then
inlines the contents of heap_freeze_execute_prepared() where it is
called in heap_page_prune().
---
 src/backend/access/heap/heapam.c    | 49 +++++++----------------------
 src/backend/access/heap/pruneheap.c | 22 ++++++++++---
 src/include/access/heapam.h         | 28 +++++++++--------
 3 files changed, 44 insertions(+), 55 deletions(-)

diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.c
index be48098f7f3..1c1785994b1 100644
--- a/src/backend/access/heap/heapam.c
+++ b/src/backend/access/heap/heapam.c
@@ -6340,9 +6340,9 @@ FreezeMultiXactId(MultiXactId multi, uint16 t_infomask,
  * XIDs or MultiXactIds that will need to be processed by a future VACUUM.
  *
  * VACUUM caller must assemble HeapTupleFreeze freeze plan entries for every
- * tuple that we returned true for, and call heap_freeze_execute_prepared to
- * execute freezing.  Caller must initialize pagefrz fields for page as a
- * whole before first call here for each heap page.
+ * tuple that we returned true for, and then execute freezing.  Caller must
+ * initialize pagefrz fields for page as a whole before first call here for
+ * each heap page.
  *
  * VACUUM caller decides on whether or not to freeze the page as a whole.
  * We'll often prepare freeze plans for a page that caller just discards.
@@ -6657,8 +6657,8 @@ heap_execute_freeze_tuple(HeapTupleHeader tuple, HeapTupleFreeze *frz)
 }
 
 /*
-* Perform xmin/xmax XID status sanity checks before calling
-* heap_freeze_execute_prepared().
+* Perform xmin/xmax XID status sanity checks before actually executing freeze
+* plans.
 *
 * heap_prepare_freeze_tuple doesn't perform these checks directly because
 * pg_xact lookups are relatively expensive.  They shouldn't be repeated
@@ -6711,30 +6711,17 @@ heap_pre_freeze_checks(Buffer buffer,
 }
 
 /*
- * heap_freeze_execute_prepared
- *
- * Executes freezing of one or more heap tuples on a page on behalf of caller.
- * Caller passes an array of tuple plans from heap_prepare_freeze_tuple.
- * Caller must set 'offset' in each plan for us.  Note that we destructively
- * sort caller's tuples array in-place, so caller had better be done with it.
- *
- * WAL-logs the changes so that VACUUM can advance the rel's relfrozenxid
- * later on without any risk of unsafe pg_xact lookups, even following a hard
- * crash (or when querying from a standby).  We represent freezing by setting
- * infomask bits in tuple headers, but this shouldn't be thought of as a hint.
- * See section on buffer access rules in src/backend/storage/buffer/README.
+ * Helper which executes freezing of one or more heap tuples on a page on
+ * behalf of caller. Caller passes an array of tuple plans from
+ * heap_prepare_freeze_tuple. Caller must set 'offset' in each plan for us.
+ * Must be called in a critical section that also marks the buffer dirty and,
+ * if needed, emits WAL.
  */
 void
-heap_freeze_execute_prepared(Relation rel, Buffer buffer,
-							 TransactionId snapshotConflictHorizon,
-							 HeapTupleFreeze *tuples, int ntuples)
+heap_freeze_prepared_tuples(Buffer buffer, HeapTupleFreeze *tuples, int ntuples)
 {
 	Page		page = BufferGetPage(buffer);
 
-	Assert(ntuples > 0);
-
-	START_CRIT_SECTION();
-
 	for (int i = 0; i < ntuples; i++)
 	{
 		HeapTupleFreeze *frz = tuples + i;
@@ -6746,20 +6733,6 @@ heap_freeze_execute_prepared(Relation rel, Buffer buffer,
 	}
 
 	MarkBufferDirty(buffer);
-
-	/* Now WAL-log freezing if necessary */
-	if (RelationNeedsWAL(rel))
-	{
-		log_heap_prune_and_freeze(rel, buffer, snapshotConflictHorizon,
-								  false,	/* no cleanup lock required */
-								  PRUNE_VACUUM_SCAN,
-								  tuples, ntuples,
-								  NULL, 0,	/* redirected */
-								  NULL, 0,	/* dead */
-								  NULL, 0); /* unused */
-	}
-
-	END_CRIT_SECTION();
 }
 
 /*
diff --git a/src/backend/access/heap/pruneheap.c b/src/backend/access/heap/pruneheap.c
index fe463ad7146..8914d4bf5c8 100644
--- a/src/backend/access/heap/pruneheap.c
+++ b/src/backend/access/heap/pruneheap.c
@@ -635,10 +635,24 @@ heap_page_prune_and_freeze(Relation relation, Buffer buffer,
 
 	if (do_freeze)
 	{
-		/* Execute all freeze plans for page as a single atomic action */
-		heap_freeze_execute_prepared(relation, buffer,
-									 frz_conflict_horizon,
-									 prstate.frozen, presult->nfrozen);
+		START_CRIT_SECTION();
+
+		Assert(presult->nfrozen > 0);
+
+		heap_freeze_prepared_tuples(buffer, prstate.frozen, presult->nfrozen);
+
+		MarkBufferDirty(buffer);
+
+		/* Now WAL-log freezing if necessary */
+		if (RelationNeedsWAL(relation))
+			log_heap_prune_and_freeze(relation, buffer,
+									  frz_conflict_horizon, false, reason,
+									  prstate.frozen, presult->nfrozen,
+									  NULL, 0,	/* redirected */
+									  NULL, 0,	/* dead */
+									  NULL, 0); /* unused */
+
+		END_CRIT_SECTION();
 	}
 	else if (!pagefrz || !presult->all_frozen || presult->nfrozen > 0)
 	{
diff --git a/src/include/access/heapam.h b/src/include/access/heapam.h
index dbf6323b5ff..ac0ef6e4281 100644
--- a/src/include/access/heapam.h
+++ b/src/include/access/heapam.h
@@ -14,6 +14,7 @@
 #ifndef HEAPAM_H
 #define HEAPAM_H
 
+#include "access/heapam_xlog.h"
 #include "access/relation.h"	/* for backward compatibility */
 #include "access/relscan.h"
 #include "access/sdir.h"
@@ -101,8 +102,8 @@ typedef enum
 } HTSV_Result;
 
 /*
- * heap_prepare_freeze_tuple may request that heap_freeze_execute_prepared
- * check any tuple's to-be-frozen xmin and/or xmax status using pg_xact
+ * heap_prepare_freeze_tuple may request that any tuple's to-be-frozen xmin
+ * and/or xmax status is checked using pg_xact during freezing execution.
  */
 #define		HEAP_FREEZE_CHECK_XMIN_COMMITTED	0x01
 #define		HEAP_FREEZE_CHECK_XMAX_ABORTED		0x02
@@ -154,14 +155,14 @@ typedef struct HeapPageFreeze
 	/*
 	 * "Freeze" NewRelfrozenXid/NewRelminMxid trackers.
 	 *
-	 * Trackers used when heap_freeze_execute_prepared freezes, or when there
-	 * are zero freeze plans for a page.  It is always valid for vacuumlazy.c
-	 * to freeze any page, by definition.  This even includes pages that have
-	 * no tuples with storage to consider in the first place.  That way the
-	 * 'totally_frozen' results from heap_prepare_freeze_tuple can always be
-	 * used in the same way, even when no freeze plans need to be executed to
-	 * "freeze the page".  Only the "freeze" path needs to consider the need
-	 * to set pages all-frozen in the visibility map under this scheme.
+	 * Trackers used when tuples will be frozen, or when there are zero freeze
+	 * plans for a page.  It is always valid for vacuumlazy.c to freeze any
+	 * page, by definition.  This even includes pages that have no tuples with
+	 * storage to consider in the first place.  That way the 'totally_frozen'
+	 * results from heap_prepare_freeze_tuple can always be used in the same
+	 * way, even when no freeze plans need to be executed to "freeze the
+	 * page". Only the "freeze" path needs to consider the need to set pages
+	 * all-frozen in the visibility map under this scheme.
 	 *
 	 * When we freeze a page, we generally freeze all XIDs < OldestXmin, only
 	 * leaving behind XIDs that are ineligible for freezing, if any.  And so
@@ -343,12 +344,13 @@ extern bool heap_prepare_freeze_tuple(HeapTupleHeader tuple,
 
 extern void heap_pre_freeze_checks(Buffer buffer,
 								   HeapTupleFreeze *tuples, int ntuples);
-extern void heap_freeze_execute_prepared(Relation rel, Buffer buffer,
-										 TransactionId snapshotConflictHorizon,
-										 HeapTupleFreeze *tuples, int ntuples);
+
+extern void heap_freeze_prepared_tuples(Buffer buffer,
+										HeapTupleFreeze *tuples, int ntuples);
 extern bool heap_freeze_tuple(HeapTupleHeader tuple,
 							  TransactionId relfrozenxid, TransactionId relminmxid,
 							  TransactionId FreezeLimit, TransactionId MultiXactCutoff);
+
 extern bool heap_tuple_should_freeze(HeapTupleHeader tuple,
 									 const struct VacuumCutoffs *cutoffs,
 									 TransactionId *NoFreezePageRelfrozenXid,
-- 
2.40.1


--ck6erxojvlx23byk
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
	filename="v7-0012-Merge-prune-and-freeze-records.patch"



^ permalink  raw  reply  [nested|flat] 11+ messages in thread

* [PATCH v7 11/16] Remove heap_freeze_execute_prepared()
@ 2024-03-26 13:10  Melanie Plageman <[email protected]>
  0 siblings, 0 replies; 11+ messages in thread

From: Melanie Plageman @ 2024-03-26 13:10 UTC (permalink / raw)

In order to merge freeze and prune records, the execution of tuple
freezing and the WAL logging of the changes to the page must be
separated so that the WAL logging can be combined with prune WAL
logging. This commit makes a helper for the tuple freezing and then
inlines the contents of heap_freeze_execute_prepared() where it is
called in heap_page_prune().
---
 src/backend/access/heap/heapam.c    | 49 +++++++----------------------
 src/backend/access/heap/pruneheap.c | 22 ++++++++++---
 src/include/access/heapam.h         | 28 +++++++++--------
 3 files changed, 44 insertions(+), 55 deletions(-)

diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.c
index be48098f7f3..1c1785994b1 100644
--- a/src/backend/access/heap/heapam.c
+++ b/src/backend/access/heap/heapam.c
@@ -6340,9 +6340,9 @@ FreezeMultiXactId(MultiXactId multi, uint16 t_infomask,
  * XIDs or MultiXactIds that will need to be processed by a future VACUUM.
  *
  * VACUUM caller must assemble HeapTupleFreeze freeze plan entries for every
- * tuple that we returned true for, and call heap_freeze_execute_prepared to
- * execute freezing.  Caller must initialize pagefrz fields for page as a
- * whole before first call here for each heap page.
+ * tuple that we returned true for, and then execute freezing.  Caller must
+ * initialize pagefrz fields for page as a whole before first call here for
+ * each heap page.
  *
  * VACUUM caller decides on whether or not to freeze the page as a whole.
  * We'll often prepare freeze plans for a page that caller just discards.
@@ -6657,8 +6657,8 @@ heap_execute_freeze_tuple(HeapTupleHeader tuple, HeapTupleFreeze *frz)
 }
 
 /*
-* Perform xmin/xmax XID status sanity checks before calling
-* heap_freeze_execute_prepared().
+* Perform xmin/xmax XID status sanity checks before actually executing freeze
+* plans.
 *
 * heap_prepare_freeze_tuple doesn't perform these checks directly because
 * pg_xact lookups are relatively expensive.  They shouldn't be repeated
@@ -6711,30 +6711,17 @@ heap_pre_freeze_checks(Buffer buffer,
 }
 
 /*
- * heap_freeze_execute_prepared
- *
- * Executes freezing of one or more heap tuples on a page on behalf of caller.
- * Caller passes an array of tuple plans from heap_prepare_freeze_tuple.
- * Caller must set 'offset' in each plan for us.  Note that we destructively
- * sort caller's tuples array in-place, so caller had better be done with it.
- *
- * WAL-logs the changes so that VACUUM can advance the rel's relfrozenxid
- * later on without any risk of unsafe pg_xact lookups, even following a hard
- * crash (or when querying from a standby).  We represent freezing by setting
- * infomask bits in tuple headers, but this shouldn't be thought of as a hint.
- * See section on buffer access rules in src/backend/storage/buffer/README.
+ * Helper which executes freezing of one or more heap tuples on a page on
+ * behalf of caller. Caller passes an array of tuple plans from
+ * heap_prepare_freeze_tuple. Caller must set 'offset' in each plan for us.
+ * Must be called in a critical section that also marks the buffer dirty and,
+ * if needed, emits WAL.
  */
 void
-heap_freeze_execute_prepared(Relation rel, Buffer buffer,
-							 TransactionId snapshotConflictHorizon,
-							 HeapTupleFreeze *tuples, int ntuples)
+heap_freeze_prepared_tuples(Buffer buffer, HeapTupleFreeze *tuples, int ntuples)
 {
 	Page		page = BufferGetPage(buffer);
 
-	Assert(ntuples > 0);
-
-	START_CRIT_SECTION();
-
 	for (int i = 0; i < ntuples; i++)
 	{
 		HeapTupleFreeze *frz = tuples + i;
@@ -6746,20 +6733,6 @@ heap_freeze_execute_prepared(Relation rel, Buffer buffer,
 	}
 
 	MarkBufferDirty(buffer);
-
-	/* Now WAL-log freezing if necessary */
-	if (RelationNeedsWAL(rel))
-	{
-		log_heap_prune_and_freeze(rel, buffer, snapshotConflictHorizon,
-								  false,	/* no cleanup lock required */
-								  PRUNE_VACUUM_SCAN,
-								  tuples, ntuples,
-								  NULL, 0,	/* redirected */
-								  NULL, 0,	/* dead */
-								  NULL, 0); /* unused */
-	}
-
-	END_CRIT_SECTION();
 }
 
 /*
diff --git a/src/backend/access/heap/pruneheap.c b/src/backend/access/heap/pruneheap.c
index fe463ad7146..8914d4bf5c8 100644
--- a/src/backend/access/heap/pruneheap.c
+++ b/src/backend/access/heap/pruneheap.c
@@ -635,10 +635,24 @@ heap_page_prune_and_freeze(Relation relation, Buffer buffer,
 
 	if (do_freeze)
 	{
-		/* Execute all freeze plans for page as a single atomic action */
-		heap_freeze_execute_prepared(relation, buffer,
-									 frz_conflict_horizon,
-									 prstate.frozen, presult->nfrozen);
+		START_CRIT_SECTION();
+
+		Assert(presult->nfrozen > 0);
+
+		heap_freeze_prepared_tuples(buffer, prstate.frozen, presult->nfrozen);
+
+		MarkBufferDirty(buffer);
+
+		/* Now WAL-log freezing if necessary */
+		if (RelationNeedsWAL(relation))
+			log_heap_prune_and_freeze(relation, buffer,
+									  frz_conflict_horizon, false, reason,
+									  prstate.frozen, presult->nfrozen,
+									  NULL, 0,	/* redirected */
+									  NULL, 0,	/* dead */
+									  NULL, 0); /* unused */
+
+		END_CRIT_SECTION();
 	}
 	else if (!pagefrz || !presult->all_frozen || presult->nfrozen > 0)
 	{
diff --git a/src/include/access/heapam.h b/src/include/access/heapam.h
index dbf6323b5ff..ac0ef6e4281 100644
--- a/src/include/access/heapam.h
+++ b/src/include/access/heapam.h
@@ -14,6 +14,7 @@
 #ifndef HEAPAM_H
 #define HEAPAM_H
 
+#include "access/heapam_xlog.h"
 #include "access/relation.h"	/* for backward compatibility */
 #include "access/relscan.h"
 #include "access/sdir.h"
@@ -101,8 +102,8 @@ typedef enum
 } HTSV_Result;
 
 /*
- * heap_prepare_freeze_tuple may request that heap_freeze_execute_prepared
- * check any tuple's to-be-frozen xmin and/or xmax status using pg_xact
+ * heap_prepare_freeze_tuple may request that any tuple's to-be-frozen xmin
+ * and/or xmax status is checked using pg_xact during freezing execution.
  */
 #define		HEAP_FREEZE_CHECK_XMIN_COMMITTED	0x01
 #define		HEAP_FREEZE_CHECK_XMAX_ABORTED		0x02
@@ -154,14 +155,14 @@ typedef struct HeapPageFreeze
 	/*
 	 * "Freeze" NewRelfrozenXid/NewRelminMxid trackers.
 	 *
-	 * Trackers used when heap_freeze_execute_prepared freezes, or when there
-	 * are zero freeze plans for a page.  It is always valid for vacuumlazy.c
-	 * to freeze any page, by definition.  This even includes pages that have
-	 * no tuples with storage to consider in the first place.  That way the
-	 * 'totally_frozen' results from heap_prepare_freeze_tuple can always be
-	 * used in the same way, even when no freeze plans need to be executed to
-	 * "freeze the page".  Only the "freeze" path needs to consider the need
-	 * to set pages all-frozen in the visibility map under this scheme.
+	 * Trackers used when tuples will be frozen, or when there are zero freeze
+	 * plans for a page.  It is always valid for vacuumlazy.c to freeze any
+	 * page, by definition.  This even includes pages that have no tuples with
+	 * storage to consider in the first place.  That way the 'totally_frozen'
+	 * results from heap_prepare_freeze_tuple can always be used in the same
+	 * way, even when no freeze plans need to be executed to "freeze the
+	 * page". Only the "freeze" path needs to consider the need to set pages
+	 * all-frozen in the visibility map under this scheme.
 	 *
 	 * When we freeze a page, we generally freeze all XIDs < OldestXmin, only
 	 * leaving behind XIDs that are ineligible for freezing, if any.  And so
@@ -343,12 +344,13 @@ extern bool heap_prepare_freeze_tuple(HeapTupleHeader tuple,
 
 extern void heap_pre_freeze_checks(Buffer buffer,
 								   HeapTupleFreeze *tuples, int ntuples);
-extern void heap_freeze_execute_prepared(Relation rel, Buffer buffer,
-										 TransactionId snapshotConflictHorizon,
-										 HeapTupleFreeze *tuples, int ntuples);
+
+extern void heap_freeze_prepared_tuples(Buffer buffer,
+										HeapTupleFreeze *tuples, int ntuples);
 extern bool heap_freeze_tuple(HeapTupleHeader tuple,
 							  TransactionId relfrozenxid, TransactionId relminmxid,
 							  TransactionId FreezeLimit, TransactionId MultiXactCutoff);
+
 extern bool heap_tuple_should_freeze(HeapTupleHeader tuple,
 									 const struct VacuumCutoffs *cutoffs,
 									 TransactionId *NoFreezePageRelfrozenXid,
-- 
2.40.1


--ck6erxojvlx23byk
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
	filename="v7-0012-Merge-prune-and-freeze-records.patch"



^ permalink  raw  reply  [nested|flat] 11+ messages in thread


end of thread, other threads:[~2024-03-26 13:10 UTC | newest]

Thread overview: 11+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2022-02-25 09:34 Re: Minimal logical decoding on standbys Drouvot, Bertrand <[email protected]>
2022-06-30 08:49 ` Drouvot, Bertrand <[email protected]>
2022-07-01 20:03   ` Ibrar Ahmed <[email protected]>
2022-07-04 13:12     ` Drouvot, Bertrand <[email protected]>
2022-07-04 13:17       ` Ibrar Ahmed <[email protected]>
2024-01-07 22:03 [PATCH v4 12/19] Remove heap_freeze_execute_prepared() Melanie Plageman <[email protected]>
2024-01-07 22:03 [PATCH v4 12/19] Remove heap_freeze_execute_prepared() Melanie Plageman <[email protected]>
2024-01-07 22:03 [PATCH v4 12/19] Remove heap_freeze_execute_prepared() Melanie Plageman <[email protected]>
2024-03-26 13:10 [PATCH v7 11/16] Remove heap_freeze_execute_prepared() Melanie Plageman <[email protected]>
2024-03-26 13:10 [PATCH v9 11/21] Remove heap_freeze_execute_prepared() Melanie Plageman <[email protected]>
2024-03-26 13:10 [PATCH v7 11/16] Remove heap_freeze_execute_prepared() Melanie Plageman <[email protected]>

This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox