public inbox for [email protected]  

help / color / mirror / Atom feed

[PATCH v2 06/17] lazy_scan_prune reorder freeze execution logic
26+ messages / 6 participants
[nested] [flat]

* [PATCH v2 06/17] lazy_scan_prune reorder freeze execution logic
@ 2024-01-07 19:50  Melanie Plageman <[email protected]>
  0 siblings, 0 replies; 26+ messages in thread

From: Melanie Plageman @ 2024-01-07 19:50 UTC (permalink / raw)

To combine the prune and freeze records, freezing must be done before a
pruning WAL record is emitted. We will move the freeze execution into
heap_page_prune() in future commits. lazy_scan_prune() currently
executes freezing, updates vacrel->NewRelfrozenXid and
vacrel->NewRelminMxid, and resets the snapshotConflictHorizon that the
visibility map update record may use in the same block of if statements.

This commit starts reordering that logic so that the freeze execution
can be separated from the other updates which should not be done in
pruning. It also adds a helper calculating freeze snapshot conflict
horizon. This will be useful when the freeze execution is moved into
pruning because not all callers of heap_page_prune() have access to
VacuumCutoffs.
---
 src/backend/access/heap/vacuumlazy.c | 112 ++++++++++++++++-----------
 1 file changed, 67 insertions(+), 45 deletions(-)

diff --git a/src/backend/access/heap/vacuumlazy.c b/src/backend/access/heap/vacuumlazy.c
index 4187c998d25..abbb7ab3ada 100644
--- a/src/backend/access/heap/vacuumlazy.c
+++ b/src/backend/access/heap/vacuumlazy.c
@@ -269,6 +269,8 @@ static void update_vacuum_error_info(LVRelState *vacrel,
 static void restore_vacuum_error_info(LVRelState *vacrel,
 									  const LVSavedErrInfo *saved_vacrel);
 
+static TransactionId heap_frz_conflict_horizon(PruneResult *presult,
+											   HeapPageFreeze *pagefrz);
 
 /*
  *	heap_vacuum_rel() -- perform VACUUM for one heap relation
@@ -1373,6 +1375,33 @@ lazy_scan_new_or_empty(LVRelState *vacrel, Buffer buf, BlockNumber blkno,
 	return false;
 }
 
+/*
+ * Determine the snapshotConflictHorizon for freezing. Must only be called
+ * after pruning and determining if the page is freezable.
+ */
+static TransactionId
+heap_frz_conflict_horizon(PruneResult *presult, HeapPageFreeze *pagefrz)
+{
+	TransactionId result;
+
+	/*
+	 * 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)
+		result = presult->frz_conflict_horizon;
+	else
+	{
+		/* Avoids false conflicts when hot_standby_feedback in use */
+		result = pagefrz->cutoffs->OldestXmin;
+		TransactionIdRetreat(result);
+	}
+
+	return result;
+}
+
 /*
  *	lazy_scan_prune() -- lazy_scan_heap() pruning and freezing.
  *
@@ -1421,6 +1450,7 @@ lazy_scan_prune(LVRelState *vacrel,
 				recently_dead_tuples;
 	HeapPageFreeze pagefrz;
 	bool		hastup = false;
+	bool		do_freeze;
 	int64		fpi_before = pgWalUsage.wal_fpi;
 	OffsetNumber deadoffsets[MaxHeapTuplesPerPage];
 
@@ -1580,10 +1610,15 @@ lazy_scan_prune(LVRelState *vacrel,
 	 * freeze when pruning generated an FPI, if doing so means that we set the
 	 * page all-frozen afterwards (might not happen until final heap pass).
 	 */
-	if (pagefrz.freeze_required || presult.nfrozen == 0 ||
+	do_freeze = pagefrz.freeze_required ||
 		(presult.all_visible_except_removable && presult.all_frozen &&
-		 fpi_before != pgWalUsage.wal_fpi))
+		 presult.nfrozen > 0 &&
+		 fpi_before != pgWalUsage.wal_fpi);
+
+	if (do_freeze)
 	{
+		TransactionId snapshotConflictHorizon;
+
 		/*
 		 * We're freezing the page.  Our final NewRelfrozenXid doesn't need to
 		 * be affected by the XIDs that are just about to be frozen anyway.
@@ -1591,52 +1626,39 @@ lazy_scan_prune(LVRelState *vacrel,
 		vacrel->NewRelfrozenXid = pagefrz.FreezePageRelfrozenXid;
 		vacrel->NewRelminMxid = pagefrz.FreezePageRelminMxid;
 
-		if (presult.nfrozen == 0)
-		{
-			/*
-			 * We have no freeze plans to execute, so there's no added cost
-			 * from following the freeze path.  That's why it was chosen. This
-			 * is important in the case where the page only contains totally
-			 * frozen tuples at this point (perhaps only following pruning).
-			 * Such pages can be marked all-frozen in the VM by our caller,
-			 * even though none of its tuples were newly frozen here (note
-			 * that the "no freeze" path never sets pages all-frozen).
-			 *
-			 * We never increment the frozen_pages instrumentation counter
-			 * here, since it only counts pages with newly frozen tuples
-			 * (don't confuse that with pages newly set all-frozen in VM).
-			 */
-		}
-		else
-		{
-			TransactionId snapshotConflictHorizon;
+		vacrel->frozen_pages++;
 
-			vacrel->frozen_pages++;
+		snapshotConflictHorizon = heap_frz_conflict_horizon(&presult, &pagefrz);
 
-			/*
-			 * 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)
-			{
-				/* Using same cutoff when setting VM is now unnecessary */
-				snapshotConflictHorizon = presult.frz_conflict_horizon;
-				presult.frz_conflict_horizon = InvalidTransactionId;
-			}
-			else
-			{
-				/* Avoids false conflicts when hot_standby_feedback in use */
-				snapshotConflictHorizon = vacrel->cutoffs.OldestXmin;
-				TransactionIdRetreat(snapshotConflictHorizon);
-			}
+		/* Using same cutoff when setting VM is now unnecessary */
+		if (presult.all_visible_except_removable && presult.all_frozen)
+			presult.frz_conflict_horizon = InvalidTransactionId;
 
-			/* Execute all freeze plans for page as a single atomic action */
-			heap_freeze_execute_prepared(vacrel->rel, buf,
-										 snapshotConflictHorizon,
-										 presult.frozen, presult.nfrozen);
-		}
+		/* Execute all freeze plans for page as a single atomic action */
+		heap_freeze_execute_prepared(vacrel->rel, buf,
+									 snapshotConflictHorizon,
+									 presult.frozen, presult.nfrozen);
+	}
+	else if (presult.all_frozen && presult.nfrozen == 0)
+	{
+		/* Page should be all visible except to-be-removed tuples */
+		Assert(presult.all_visible_except_removable);
+
+		/*
+		 * We have no freeze plans to execute, so there's no added cost from
+		 * following the freeze path.  That's why it was chosen. This is
+		 * important in the case where the page only contains totally frozen
+		 * tuples at this point (perhaps only following pruning). Such pages
+		 * can be marked all-frozen in the VM by our caller, even though none
+		 * of its tuples were newly frozen here (note that the "no freeze"
+		 * path never sets pages all-frozen).
+		 *
+		 * We never increment the frozen_pages instrumentation counter here,
+		 * since it only counts pages with newly frozen tuples (don't confuse
+		 * that with pages newly set all-frozen in VM).
+		 */
+		vacrel->NewRelfrozenXid = pagefrz.FreezePageRelfrozenXid;
+		vacrel->NewRelminMxid = pagefrz.FreezePageRelminMxid;
 	}
 	else
 	{
-- 
2.40.1


--rdqtp5puvxqotfdw
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
	filename="v2-0007-Execute-freezing-in-heap_page_prune.patch"



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

* [PATCH v3 06/17] lazy_scan_prune reorder freeze execution logic
@ 2024-01-07 19:50  Melanie Plageman <[email protected]>
  0 siblings, 0 replies; 26+ messages in thread

From: Melanie Plageman @ 2024-01-07 19:50 UTC (permalink / raw)

To combine the prune and freeze records, freezing must be done before a
pruning WAL record is emitted. We will move the freeze execution into
heap_page_prune() in future commits. lazy_scan_prune() currently
executes freezing, updates vacrel->NewRelfrozenXid and
vacrel->NewRelminMxid, and resets the snapshotConflictHorizon that the
visibility map update record may use in the same block of if statements.

This commit starts reordering that logic so that the freeze execution
can be separated from the other updates which should not be done in
pruning. It also adds a helper calculating freeze snapshot conflict
horizon. This will be useful when the freeze execution is moved into
pruning because not all callers of heap_page_prune() have access to
VacuumCutoffs.
---
 src/backend/access/heap/vacuumlazy.c | 112 ++++++++++++++++-----------
 1 file changed, 67 insertions(+), 45 deletions(-)

diff --git a/src/backend/access/heap/vacuumlazy.c b/src/backend/access/heap/vacuumlazy.c
index 4187c998d25..abbb7ab3ada 100644
--- a/src/backend/access/heap/vacuumlazy.c
+++ b/src/backend/access/heap/vacuumlazy.c
@@ -269,6 +269,8 @@ static void update_vacuum_error_info(LVRelState *vacrel,
 static void restore_vacuum_error_info(LVRelState *vacrel,
 									  const LVSavedErrInfo *saved_vacrel);
 
+static TransactionId heap_frz_conflict_horizon(PruneResult *presult,
+											   HeapPageFreeze *pagefrz);
 
 /*
  *	heap_vacuum_rel() -- perform VACUUM for one heap relation
@@ -1373,6 +1375,33 @@ lazy_scan_new_or_empty(LVRelState *vacrel, Buffer buf, BlockNumber blkno,
 	return false;
 }
 
+/*
+ * Determine the snapshotConflictHorizon for freezing. Must only be called
+ * after pruning and determining if the page is freezable.
+ */
+static TransactionId
+heap_frz_conflict_horizon(PruneResult *presult, HeapPageFreeze *pagefrz)
+{
+	TransactionId result;
+
+	/*
+	 * 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)
+		result = presult->frz_conflict_horizon;
+	else
+	{
+		/* Avoids false conflicts when hot_standby_feedback in use */
+		result = pagefrz->cutoffs->OldestXmin;
+		TransactionIdRetreat(result);
+	}
+
+	return result;
+}
+
 /*
  *	lazy_scan_prune() -- lazy_scan_heap() pruning and freezing.
  *
@@ -1421,6 +1450,7 @@ lazy_scan_prune(LVRelState *vacrel,
 				recently_dead_tuples;
 	HeapPageFreeze pagefrz;
 	bool		hastup = false;
+	bool		do_freeze;
 	int64		fpi_before = pgWalUsage.wal_fpi;
 	OffsetNumber deadoffsets[MaxHeapTuplesPerPage];
 
@@ -1580,10 +1610,15 @@ lazy_scan_prune(LVRelState *vacrel,
 	 * freeze when pruning generated an FPI, if doing so means that we set the
 	 * page all-frozen afterwards (might not happen until final heap pass).
 	 */
-	if (pagefrz.freeze_required || presult.nfrozen == 0 ||
+	do_freeze = pagefrz.freeze_required ||
 		(presult.all_visible_except_removable && presult.all_frozen &&
-		 fpi_before != pgWalUsage.wal_fpi))
+		 presult.nfrozen > 0 &&
+		 fpi_before != pgWalUsage.wal_fpi);
+
+	if (do_freeze)
 	{
+		TransactionId snapshotConflictHorizon;
+
 		/*
 		 * We're freezing the page.  Our final NewRelfrozenXid doesn't need to
 		 * be affected by the XIDs that are just about to be frozen anyway.
@@ -1591,52 +1626,39 @@ lazy_scan_prune(LVRelState *vacrel,
 		vacrel->NewRelfrozenXid = pagefrz.FreezePageRelfrozenXid;
 		vacrel->NewRelminMxid = pagefrz.FreezePageRelminMxid;
 
-		if (presult.nfrozen == 0)
-		{
-			/*
-			 * We have no freeze plans to execute, so there's no added cost
-			 * from following the freeze path.  That's why it was chosen. This
-			 * is important in the case where the page only contains totally
-			 * frozen tuples at this point (perhaps only following pruning).
-			 * Such pages can be marked all-frozen in the VM by our caller,
-			 * even though none of its tuples were newly frozen here (note
-			 * that the "no freeze" path never sets pages all-frozen).
-			 *
-			 * We never increment the frozen_pages instrumentation counter
-			 * here, since it only counts pages with newly frozen tuples
-			 * (don't confuse that with pages newly set all-frozen in VM).
-			 */
-		}
-		else
-		{
-			TransactionId snapshotConflictHorizon;
+		vacrel->frozen_pages++;
 
-			vacrel->frozen_pages++;
+		snapshotConflictHorizon = heap_frz_conflict_horizon(&presult, &pagefrz);
 
-			/*
-			 * 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)
-			{
-				/* Using same cutoff when setting VM is now unnecessary */
-				snapshotConflictHorizon = presult.frz_conflict_horizon;
-				presult.frz_conflict_horizon = InvalidTransactionId;
-			}
-			else
-			{
-				/* Avoids false conflicts when hot_standby_feedback in use */
-				snapshotConflictHorizon = vacrel->cutoffs.OldestXmin;
-				TransactionIdRetreat(snapshotConflictHorizon);
-			}
+		/* Using same cutoff when setting VM is now unnecessary */
+		if (presult.all_visible_except_removable && presult.all_frozen)
+			presult.frz_conflict_horizon = InvalidTransactionId;
 
-			/* Execute all freeze plans for page as a single atomic action */
-			heap_freeze_execute_prepared(vacrel->rel, buf,
-										 snapshotConflictHorizon,
-										 presult.frozen, presult.nfrozen);
-		}
+		/* Execute all freeze plans for page as a single atomic action */
+		heap_freeze_execute_prepared(vacrel->rel, buf,
+									 snapshotConflictHorizon,
+									 presult.frozen, presult.nfrozen);
+	}
+	else if (presult.all_frozen && presult.nfrozen == 0)
+	{
+		/* Page should be all visible except to-be-removed tuples */
+		Assert(presult.all_visible_except_removable);
+
+		/*
+		 * We have no freeze plans to execute, so there's no added cost from
+		 * following the freeze path.  That's why it was chosen. This is
+		 * important in the case where the page only contains totally frozen
+		 * tuples at this point (perhaps only following pruning). Such pages
+		 * can be marked all-frozen in the VM by our caller, even though none
+		 * of its tuples were newly frozen here (note that the "no freeze"
+		 * path never sets pages all-frozen).
+		 *
+		 * We never increment the frozen_pages instrumentation counter here,
+		 * since it only counts pages with newly frozen tuples (don't confuse
+		 * that with pages newly set all-frozen in VM).
+		 */
+		vacrel->NewRelfrozenXid = pagefrz.FreezePageRelfrozenXid;
+		vacrel->NewRelminMxid = pagefrz.FreezePageRelminMxid;
 	}
 	else
 	{
-- 
2.40.1


--racicctn4wry6xe5
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
	filename="v3-0007-Execute-freezing-in-heap_page_prune.patch"



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

* [PATCH v2 06/17] lazy_scan_prune reorder freeze execution logic
@ 2024-01-07 19:50  Melanie Plageman <[email protected]>
  0 siblings, 0 replies; 26+ messages in thread

From: Melanie Plageman @ 2024-01-07 19:50 UTC (permalink / raw)

To combine the prune and freeze records, freezing must be done before a
pruning WAL record is emitted. We will move the freeze execution into
heap_page_prune() in future commits. lazy_scan_prune() currently
executes freezing, updates vacrel->NewRelfrozenXid and
vacrel->NewRelminMxid, and resets the snapshotConflictHorizon that the
visibility map update record may use in the same block of if statements.

This commit starts reordering that logic so that the freeze execution
can be separated from the other updates which should not be done in
pruning. It also adds a helper calculating freeze snapshot conflict
horizon. This will be useful when the freeze execution is moved into
pruning because not all callers of heap_page_prune() have access to
VacuumCutoffs.
---
 src/backend/access/heap/vacuumlazy.c | 112 ++++++++++++++++-----------
 1 file changed, 67 insertions(+), 45 deletions(-)

diff --git a/src/backend/access/heap/vacuumlazy.c b/src/backend/access/heap/vacuumlazy.c
index 4187c998d25..abbb7ab3ada 100644
--- a/src/backend/access/heap/vacuumlazy.c
+++ b/src/backend/access/heap/vacuumlazy.c
@@ -269,6 +269,8 @@ static void update_vacuum_error_info(LVRelState *vacrel,
 static void restore_vacuum_error_info(LVRelState *vacrel,
 									  const LVSavedErrInfo *saved_vacrel);
 
+static TransactionId heap_frz_conflict_horizon(PruneResult *presult,
+											   HeapPageFreeze *pagefrz);
 
 /*
  *	heap_vacuum_rel() -- perform VACUUM for one heap relation
@@ -1373,6 +1375,33 @@ lazy_scan_new_or_empty(LVRelState *vacrel, Buffer buf, BlockNumber blkno,
 	return false;
 }
 
+/*
+ * Determine the snapshotConflictHorizon for freezing. Must only be called
+ * after pruning and determining if the page is freezable.
+ */
+static TransactionId
+heap_frz_conflict_horizon(PruneResult *presult, HeapPageFreeze *pagefrz)
+{
+	TransactionId result;
+
+	/*
+	 * 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)
+		result = presult->frz_conflict_horizon;
+	else
+	{
+		/* Avoids false conflicts when hot_standby_feedback in use */
+		result = pagefrz->cutoffs->OldestXmin;
+		TransactionIdRetreat(result);
+	}
+
+	return result;
+}
+
 /*
  *	lazy_scan_prune() -- lazy_scan_heap() pruning and freezing.
  *
@@ -1421,6 +1450,7 @@ lazy_scan_prune(LVRelState *vacrel,
 				recently_dead_tuples;
 	HeapPageFreeze pagefrz;
 	bool		hastup = false;
+	bool		do_freeze;
 	int64		fpi_before = pgWalUsage.wal_fpi;
 	OffsetNumber deadoffsets[MaxHeapTuplesPerPage];
 
@@ -1580,10 +1610,15 @@ lazy_scan_prune(LVRelState *vacrel,
 	 * freeze when pruning generated an FPI, if doing so means that we set the
 	 * page all-frozen afterwards (might not happen until final heap pass).
 	 */
-	if (pagefrz.freeze_required || presult.nfrozen == 0 ||
+	do_freeze = pagefrz.freeze_required ||
 		(presult.all_visible_except_removable && presult.all_frozen &&
-		 fpi_before != pgWalUsage.wal_fpi))
+		 presult.nfrozen > 0 &&
+		 fpi_before != pgWalUsage.wal_fpi);
+
+	if (do_freeze)
 	{
+		TransactionId snapshotConflictHorizon;
+
 		/*
 		 * We're freezing the page.  Our final NewRelfrozenXid doesn't need to
 		 * be affected by the XIDs that are just about to be frozen anyway.
@@ -1591,52 +1626,39 @@ lazy_scan_prune(LVRelState *vacrel,
 		vacrel->NewRelfrozenXid = pagefrz.FreezePageRelfrozenXid;
 		vacrel->NewRelminMxid = pagefrz.FreezePageRelminMxid;
 
-		if (presult.nfrozen == 0)
-		{
-			/*
-			 * We have no freeze plans to execute, so there's no added cost
-			 * from following the freeze path.  That's why it was chosen. This
-			 * is important in the case where the page only contains totally
-			 * frozen tuples at this point (perhaps only following pruning).
-			 * Such pages can be marked all-frozen in the VM by our caller,
-			 * even though none of its tuples were newly frozen here (note
-			 * that the "no freeze" path never sets pages all-frozen).
-			 *
-			 * We never increment the frozen_pages instrumentation counter
-			 * here, since it only counts pages with newly frozen tuples
-			 * (don't confuse that with pages newly set all-frozen in VM).
-			 */
-		}
-		else
-		{
-			TransactionId snapshotConflictHorizon;
+		vacrel->frozen_pages++;
 
-			vacrel->frozen_pages++;
+		snapshotConflictHorizon = heap_frz_conflict_horizon(&presult, &pagefrz);
 
-			/*
-			 * 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)
-			{
-				/* Using same cutoff when setting VM is now unnecessary */
-				snapshotConflictHorizon = presult.frz_conflict_horizon;
-				presult.frz_conflict_horizon = InvalidTransactionId;
-			}
-			else
-			{
-				/* Avoids false conflicts when hot_standby_feedback in use */
-				snapshotConflictHorizon = vacrel->cutoffs.OldestXmin;
-				TransactionIdRetreat(snapshotConflictHorizon);
-			}
+		/* Using same cutoff when setting VM is now unnecessary */
+		if (presult.all_visible_except_removable && presult.all_frozen)
+			presult.frz_conflict_horizon = InvalidTransactionId;
 
-			/* Execute all freeze plans for page as a single atomic action */
-			heap_freeze_execute_prepared(vacrel->rel, buf,
-										 snapshotConflictHorizon,
-										 presult.frozen, presult.nfrozen);
-		}
+		/* Execute all freeze plans for page as a single atomic action */
+		heap_freeze_execute_prepared(vacrel->rel, buf,
+									 snapshotConflictHorizon,
+									 presult.frozen, presult.nfrozen);
+	}
+	else if (presult.all_frozen && presult.nfrozen == 0)
+	{
+		/* Page should be all visible except to-be-removed tuples */
+		Assert(presult.all_visible_except_removable);
+
+		/*
+		 * We have no freeze plans to execute, so there's no added cost from
+		 * following the freeze path.  That's why it was chosen. This is
+		 * important in the case where the page only contains totally frozen
+		 * tuples at this point (perhaps only following pruning). Such pages
+		 * can be marked all-frozen in the VM by our caller, even though none
+		 * of its tuples were newly frozen here (note that the "no freeze"
+		 * path never sets pages all-frozen).
+		 *
+		 * We never increment the frozen_pages instrumentation counter here,
+		 * since it only counts pages with newly frozen tuples (don't confuse
+		 * that with pages newly set all-frozen in VM).
+		 */
+		vacrel->NewRelfrozenXid = pagefrz.FreezePageRelfrozenXid;
+		vacrel->NewRelminMxid = pagefrz.FreezePageRelminMxid;
 	}
 	else
 	{
-- 
2.40.1


--rdqtp5puvxqotfdw
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
	filename="v2-0007-Execute-freezing-in-heap_page_prune.patch"



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

* [PATCH v3 06/17] lazy_scan_prune reorder freeze execution logic
@ 2024-01-07 19:50  Melanie Plageman <[email protected]>
  0 siblings, 0 replies; 26+ messages in thread

From: Melanie Plageman @ 2024-01-07 19:50 UTC (permalink / raw)

To combine the prune and freeze records, freezing must be done before a
pruning WAL record is emitted. We will move the freeze execution into
heap_page_prune() in future commits. lazy_scan_prune() currently
executes freezing, updates vacrel->NewRelfrozenXid and
vacrel->NewRelminMxid, and resets the snapshotConflictHorizon that the
visibility map update record may use in the same block of if statements.

This commit starts reordering that logic so that the freeze execution
can be separated from the other updates which should not be done in
pruning. It also adds a helper calculating freeze snapshot conflict
horizon. This will be useful when the freeze execution is moved into
pruning because not all callers of heap_page_prune() have access to
VacuumCutoffs.
---
 src/backend/access/heap/vacuumlazy.c | 112 ++++++++++++++++-----------
 1 file changed, 67 insertions(+), 45 deletions(-)

diff --git a/src/backend/access/heap/vacuumlazy.c b/src/backend/access/heap/vacuumlazy.c
index 4187c998d25..abbb7ab3ada 100644
--- a/src/backend/access/heap/vacuumlazy.c
+++ b/src/backend/access/heap/vacuumlazy.c
@@ -269,6 +269,8 @@ static void update_vacuum_error_info(LVRelState *vacrel,
 static void restore_vacuum_error_info(LVRelState *vacrel,
 									  const LVSavedErrInfo *saved_vacrel);
 
+static TransactionId heap_frz_conflict_horizon(PruneResult *presult,
+											   HeapPageFreeze *pagefrz);
 
 /*
  *	heap_vacuum_rel() -- perform VACUUM for one heap relation
@@ -1373,6 +1375,33 @@ lazy_scan_new_or_empty(LVRelState *vacrel, Buffer buf, BlockNumber blkno,
 	return false;
 }
 
+/*
+ * Determine the snapshotConflictHorizon for freezing. Must only be called
+ * after pruning and determining if the page is freezable.
+ */
+static TransactionId
+heap_frz_conflict_horizon(PruneResult *presult, HeapPageFreeze *pagefrz)
+{
+	TransactionId result;
+
+	/*
+	 * 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)
+		result = presult->frz_conflict_horizon;
+	else
+	{
+		/* Avoids false conflicts when hot_standby_feedback in use */
+		result = pagefrz->cutoffs->OldestXmin;
+		TransactionIdRetreat(result);
+	}
+
+	return result;
+}
+
 /*
  *	lazy_scan_prune() -- lazy_scan_heap() pruning and freezing.
  *
@@ -1421,6 +1450,7 @@ lazy_scan_prune(LVRelState *vacrel,
 				recently_dead_tuples;
 	HeapPageFreeze pagefrz;
 	bool		hastup = false;
+	bool		do_freeze;
 	int64		fpi_before = pgWalUsage.wal_fpi;
 	OffsetNumber deadoffsets[MaxHeapTuplesPerPage];
 
@@ -1580,10 +1610,15 @@ lazy_scan_prune(LVRelState *vacrel,
 	 * freeze when pruning generated an FPI, if doing so means that we set the
 	 * page all-frozen afterwards (might not happen until final heap pass).
 	 */
-	if (pagefrz.freeze_required || presult.nfrozen == 0 ||
+	do_freeze = pagefrz.freeze_required ||
 		(presult.all_visible_except_removable && presult.all_frozen &&
-		 fpi_before != pgWalUsage.wal_fpi))
+		 presult.nfrozen > 0 &&
+		 fpi_before != pgWalUsage.wal_fpi);
+
+	if (do_freeze)
 	{
+		TransactionId snapshotConflictHorizon;
+
 		/*
 		 * We're freezing the page.  Our final NewRelfrozenXid doesn't need to
 		 * be affected by the XIDs that are just about to be frozen anyway.
@@ -1591,52 +1626,39 @@ lazy_scan_prune(LVRelState *vacrel,
 		vacrel->NewRelfrozenXid = pagefrz.FreezePageRelfrozenXid;
 		vacrel->NewRelminMxid = pagefrz.FreezePageRelminMxid;
 
-		if (presult.nfrozen == 0)
-		{
-			/*
-			 * We have no freeze plans to execute, so there's no added cost
-			 * from following the freeze path.  That's why it was chosen. This
-			 * is important in the case where the page only contains totally
-			 * frozen tuples at this point (perhaps only following pruning).
-			 * Such pages can be marked all-frozen in the VM by our caller,
-			 * even though none of its tuples were newly frozen here (note
-			 * that the "no freeze" path never sets pages all-frozen).
-			 *
-			 * We never increment the frozen_pages instrumentation counter
-			 * here, since it only counts pages with newly frozen tuples
-			 * (don't confuse that with pages newly set all-frozen in VM).
-			 */
-		}
-		else
-		{
-			TransactionId snapshotConflictHorizon;
+		vacrel->frozen_pages++;
 
-			vacrel->frozen_pages++;
+		snapshotConflictHorizon = heap_frz_conflict_horizon(&presult, &pagefrz);
 
-			/*
-			 * 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)
-			{
-				/* Using same cutoff when setting VM is now unnecessary */
-				snapshotConflictHorizon = presult.frz_conflict_horizon;
-				presult.frz_conflict_horizon = InvalidTransactionId;
-			}
-			else
-			{
-				/* Avoids false conflicts when hot_standby_feedback in use */
-				snapshotConflictHorizon = vacrel->cutoffs.OldestXmin;
-				TransactionIdRetreat(snapshotConflictHorizon);
-			}
+		/* Using same cutoff when setting VM is now unnecessary */
+		if (presult.all_visible_except_removable && presult.all_frozen)
+			presult.frz_conflict_horizon = InvalidTransactionId;
 
-			/* Execute all freeze plans for page as a single atomic action */
-			heap_freeze_execute_prepared(vacrel->rel, buf,
-										 snapshotConflictHorizon,
-										 presult.frozen, presult.nfrozen);
-		}
+		/* Execute all freeze plans for page as a single atomic action */
+		heap_freeze_execute_prepared(vacrel->rel, buf,
+									 snapshotConflictHorizon,
+									 presult.frozen, presult.nfrozen);
+	}
+	else if (presult.all_frozen && presult.nfrozen == 0)
+	{
+		/* Page should be all visible except to-be-removed tuples */
+		Assert(presult.all_visible_except_removable);
+
+		/*
+		 * We have no freeze plans to execute, so there's no added cost from
+		 * following the freeze path.  That's why it was chosen. This is
+		 * important in the case where the page only contains totally frozen
+		 * tuples at this point (perhaps only following pruning). Such pages
+		 * can be marked all-frozen in the VM by our caller, even though none
+		 * of its tuples were newly frozen here (note that the "no freeze"
+		 * path never sets pages all-frozen).
+		 *
+		 * We never increment the frozen_pages instrumentation counter here,
+		 * since it only counts pages with newly frozen tuples (don't confuse
+		 * that with pages newly set all-frozen in VM).
+		 */
+		vacrel->NewRelfrozenXid = pagefrz.FreezePageRelfrozenXid;
+		vacrel->NewRelminMxid = pagefrz.FreezePageRelminMxid;
 	}
 	else
 	{
-- 
2.40.1


--racicctn4wry6xe5
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
	filename="v3-0007-Execute-freezing-in-heap_page_prune.patch"



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

* [PATCH v3 06/17] lazy_scan_prune reorder freeze execution logic
@ 2024-01-07 19:50  Melanie Plageman <[email protected]>
  0 siblings, 0 replies; 26+ messages in thread

From: Melanie Plageman @ 2024-01-07 19:50 UTC (permalink / raw)

To combine the prune and freeze records, freezing must be done before a
pruning WAL record is emitted. We will move the freeze execution into
heap_page_prune() in future commits. lazy_scan_prune() currently
executes freezing, updates vacrel->NewRelfrozenXid and
vacrel->NewRelminMxid, and resets the snapshotConflictHorizon that the
visibility map update record may use in the same block of if statements.

This commit starts reordering that logic so that the freeze execution
can be separated from the other updates which should not be done in
pruning. It also adds a helper calculating freeze snapshot conflict
horizon. This will be useful when the freeze execution is moved into
pruning because not all callers of heap_page_prune() have access to
VacuumCutoffs.
---
 src/backend/access/heap/vacuumlazy.c | 112 ++++++++++++++++-----------
 1 file changed, 67 insertions(+), 45 deletions(-)

diff --git a/src/backend/access/heap/vacuumlazy.c b/src/backend/access/heap/vacuumlazy.c
index 4187c998d25..abbb7ab3ada 100644
--- a/src/backend/access/heap/vacuumlazy.c
+++ b/src/backend/access/heap/vacuumlazy.c
@@ -269,6 +269,8 @@ static void update_vacuum_error_info(LVRelState *vacrel,
 static void restore_vacuum_error_info(LVRelState *vacrel,
 									  const LVSavedErrInfo *saved_vacrel);
 
+static TransactionId heap_frz_conflict_horizon(PruneResult *presult,
+											   HeapPageFreeze *pagefrz);
 
 /*
  *	heap_vacuum_rel() -- perform VACUUM for one heap relation
@@ -1373,6 +1375,33 @@ lazy_scan_new_or_empty(LVRelState *vacrel, Buffer buf, BlockNumber blkno,
 	return false;
 }
 
+/*
+ * Determine the snapshotConflictHorizon for freezing. Must only be called
+ * after pruning and determining if the page is freezable.
+ */
+static TransactionId
+heap_frz_conflict_horizon(PruneResult *presult, HeapPageFreeze *pagefrz)
+{
+	TransactionId result;
+
+	/*
+	 * 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)
+		result = presult->frz_conflict_horizon;
+	else
+	{
+		/* Avoids false conflicts when hot_standby_feedback in use */
+		result = pagefrz->cutoffs->OldestXmin;
+		TransactionIdRetreat(result);
+	}
+
+	return result;
+}
+
 /*
  *	lazy_scan_prune() -- lazy_scan_heap() pruning and freezing.
  *
@@ -1421,6 +1450,7 @@ lazy_scan_prune(LVRelState *vacrel,
 				recently_dead_tuples;
 	HeapPageFreeze pagefrz;
 	bool		hastup = false;
+	bool		do_freeze;
 	int64		fpi_before = pgWalUsage.wal_fpi;
 	OffsetNumber deadoffsets[MaxHeapTuplesPerPage];
 
@@ -1580,10 +1610,15 @@ lazy_scan_prune(LVRelState *vacrel,
 	 * freeze when pruning generated an FPI, if doing so means that we set the
 	 * page all-frozen afterwards (might not happen until final heap pass).
 	 */
-	if (pagefrz.freeze_required || presult.nfrozen == 0 ||
+	do_freeze = pagefrz.freeze_required ||
 		(presult.all_visible_except_removable && presult.all_frozen &&
-		 fpi_before != pgWalUsage.wal_fpi))
+		 presult.nfrozen > 0 &&
+		 fpi_before != pgWalUsage.wal_fpi);
+
+	if (do_freeze)
 	{
+		TransactionId snapshotConflictHorizon;
+
 		/*
 		 * We're freezing the page.  Our final NewRelfrozenXid doesn't need to
 		 * be affected by the XIDs that are just about to be frozen anyway.
@@ -1591,52 +1626,39 @@ lazy_scan_prune(LVRelState *vacrel,
 		vacrel->NewRelfrozenXid = pagefrz.FreezePageRelfrozenXid;
 		vacrel->NewRelminMxid = pagefrz.FreezePageRelminMxid;
 
-		if (presult.nfrozen == 0)
-		{
-			/*
-			 * We have no freeze plans to execute, so there's no added cost
-			 * from following the freeze path.  That's why it was chosen. This
-			 * is important in the case where the page only contains totally
-			 * frozen tuples at this point (perhaps only following pruning).
-			 * Such pages can be marked all-frozen in the VM by our caller,
-			 * even though none of its tuples were newly frozen here (note
-			 * that the "no freeze" path never sets pages all-frozen).
-			 *
-			 * We never increment the frozen_pages instrumentation counter
-			 * here, since it only counts pages with newly frozen tuples
-			 * (don't confuse that with pages newly set all-frozen in VM).
-			 */
-		}
-		else
-		{
-			TransactionId snapshotConflictHorizon;
+		vacrel->frozen_pages++;
 
-			vacrel->frozen_pages++;
+		snapshotConflictHorizon = heap_frz_conflict_horizon(&presult, &pagefrz);
 
-			/*
-			 * 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)
-			{
-				/* Using same cutoff when setting VM is now unnecessary */
-				snapshotConflictHorizon = presult.frz_conflict_horizon;
-				presult.frz_conflict_horizon = InvalidTransactionId;
-			}
-			else
-			{
-				/* Avoids false conflicts when hot_standby_feedback in use */
-				snapshotConflictHorizon = vacrel->cutoffs.OldestXmin;
-				TransactionIdRetreat(snapshotConflictHorizon);
-			}
+		/* Using same cutoff when setting VM is now unnecessary */
+		if (presult.all_visible_except_removable && presult.all_frozen)
+			presult.frz_conflict_horizon = InvalidTransactionId;
 
-			/* Execute all freeze plans for page as a single atomic action */
-			heap_freeze_execute_prepared(vacrel->rel, buf,
-										 snapshotConflictHorizon,
-										 presult.frozen, presult.nfrozen);
-		}
+		/* Execute all freeze plans for page as a single atomic action */
+		heap_freeze_execute_prepared(vacrel->rel, buf,
+									 snapshotConflictHorizon,
+									 presult.frozen, presult.nfrozen);
+	}
+	else if (presult.all_frozen && presult.nfrozen == 0)
+	{
+		/* Page should be all visible except to-be-removed tuples */
+		Assert(presult.all_visible_except_removable);
+
+		/*
+		 * We have no freeze plans to execute, so there's no added cost from
+		 * following the freeze path.  That's why it was chosen. This is
+		 * important in the case where the page only contains totally frozen
+		 * tuples at this point (perhaps only following pruning). Such pages
+		 * can be marked all-frozen in the VM by our caller, even though none
+		 * of its tuples were newly frozen here (note that the "no freeze"
+		 * path never sets pages all-frozen).
+		 *
+		 * We never increment the frozen_pages instrumentation counter here,
+		 * since it only counts pages with newly frozen tuples (don't confuse
+		 * that with pages newly set all-frozen in VM).
+		 */
+		vacrel->NewRelfrozenXid = pagefrz.FreezePageRelfrozenXid;
+		vacrel->NewRelminMxid = pagefrz.FreezePageRelminMxid;
 	}
 	else
 	{
-- 
2.40.1


--racicctn4wry6xe5
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
	filename="v3-0007-Execute-freezing-in-heap_page_prune.patch"



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

* [PATCH v2 06/17] lazy_scan_prune reorder freeze execution logic
@ 2024-01-07 19:50  Melanie Plageman <[email protected]>
  0 siblings, 0 replies; 26+ messages in thread

From: Melanie Plageman @ 2024-01-07 19:50 UTC (permalink / raw)

To combine the prune and freeze records, freezing must be done before a
pruning WAL record is emitted. We will move the freeze execution into
heap_page_prune() in future commits. lazy_scan_prune() currently
executes freezing, updates vacrel->NewRelfrozenXid and
vacrel->NewRelminMxid, and resets the snapshotConflictHorizon that the
visibility map update record may use in the same block of if statements.

This commit starts reordering that logic so that the freeze execution
can be separated from the other updates which should not be done in
pruning. It also adds a helper calculating freeze snapshot conflict
horizon. This will be useful when the freeze execution is moved into
pruning because not all callers of heap_page_prune() have access to
VacuumCutoffs.
---
 src/backend/access/heap/vacuumlazy.c | 112 ++++++++++++++++-----------
 1 file changed, 67 insertions(+), 45 deletions(-)

diff --git a/src/backend/access/heap/vacuumlazy.c b/src/backend/access/heap/vacuumlazy.c
index 4187c998d25..abbb7ab3ada 100644
--- a/src/backend/access/heap/vacuumlazy.c
+++ b/src/backend/access/heap/vacuumlazy.c
@@ -269,6 +269,8 @@ static void update_vacuum_error_info(LVRelState *vacrel,
 static void restore_vacuum_error_info(LVRelState *vacrel,
 									  const LVSavedErrInfo *saved_vacrel);
 
+static TransactionId heap_frz_conflict_horizon(PruneResult *presult,
+											   HeapPageFreeze *pagefrz);
 
 /*
  *	heap_vacuum_rel() -- perform VACUUM for one heap relation
@@ -1373,6 +1375,33 @@ lazy_scan_new_or_empty(LVRelState *vacrel, Buffer buf, BlockNumber blkno,
 	return false;
 }
 
+/*
+ * Determine the snapshotConflictHorizon for freezing. Must only be called
+ * after pruning and determining if the page is freezable.
+ */
+static TransactionId
+heap_frz_conflict_horizon(PruneResult *presult, HeapPageFreeze *pagefrz)
+{
+	TransactionId result;
+
+	/*
+	 * 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)
+		result = presult->frz_conflict_horizon;
+	else
+	{
+		/* Avoids false conflicts when hot_standby_feedback in use */
+		result = pagefrz->cutoffs->OldestXmin;
+		TransactionIdRetreat(result);
+	}
+
+	return result;
+}
+
 /*
  *	lazy_scan_prune() -- lazy_scan_heap() pruning and freezing.
  *
@@ -1421,6 +1450,7 @@ lazy_scan_prune(LVRelState *vacrel,
 				recently_dead_tuples;
 	HeapPageFreeze pagefrz;
 	bool		hastup = false;
+	bool		do_freeze;
 	int64		fpi_before = pgWalUsage.wal_fpi;
 	OffsetNumber deadoffsets[MaxHeapTuplesPerPage];
 
@@ -1580,10 +1610,15 @@ lazy_scan_prune(LVRelState *vacrel,
 	 * freeze when pruning generated an FPI, if doing so means that we set the
 	 * page all-frozen afterwards (might not happen until final heap pass).
 	 */
-	if (pagefrz.freeze_required || presult.nfrozen == 0 ||
+	do_freeze = pagefrz.freeze_required ||
 		(presult.all_visible_except_removable && presult.all_frozen &&
-		 fpi_before != pgWalUsage.wal_fpi))
+		 presult.nfrozen > 0 &&
+		 fpi_before != pgWalUsage.wal_fpi);
+
+	if (do_freeze)
 	{
+		TransactionId snapshotConflictHorizon;
+
 		/*
 		 * We're freezing the page.  Our final NewRelfrozenXid doesn't need to
 		 * be affected by the XIDs that are just about to be frozen anyway.
@@ -1591,52 +1626,39 @@ lazy_scan_prune(LVRelState *vacrel,
 		vacrel->NewRelfrozenXid = pagefrz.FreezePageRelfrozenXid;
 		vacrel->NewRelminMxid = pagefrz.FreezePageRelminMxid;
 
-		if (presult.nfrozen == 0)
-		{
-			/*
-			 * We have no freeze plans to execute, so there's no added cost
-			 * from following the freeze path.  That's why it was chosen. This
-			 * is important in the case where the page only contains totally
-			 * frozen tuples at this point (perhaps only following pruning).
-			 * Such pages can be marked all-frozen in the VM by our caller,
-			 * even though none of its tuples were newly frozen here (note
-			 * that the "no freeze" path never sets pages all-frozen).
-			 *
-			 * We never increment the frozen_pages instrumentation counter
-			 * here, since it only counts pages with newly frozen tuples
-			 * (don't confuse that with pages newly set all-frozen in VM).
-			 */
-		}
-		else
-		{
-			TransactionId snapshotConflictHorizon;
+		vacrel->frozen_pages++;
 
-			vacrel->frozen_pages++;
+		snapshotConflictHorizon = heap_frz_conflict_horizon(&presult, &pagefrz);
 
-			/*
-			 * 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)
-			{
-				/* Using same cutoff when setting VM is now unnecessary */
-				snapshotConflictHorizon = presult.frz_conflict_horizon;
-				presult.frz_conflict_horizon = InvalidTransactionId;
-			}
-			else
-			{
-				/* Avoids false conflicts when hot_standby_feedback in use */
-				snapshotConflictHorizon = vacrel->cutoffs.OldestXmin;
-				TransactionIdRetreat(snapshotConflictHorizon);
-			}
+		/* Using same cutoff when setting VM is now unnecessary */
+		if (presult.all_visible_except_removable && presult.all_frozen)
+			presult.frz_conflict_horizon = InvalidTransactionId;
 
-			/* Execute all freeze plans for page as a single atomic action */
-			heap_freeze_execute_prepared(vacrel->rel, buf,
-										 snapshotConflictHorizon,
-										 presult.frozen, presult.nfrozen);
-		}
+		/* Execute all freeze plans for page as a single atomic action */
+		heap_freeze_execute_prepared(vacrel->rel, buf,
+									 snapshotConflictHorizon,
+									 presult.frozen, presult.nfrozen);
+	}
+	else if (presult.all_frozen && presult.nfrozen == 0)
+	{
+		/* Page should be all visible except to-be-removed tuples */
+		Assert(presult.all_visible_except_removable);
+
+		/*
+		 * We have no freeze plans to execute, so there's no added cost from
+		 * following the freeze path.  That's why it was chosen. This is
+		 * important in the case where the page only contains totally frozen
+		 * tuples at this point (perhaps only following pruning). Such pages
+		 * can be marked all-frozen in the VM by our caller, even though none
+		 * of its tuples were newly frozen here (note that the "no freeze"
+		 * path never sets pages all-frozen).
+		 *
+		 * We never increment the frozen_pages instrumentation counter here,
+		 * since it only counts pages with newly frozen tuples (don't confuse
+		 * that with pages newly set all-frozen in VM).
+		 */
+		vacrel->NewRelfrozenXid = pagefrz.FreezePageRelfrozenXid;
+		vacrel->NewRelminMxid = pagefrz.FreezePageRelminMxid;
 	}
 	else
 	{
-- 
2.40.1


--rdqtp5puvxqotfdw
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
	filename="v2-0007-Execute-freezing-in-heap_page_prune.patch"



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

* [PATCH v4 08/19] lazy_scan_prune reorder freeze execution logic
@ 2024-03-19 23:30  Melanie Plageman <[email protected]>
  0 siblings, 0 replies; 26+ messages in thread

From: Melanie Plageman @ 2024-03-19 23:30 UTC (permalink / raw)

To combine the prune and freeze records, freezing must be done before a
pruning WAL record is emitted. We will move the freeze execution into
heap_page_prune() in future commits. lazy_scan_prune() currently
executes freezing, updates vacrel->NewRelfrozenXid and
vacrel->NewRelminMxid, and resets the snapshotConflictHorizon that the
visibility map update record may use in the same block of if statements.

This commit starts reordering that logic so that the freeze execution
can be separated from the other updates which should not be done in
pruning.
---
 src/backend/access/heap/vacuumlazy.c | 92 +++++++++++++++-------------
 1 file changed, 49 insertions(+), 43 deletions(-)

diff --git a/src/backend/access/heap/vacuumlazy.c b/src/backend/access/heap/vacuumlazy.c
index 4187c998d25..74ebab25a95 100644
--- a/src/backend/access/heap/vacuumlazy.c
+++ b/src/backend/access/heap/vacuumlazy.c
@@ -1421,6 +1421,7 @@ lazy_scan_prune(LVRelState *vacrel,
 				recently_dead_tuples;
 	HeapPageFreeze pagefrz;
 	bool		hastup = false;
+	bool		do_freeze;
 	int64		fpi_before = pgWalUsage.wal_fpi;
 	OffsetNumber deadoffsets[MaxHeapTuplesPerPage];
 
@@ -1580,10 +1581,15 @@ lazy_scan_prune(LVRelState *vacrel,
 	 * freeze when pruning generated an FPI, if doing so means that we set the
 	 * page all-frozen afterwards (might not happen until final heap pass).
 	 */
-	if (pagefrz.freeze_required || presult.nfrozen == 0 ||
+	do_freeze = pagefrz.freeze_required ||
 		(presult.all_visible_except_removable && presult.all_frozen &&
-		 fpi_before != pgWalUsage.wal_fpi))
+		 presult.nfrozen > 0 &&
+		 fpi_before != pgWalUsage.wal_fpi);
+
+	if (do_freeze)
 	{
+		TransactionId snapshotConflictHorizon;
+
 		/*
 		 * We're freezing the page.  Our final NewRelfrozenXid doesn't need to
 		 * be affected by the XIDs that are just about to be frozen anyway.
@@ -1591,52 +1597,52 @@ lazy_scan_prune(LVRelState *vacrel,
 		vacrel->NewRelfrozenXid = pagefrz.FreezePageRelfrozenXid;
 		vacrel->NewRelminMxid = pagefrz.FreezePageRelminMxid;
 
-		if (presult.nfrozen == 0)
-		{
-			/*
-			 * We have no freeze plans to execute, so there's no added cost
-			 * from following the freeze path.  That's why it was chosen. This
-			 * is important in the case where the page only contains totally
-			 * frozen tuples at this point (perhaps only following pruning).
-			 * Such pages can be marked all-frozen in the VM by our caller,
-			 * even though none of its tuples were newly frozen here (note
-			 * that the "no freeze" path never sets pages all-frozen).
-			 *
-			 * We never increment the frozen_pages instrumentation counter
-			 * here, since it only counts pages with newly frozen tuples
-			 * (don't confuse that with pages newly set all-frozen in VM).
-			 */
-		}
+		vacrel->frozen_pages++;
+
+		/*
+		 * 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)
+			snapshotConflictHorizon = presult.frz_conflict_horizon;
 		else
 		{
-			TransactionId snapshotConflictHorizon;
+			/* Avoids false conflicts when hot_standby_feedback in use */
+			snapshotConflictHorizon = pagefrz.cutoffs->OldestXmin;
+			TransactionIdRetreat(snapshotConflictHorizon);
+		}
 
-			vacrel->frozen_pages++;
+		/* Using same cutoff when setting VM is now unnecessary */
+		if (presult.all_visible_except_removable && presult.all_frozen)
+			presult.frz_conflict_horizon = InvalidTransactionId;
 
-			/*
-			 * 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)
-			{
-				/* Using same cutoff when setting VM is now unnecessary */
-				snapshotConflictHorizon = presult.frz_conflict_horizon;
-				presult.frz_conflict_horizon = InvalidTransactionId;
-			}
-			else
-			{
-				/* Avoids false conflicts when hot_standby_feedback in use */
-				snapshotConflictHorizon = vacrel->cutoffs.OldestXmin;
-				TransactionIdRetreat(snapshotConflictHorizon);
-			}
+		/* Execute all freeze plans for page as a single atomic action */
+		heap_freeze_execute_prepared(vacrel->rel, buf,
+									 snapshotConflictHorizon,
+									 presult.frozen, presult.nfrozen);
+	}
+	else if (presult.all_frozen && presult.nfrozen == 0)
+	{
+		/* Page should be all visible except to-be-removed tuples */
+		Assert(presult.all_visible_except_removable);
 
-			/* Execute all freeze plans for page as a single atomic action */
-			heap_freeze_execute_prepared(vacrel->rel, buf,
-										 snapshotConflictHorizon,
-										 presult.frozen, presult.nfrozen);
-		}
+		/*
+		 * We have no freeze plans to execute, so there's no added cost from
+		 * following the freeze path.  That's why it was chosen. This is
+		 * important in the case where the page only contains totally frozen
+		 * tuples at this point (perhaps only following pruning). Such pages
+		 * can be marked all-frozen in the VM by our caller, even though none
+		 * of its tuples were newly frozen here (note that the "no freeze"
+		 * path never sets pages all-frozen).
+		 *
+		 * We never increment the frozen_pages instrumentation counter here,
+		 * since it only counts pages with newly frozen tuples (don't confuse
+		 * that with pages newly set all-frozen in VM).
+		 */
+		vacrel->NewRelfrozenXid = pagefrz.FreezePageRelfrozenXid;
+		vacrel->NewRelminMxid = pagefrz.FreezePageRelminMxid;
 	}
 	else
 	{
-- 
2.40.1


--tez7m2a73jtztiij
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
	filename="v4-0009-Execute-freezing-in-heap_page_prune.patch"



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

* [PATCH v4 08/19] lazy_scan_prune reorder freeze execution logic
@ 2024-03-19 23:30  Melanie Plageman <[email protected]>
  0 siblings, 0 replies; 26+ messages in thread

From: Melanie Plageman @ 2024-03-19 23:30 UTC (permalink / raw)

To combine the prune and freeze records, freezing must be done before a
pruning WAL record is emitted. We will move the freeze execution into
heap_page_prune() in future commits. lazy_scan_prune() currently
executes freezing, updates vacrel->NewRelfrozenXid and
vacrel->NewRelminMxid, and resets the snapshotConflictHorizon that the
visibility map update record may use in the same block of if statements.

This commit starts reordering that logic so that the freeze execution
can be separated from the other updates which should not be done in
pruning.
---
 src/backend/access/heap/vacuumlazy.c | 92 +++++++++++++++-------------
 1 file changed, 49 insertions(+), 43 deletions(-)

diff --git a/src/backend/access/heap/vacuumlazy.c b/src/backend/access/heap/vacuumlazy.c
index 4187c998d25..74ebab25a95 100644
--- a/src/backend/access/heap/vacuumlazy.c
+++ b/src/backend/access/heap/vacuumlazy.c
@@ -1421,6 +1421,7 @@ lazy_scan_prune(LVRelState *vacrel,
 				recently_dead_tuples;
 	HeapPageFreeze pagefrz;
 	bool		hastup = false;
+	bool		do_freeze;
 	int64		fpi_before = pgWalUsage.wal_fpi;
 	OffsetNumber deadoffsets[MaxHeapTuplesPerPage];
 
@@ -1580,10 +1581,15 @@ lazy_scan_prune(LVRelState *vacrel,
 	 * freeze when pruning generated an FPI, if doing so means that we set the
 	 * page all-frozen afterwards (might not happen until final heap pass).
 	 */
-	if (pagefrz.freeze_required || presult.nfrozen == 0 ||
+	do_freeze = pagefrz.freeze_required ||
 		(presult.all_visible_except_removable && presult.all_frozen &&
-		 fpi_before != pgWalUsage.wal_fpi))
+		 presult.nfrozen > 0 &&
+		 fpi_before != pgWalUsage.wal_fpi);
+
+	if (do_freeze)
 	{
+		TransactionId snapshotConflictHorizon;
+
 		/*
 		 * We're freezing the page.  Our final NewRelfrozenXid doesn't need to
 		 * be affected by the XIDs that are just about to be frozen anyway.
@@ -1591,52 +1597,52 @@ lazy_scan_prune(LVRelState *vacrel,
 		vacrel->NewRelfrozenXid = pagefrz.FreezePageRelfrozenXid;
 		vacrel->NewRelminMxid = pagefrz.FreezePageRelminMxid;
 
-		if (presult.nfrozen == 0)
-		{
-			/*
-			 * We have no freeze plans to execute, so there's no added cost
-			 * from following the freeze path.  That's why it was chosen. This
-			 * is important in the case where the page only contains totally
-			 * frozen tuples at this point (perhaps only following pruning).
-			 * Such pages can be marked all-frozen in the VM by our caller,
-			 * even though none of its tuples were newly frozen here (note
-			 * that the "no freeze" path never sets pages all-frozen).
-			 *
-			 * We never increment the frozen_pages instrumentation counter
-			 * here, since it only counts pages with newly frozen tuples
-			 * (don't confuse that with pages newly set all-frozen in VM).
-			 */
-		}
+		vacrel->frozen_pages++;
+
+		/*
+		 * 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)
+			snapshotConflictHorizon = presult.frz_conflict_horizon;
 		else
 		{
-			TransactionId snapshotConflictHorizon;
+			/* Avoids false conflicts when hot_standby_feedback in use */
+			snapshotConflictHorizon = pagefrz.cutoffs->OldestXmin;
+			TransactionIdRetreat(snapshotConflictHorizon);
+		}
 
-			vacrel->frozen_pages++;
+		/* Using same cutoff when setting VM is now unnecessary */
+		if (presult.all_visible_except_removable && presult.all_frozen)
+			presult.frz_conflict_horizon = InvalidTransactionId;
 
-			/*
-			 * 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)
-			{
-				/* Using same cutoff when setting VM is now unnecessary */
-				snapshotConflictHorizon = presult.frz_conflict_horizon;
-				presult.frz_conflict_horizon = InvalidTransactionId;
-			}
-			else
-			{
-				/* Avoids false conflicts when hot_standby_feedback in use */
-				snapshotConflictHorizon = vacrel->cutoffs.OldestXmin;
-				TransactionIdRetreat(snapshotConflictHorizon);
-			}
+		/* Execute all freeze plans for page as a single atomic action */
+		heap_freeze_execute_prepared(vacrel->rel, buf,
+									 snapshotConflictHorizon,
+									 presult.frozen, presult.nfrozen);
+	}
+	else if (presult.all_frozen && presult.nfrozen == 0)
+	{
+		/* Page should be all visible except to-be-removed tuples */
+		Assert(presult.all_visible_except_removable);
 
-			/* Execute all freeze plans for page as a single atomic action */
-			heap_freeze_execute_prepared(vacrel->rel, buf,
-										 snapshotConflictHorizon,
-										 presult.frozen, presult.nfrozen);
-		}
+		/*
+		 * We have no freeze plans to execute, so there's no added cost from
+		 * following the freeze path.  That's why it was chosen. This is
+		 * important in the case where the page only contains totally frozen
+		 * tuples at this point (perhaps only following pruning). Such pages
+		 * can be marked all-frozen in the VM by our caller, even though none
+		 * of its tuples were newly frozen here (note that the "no freeze"
+		 * path never sets pages all-frozen).
+		 *
+		 * We never increment the frozen_pages instrumentation counter here,
+		 * since it only counts pages with newly frozen tuples (don't confuse
+		 * that with pages newly set all-frozen in VM).
+		 */
+		vacrel->NewRelfrozenXid = pagefrz.FreezePageRelfrozenXid;
+		vacrel->NewRelminMxid = pagefrz.FreezePageRelminMxid;
 	}
 	else
 	{
-- 
2.40.1


--tez7m2a73jtztiij
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
	filename="v4-0009-Execute-freezing-in-heap_page_prune.patch"



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

* [PATCH v4 08/19] lazy_scan_prune reorder freeze execution logic
@ 2024-03-19 23:30  Melanie Plageman <[email protected]>
  0 siblings, 0 replies; 26+ messages in thread

From: Melanie Plageman @ 2024-03-19 23:30 UTC (permalink / raw)

To combine the prune and freeze records, freezing must be done before a
pruning WAL record is emitted. We will move the freeze execution into
heap_page_prune() in future commits. lazy_scan_prune() currently
executes freezing, updates vacrel->NewRelfrozenXid and
vacrel->NewRelminMxid, and resets the snapshotConflictHorizon that the
visibility map update record may use in the same block of if statements.

This commit starts reordering that logic so that the freeze execution
can be separated from the other updates which should not be done in
pruning.
---
 src/backend/access/heap/vacuumlazy.c | 92 +++++++++++++++-------------
 1 file changed, 49 insertions(+), 43 deletions(-)

diff --git a/src/backend/access/heap/vacuumlazy.c b/src/backend/access/heap/vacuumlazy.c
index 4187c998d25..74ebab25a95 100644
--- a/src/backend/access/heap/vacuumlazy.c
+++ b/src/backend/access/heap/vacuumlazy.c
@@ -1421,6 +1421,7 @@ lazy_scan_prune(LVRelState *vacrel,
 				recently_dead_tuples;
 	HeapPageFreeze pagefrz;
 	bool		hastup = false;
+	bool		do_freeze;
 	int64		fpi_before = pgWalUsage.wal_fpi;
 	OffsetNumber deadoffsets[MaxHeapTuplesPerPage];
 
@@ -1580,10 +1581,15 @@ lazy_scan_prune(LVRelState *vacrel,
 	 * freeze when pruning generated an FPI, if doing so means that we set the
 	 * page all-frozen afterwards (might not happen until final heap pass).
 	 */
-	if (pagefrz.freeze_required || presult.nfrozen == 0 ||
+	do_freeze = pagefrz.freeze_required ||
 		(presult.all_visible_except_removable && presult.all_frozen &&
-		 fpi_before != pgWalUsage.wal_fpi))
+		 presult.nfrozen > 0 &&
+		 fpi_before != pgWalUsage.wal_fpi);
+
+	if (do_freeze)
 	{
+		TransactionId snapshotConflictHorizon;
+
 		/*
 		 * We're freezing the page.  Our final NewRelfrozenXid doesn't need to
 		 * be affected by the XIDs that are just about to be frozen anyway.
@@ -1591,52 +1597,52 @@ lazy_scan_prune(LVRelState *vacrel,
 		vacrel->NewRelfrozenXid = pagefrz.FreezePageRelfrozenXid;
 		vacrel->NewRelminMxid = pagefrz.FreezePageRelminMxid;
 
-		if (presult.nfrozen == 0)
-		{
-			/*
-			 * We have no freeze plans to execute, so there's no added cost
-			 * from following the freeze path.  That's why it was chosen. This
-			 * is important in the case where the page only contains totally
-			 * frozen tuples at this point (perhaps only following pruning).
-			 * Such pages can be marked all-frozen in the VM by our caller,
-			 * even though none of its tuples were newly frozen here (note
-			 * that the "no freeze" path never sets pages all-frozen).
-			 *
-			 * We never increment the frozen_pages instrumentation counter
-			 * here, since it only counts pages with newly frozen tuples
-			 * (don't confuse that with pages newly set all-frozen in VM).
-			 */
-		}
+		vacrel->frozen_pages++;
+
+		/*
+		 * 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)
+			snapshotConflictHorizon = presult.frz_conflict_horizon;
 		else
 		{
-			TransactionId snapshotConflictHorizon;
+			/* Avoids false conflicts when hot_standby_feedback in use */
+			snapshotConflictHorizon = pagefrz.cutoffs->OldestXmin;
+			TransactionIdRetreat(snapshotConflictHorizon);
+		}
 
-			vacrel->frozen_pages++;
+		/* Using same cutoff when setting VM is now unnecessary */
+		if (presult.all_visible_except_removable && presult.all_frozen)
+			presult.frz_conflict_horizon = InvalidTransactionId;
 
-			/*
-			 * 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)
-			{
-				/* Using same cutoff when setting VM is now unnecessary */
-				snapshotConflictHorizon = presult.frz_conflict_horizon;
-				presult.frz_conflict_horizon = InvalidTransactionId;
-			}
-			else
-			{
-				/* Avoids false conflicts when hot_standby_feedback in use */
-				snapshotConflictHorizon = vacrel->cutoffs.OldestXmin;
-				TransactionIdRetreat(snapshotConflictHorizon);
-			}
+		/* Execute all freeze plans for page as a single atomic action */
+		heap_freeze_execute_prepared(vacrel->rel, buf,
+									 snapshotConflictHorizon,
+									 presult.frozen, presult.nfrozen);
+	}
+	else if (presult.all_frozen && presult.nfrozen == 0)
+	{
+		/* Page should be all visible except to-be-removed tuples */
+		Assert(presult.all_visible_except_removable);
 
-			/* Execute all freeze plans for page as a single atomic action */
-			heap_freeze_execute_prepared(vacrel->rel, buf,
-										 snapshotConflictHorizon,
-										 presult.frozen, presult.nfrozen);
-		}
+		/*
+		 * We have no freeze plans to execute, so there's no added cost from
+		 * following the freeze path.  That's why it was chosen. This is
+		 * important in the case where the page only contains totally frozen
+		 * tuples at this point (perhaps only following pruning). Such pages
+		 * can be marked all-frozen in the VM by our caller, even though none
+		 * of its tuples were newly frozen here (note that the "no freeze"
+		 * path never sets pages all-frozen).
+		 *
+		 * We never increment the frozen_pages instrumentation counter here,
+		 * since it only counts pages with newly frozen tuples (don't confuse
+		 * that with pages newly set all-frozen in VM).
+		 */
+		vacrel->NewRelfrozenXid = pagefrz.FreezePageRelfrozenXid;
+		vacrel->NewRelminMxid = pagefrz.FreezePageRelminMxid;
 	}
 	else
 	{
-- 
2.40.1


--tez7m2a73jtztiij
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
	filename="v4-0009-Execute-freezing-in-heap_page_prune.patch"



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

* [PATCH v9 07/21] lazy_scan_prune reorder freeze execution logic
@ 2024-03-25 23:39  Melanie Plageman <[email protected]>
  0 siblings, 0 replies; 26+ messages in thread

From: Melanie Plageman @ 2024-03-25 23:39 UTC (permalink / raw)

To combine the prune and freeze records, freezing must be done before a
pruning WAL record is emitted. We will move the freeze execution into
heap_page_prune() in future commits. lazy_scan_prune() currently
executes freezing, updates vacrel->NewRelfrozenXid and
vacrel->NewRelminMxid, and resets the snapshotConflictHorizon that the
visibility map update record may use in the same block of if statements.

This commit starts reordering that logic so that the freeze execution
can be separated from the other updates which should not be done in
pruning.
---
 src/backend/access/heap/vacuumlazy.c | 93 +++++++++++++++-------------
 1 file changed, 50 insertions(+), 43 deletions(-)

diff --git a/src/backend/access/heap/vacuumlazy.c b/src/backend/access/heap/vacuumlazy.c
index 2a3cc5c7cd3..f474e661428 100644
--- a/src/backend/access/heap/vacuumlazy.c
+++ b/src/backend/access/heap/vacuumlazy.c
@@ -1421,6 +1421,7 @@ lazy_scan_prune(LVRelState *vacrel,
 				recently_dead_tuples;
 	HeapPageFreeze pagefrz;
 	bool		hastup = false;
+	bool		do_freeze;
 	int64		fpi_before = pgWalUsage.wal_fpi;
 	OffsetNumber deadoffsets[MaxHeapTuplesPerPage];
 
@@ -1576,10 +1577,15 @@ lazy_scan_prune(LVRelState *vacrel,
 	 * freeze when pruning generated an FPI, if doing so means that we set the
 	 * page all-frozen afterwards (might not happen until final heap pass).
 	 */
-	if (pagefrz.freeze_required || presult.nfrozen == 0 ||
+	do_freeze = pagefrz.freeze_required ||
 		(presult.all_visible_except_removable && presult.all_frozen &&
-		 fpi_before != pgWalUsage.wal_fpi))
+		 presult.nfrozen > 0 &&
+		 fpi_before != pgWalUsage.wal_fpi);
+
+	if (do_freeze)
 	{
+		TransactionId snapshotConflictHorizon;
+
 		/*
 		 * We're freezing the page.  Our final NewRelfrozenXid doesn't need to
 		 * be affected by the XIDs that are just about to be frozen anyway.
@@ -1587,52 +1593,53 @@ lazy_scan_prune(LVRelState *vacrel,
 		vacrel->NewRelfrozenXid = pagefrz.FreezePageRelfrozenXid;
 		vacrel->NewRelminMxid = pagefrz.FreezePageRelminMxid;
 
-		if (presult.nfrozen == 0)
-		{
-			/*
-			 * We have no freeze plans to execute, so there's no added cost
-			 * from following the freeze path.  That's why it was chosen. This
-			 * is important in the case where the page only contains totally
-			 * frozen tuples at this point (perhaps only following pruning).
-			 * Such pages can be marked all-frozen in the VM by our caller,
-			 * even though none of its tuples were newly frozen here (note
-			 * that the "no freeze" path never sets pages all-frozen).
-			 *
-			 * We never increment the frozen_pages instrumentation counter
-			 * here, since it only counts pages with newly frozen tuples
-			 * (don't confuse that with pages newly set all-frozen in VM).
-			 */
-		}
+		vacrel->frozen_pages++;
+
+		/*
+		 * 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)
+			snapshotConflictHorizon = presult.visibility_cutoff_xid;
 		else
 		{
-			TransactionId snapshotConflictHorizon;
+			/* Avoids false conflicts when hot_standby_feedback in use */
+			snapshotConflictHorizon = pagefrz.cutoffs->OldestXmin;
+			TransactionIdRetreat(snapshotConflictHorizon);
+		}
 
-			vacrel->frozen_pages++;
+		/* Using same cutoff when setting VM is now unnecessary */
+		if (presult.all_visible_except_removable && presult.all_frozen)
+			presult.visibility_cutoff_xid = InvalidTransactionId;
 
-			/*
-			 * We can use visibility_cutoff_xid 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)
-			{
-				/* Using same cutoff when setting VM is now unnecessary */
-				snapshotConflictHorizon = presult.visibility_cutoff_xid;
-				presult.visibility_cutoff_xid = InvalidTransactionId;
-			}
-			else
-			{
-				/* Avoids false conflicts when hot_standby_feedback in use */
-				snapshotConflictHorizon = vacrel->cutoffs.OldestXmin;
-				TransactionIdRetreat(snapshotConflictHorizon);
-			}
+		/* Execute all freeze plans for page as a single atomic action */
+		heap_freeze_execute_prepared(vacrel->rel, buf,
+									 snapshotConflictHorizon,
+									 presult.frozen, presult.nfrozen);
 
-			/* Execute all freeze plans for page as a single atomic action */
-			heap_freeze_execute_prepared(vacrel->rel, buf,
-										 snapshotConflictHorizon,
-										 presult.frozen, presult.nfrozen);
-		}
+	}
+	else if (presult.all_frozen && presult.nfrozen == 0)
+	{
+		/* Page should be all visible except to-be-removed tuples */
+		Assert(presult.all_visible_except_removable);
+
+		/*
+		 * We have no freeze plans to execute, so there's no added cost from
+		 * following the freeze path.  That's why it was chosen. This is
+		 * important in the case where the page only contains totally frozen
+		 * tuples at this point (perhaps only following pruning). Such pages
+		 * can be marked all-frozen in the VM by our caller, even though none
+		 * of its tuples were newly frozen here (note that the "no freeze"
+		 * path never sets pages all-frozen).
+		 *
+		 * We never increment the frozen_pages instrumentation counter here,
+		 * since it only counts pages with newly frozen tuples (don't confuse
+		 * that with pages newly set all-frozen in VM).
+		 */
+		vacrel->NewRelfrozenXid = pagefrz.FreezePageRelfrozenXid;
+		vacrel->NewRelminMxid = pagefrz.FreezePageRelminMxid;
 	}
 	else
 	{
-- 
2.40.1


--caj67xgx3lukmr5f
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
	filename="v9-0008-Execute-freezing-in-heap_page_prune.patch"



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

* [PATCH v7 07/16] lazy_scan_prune reorder freeze execution logic
@ 2024-03-25 23:39  Melanie Plageman <[email protected]>
  0 siblings, 0 replies; 26+ messages in thread

From: Melanie Plageman @ 2024-03-25 23:39 UTC (permalink / raw)

To combine the prune and freeze records, freezing must be done before a
pruning WAL record is emitted. We will move the freeze execution into
heap_page_prune() in future commits. lazy_scan_prune() currently
executes freezing, updates vacrel->NewRelfrozenXid and
vacrel->NewRelminMxid, and resets the snapshotConflictHorizon that the
visibility map update record may use in the same block of if statements.

This commit starts reordering that logic so that the freeze execution
can be separated from the other updates which should not be done in
pruning.
---
 src/backend/access/heap/vacuumlazy.c | 93 +++++++++++++++-------------
 1 file changed, 50 insertions(+), 43 deletions(-)

diff --git a/src/backend/access/heap/vacuumlazy.c b/src/backend/access/heap/vacuumlazy.c
index 2a3cc5c7cd3..f474e661428 100644
--- a/src/backend/access/heap/vacuumlazy.c
+++ b/src/backend/access/heap/vacuumlazy.c
@@ -1421,6 +1421,7 @@ lazy_scan_prune(LVRelState *vacrel,
 				recently_dead_tuples;
 	HeapPageFreeze pagefrz;
 	bool		hastup = false;
+	bool		do_freeze;
 	int64		fpi_before = pgWalUsage.wal_fpi;
 	OffsetNumber deadoffsets[MaxHeapTuplesPerPage];
 
@@ -1576,10 +1577,15 @@ lazy_scan_prune(LVRelState *vacrel,
 	 * freeze when pruning generated an FPI, if doing so means that we set the
 	 * page all-frozen afterwards (might not happen until final heap pass).
 	 */
-	if (pagefrz.freeze_required || presult.nfrozen == 0 ||
+	do_freeze = pagefrz.freeze_required ||
 		(presult.all_visible_except_removable && presult.all_frozen &&
-		 fpi_before != pgWalUsage.wal_fpi))
+		 presult.nfrozen > 0 &&
+		 fpi_before != pgWalUsage.wal_fpi);
+
+	if (do_freeze)
 	{
+		TransactionId snapshotConflictHorizon;
+
 		/*
 		 * We're freezing the page.  Our final NewRelfrozenXid doesn't need to
 		 * be affected by the XIDs that are just about to be frozen anyway.
@@ -1587,52 +1593,53 @@ lazy_scan_prune(LVRelState *vacrel,
 		vacrel->NewRelfrozenXid = pagefrz.FreezePageRelfrozenXid;
 		vacrel->NewRelminMxid = pagefrz.FreezePageRelminMxid;
 
-		if (presult.nfrozen == 0)
-		{
-			/*
-			 * We have no freeze plans to execute, so there's no added cost
-			 * from following the freeze path.  That's why it was chosen. This
-			 * is important in the case where the page only contains totally
-			 * frozen tuples at this point (perhaps only following pruning).
-			 * Such pages can be marked all-frozen in the VM by our caller,
-			 * even though none of its tuples were newly frozen here (note
-			 * that the "no freeze" path never sets pages all-frozen).
-			 *
-			 * We never increment the frozen_pages instrumentation counter
-			 * here, since it only counts pages with newly frozen tuples
-			 * (don't confuse that with pages newly set all-frozen in VM).
-			 */
-		}
+		vacrel->frozen_pages++;
+
+		/*
+		 * 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)
+			snapshotConflictHorizon = presult.visibility_cutoff_xid;
 		else
 		{
-			TransactionId snapshotConflictHorizon;
+			/* Avoids false conflicts when hot_standby_feedback in use */
+			snapshotConflictHorizon = pagefrz.cutoffs->OldestXmin;
+			TransactionIdRetreat(snapshotConflictHorizon);
+		}
 
-			vacrel->frozen_pages++;
+		/* Using same cutoff when setting VM is now unnecessary */
+		if (presult.all_visible_except_removable && presult.all_frozen)
+			presult.visibility_cutoff_xid = InvalidTransactionId;
 
-			/*
-			 * We can use visibility_cutoff_xid 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)
-			{
-				/* Using same cutoff when setting VM is now unnecessary */
-				snapshotConflictHorizon = presult.visibility_cutoff_xid;
-				presult.visibility_cutoff_xid = InvalidTransactionId;
-			}
-			else
-			{
-				/* Avoids false conflicts when hot_standby_feedback in use */
-				snapshotConflictHorizon = vacrel->cutoffs.OldestXmin;
-				TransactionIdRetreat(snapshotConflictHorizon);
-			}
+		/* Execute all freeze plans for page as a single atomic action */
+		heap_freeze_execute_prepared(vacrel->rel, buf,
+									 snapshotConflictHorizon,
+									 presult.frozen, presult.nfrozen);
 
-			/* Execute all freeze plans for page as a single atomic action */
-			heap_freeze_execute_prepared(vacrel->rel, buf,
-										 snapshotConflictHorizon,
-										 presult.frozen, presult.nfrozen);
-		}
+	}
+	else if (presult.all_frozen && presult.nfrozen == 0)
+	{
+		/* Page should be all visible except to-be-removed tuples */
+		Assert(presult.all_visible_except_removable);
+
+		/*
+		 * We have no freeze plans to execute, so there's no added cost from
+		 * following the freeze path.  That's why it was chosen. This is
+		 * important in the case where the page only contains totally frozen
+		 * tuples at this point (perhaps only following pruning). Such pages
+		 * can be marked all-frozen in the VM by our caller, even though none
+		 * of its tuples were newly frozen here (note that the "no freeze"
+		 * path never sets pages all-frozen).
+		 *
+		 * We never increment the frozen_pages instrumentation counter here,
+		 * since it only counts pages with newly frozen tuples (don't confuse
+		 * that with pages newly set all-frozen in VM).
+		 */
+		vacrel->NewRelfrozenXid = pagefrz.FreezePageRelfrozenXid;
+		vacrel->NewRelminMxid = pagefrz.FreezePageRelminMxid;
 	}
 	else
 	{
-- 
2.40.1


--ck6erxojvlx23byk
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
	filename="v7-0008-Execute-freezing-in-heap_page_prune.patch"



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

* [PATCH v9 07/21] lazy_scan_prune reorder freeze execution logic
@ 2024-03-25 23:39  Melanie Plageman <[email protected]>
  0 siblings, 0 replies; 26+ messages in thread

From: Melanie Plageman @ 2024-03-25 23:39 UTC (permalink / raw)

To combine the prune and freeze records, freezing must be done before a
pruning WAL record is emitted. We will move the freeze execution into
heap_page_prune() in future commits. lazy_scan_prune() currently
executes freezing, updates vacrel->NewRelfrozenXid and
vacrel->NewRelminMxid, and resets the snapshotConflictHorizon that the
visibility map update record may use in the same block of if statements.

This commit starts reordering that logic so that the freeze execution
can be separated from the other updates which should not be done in
pruning.
---
 src/backend/access/heap/vacuumlazy.c | 93 +++++++++++++++-------------
 1 file changed, 50 insertions(+), 43 deletions(-)

diff --git a/src/backend/access/heap/vacuumlazy.c b/src/backend/access/heap/vacuumlazy.c
index 2a3cc5c7cd3..f474e661428 100644
--- a/src/backend/access/heap/vacuumlazy.c
+++ b/src/backend/access/heap/vacuumlazy.c
@@ -1421,6 +1421,7 @@ lazy_scan_prune(LVRelState *vacrel,
 				recently_dead_tuples;
 	HeapPageFreeze pagefrz;
 	bool		hastup = false;
+	bool		do_freeze;
 	int64		fpi_before = pgWalUsage.wal_fpi;
 	OffsetNumber deadoffsets[MaxHeapTuplesPerPage];
 
@@ -1576,10 +1577,15 @@ lazy_scan_prune(LVRelState *vacrel,
 	 * freeze when pruning generated an FPI, if doing so means that we set the
 	 * page all-frozen afterwards (might not happen until final heap pass).
 	 */
-	if (pagefrz.freeze_required || presult.nfrozen == 0 ||
+	do_freeze = pagefrz.freeze_required ||
 		(presult.all_visible_except_removable && presult.all_frozen &&
-		 fpi_before != pgWalUsage.wal_fpi))
+		 presult.nfrozen > 0 &&
+		 fpi_before != pgWalUsage.wal_fpi);
+
+	if (do_freeze)
 	{
+		TransactionId snapshotConflictHorizon;
+
 		/*
 		 * We're freezing the page.  Our final NewRelfrozenXid doesn't need to
 		 * be affected by the XIDs that are just about to be frozen anyway.
@@ -1587,52 +1593,53 @@ lazy_scan_prune(LVRelState *vacrel,
 		vacrel->NewRelfrozenXid = pagefrz.FreezePageRelfrozenXid;
 		vacrel->NewRelminMxid = pagefrz.FreezePageRelminMxid;
 
-		if (presult.nfrozen == 0)
-		{
-			/*
-			 * We have no freeze plans to execute, so there's no added cost
-			 * from following the freeze path.  That's why it was chosen. This
-			 * is important in the case where the page only contains totally
-			 * frozen tuples at this point (perhaps only following pruning).
-			 * Such pages can be marked all-frozen in the VM by our caller,
-			 * even though none of its tuples were newly frozen here (note
-			 * that the "no freeze" path never sets pages all-frozen).
-			 *
-			 * We never increment the frozen_pages instrumentation counter
-			 * here, since it only counts pages with newly frozen tuples
-			 * (don't confuse that with pages newly set all-frozen in VM).
-			 */
-		}
+		vacrel->frozen_pages++;
+
+		/*
+		 * 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)
+			snapshotConflictHorizon = presult.visibility_cutoff_xid;
 		else
 		{
-			TransactionId snapshotConflictHorizon;
+			/* Avoids false conflicts when hot_standby_feedback in use */
+			snapshotConflictHorizon = pagefrz.cutoffs->OldestXmin;
+			TransactionIdRetreat(snapshotConflictHorizon);
+		}
 
-			vacrel->frozen_pages++;
+		/* Using same cutoff when setting VM is now unnecessary */
+		if (presult.all_visible_except_removable && presult.all_frozen)
+			presult.visibility_cutoff_xid = InvalidTransactionId;
 
-			/*
-			 * We can use visibility_cutoff_xid 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)
-			{
-				/* Using same cutoff when setting VM is now unnecessary */
-				snapshotConflictHorizon = presult.visibility_cutoff_xid;
-				presult.visibility_cutoff_xid = InvalidTransactionId;
-			}
-			else
-			{
-				/* Avoids false conflicts when hot_standby_feedback in use */
-				snapshotConflictHorizon = vacrel->cutoffs.OldestXmin;
-				TransactionIdRetreat(snapshotConflictHorizon);
-			}
+		/* Execute all freeze plans for page as a single atomic action */
+		heap_freeze_execute_prepared(vacrel->rel, buf,
+									 snapshotConflictHorizon,
+									 presult.frozen, presult.nfrozen);
 
-			/* Execute all freeze plans for page as a single atomic action */
-			heap_freeze_execute_prepared(vacrel->rel, buf,
-										 snapshotConflictHorizon,
-										 presult.frozen, presult.nfrozen);
-		}
+	}
+	else if (presult.all_frozen && presult.nfrozen == 0)
+	{
+		/* Page should be all visible except to-be-removed tuples */
+		Assert(presult.all_visible_except_removable);
+
+		/*
+		 * We have no freeze plans to execute, so there's no added cost from
+		 * following the freeze path.  That's why it was chosen. This is
+		 * important in the case where the page only contains totally frozen
+		 * tuples at this point (perhaps only following pruning). Such pages
+		 * can be marked all-frozen in the VM by our caller, even though none
+		 * of its tuples were newly frozen here (note that the "no freeze"
+		 * path never sets pages all-frozen).
+		 *
+		 * We never increment the frozen_pages instrumentation counter here,
+		 * since it only counts pages with newly frozen tuples (don't confuse
+		 * that with pages newly set all-frozen in VM).
+		 */
+		vacrel->NewRelfrozenXid = pagefrz.FreezePageRelfrozenXid;
+		vacrel->NewRelminMxid = pagefrz.FreezePageRelminMxid;
 	}
 	else
 	{
-- 
2.40.1


--caj67xgx3lukmr5f
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
	filename="v9-0008-Execute-freezing-in-heap_page_prune.patch"



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

* [PATCH v7 07/16] lazy_scan_prune reorder freeze execution logic
@ 2024-03-25 23:39  Melanie Plageman <[email protected]>
  0 siblings, 0 replies; 26+ messages in thread

From: Melanie Plageman @ 2024-03-25 23:39 UTC (permalink / raw)

To combine the prune and freeze records, freezing must be done before a
pruning WAL record is emitted. We will move the freeze execution into
heap_page_prune() in future commits. lazy_scan_prune() currently
executes freezing, updates vacrel->NewRelfrozenXid and
vacrel->NewRelminMxid, and resets the snapshotConflictHorizon that the
visibility map update record may use in the same block of if statements.

This commit starts reordering that logic so that the freeze execution
can be separated from the other updates which should not be done in
pruning.
---
 src/backend/access/heap/vacuumlazy.c | 93 +++++++++++++++-------------
 1 file changed, 50 insertions(+), 43 deletions(-)

diff --git a/src/backend/access/heap/vacuumlazy.c b/src/backend/access/heap/vacuumlazy.c
index 2a3cc5c7cd3..f474e661428 100644
--- a/src/backend/access/heap/vacuumlazy.c
+++ b/src/backend/access/heap/vacuumlazy.c
@@ -1421,6 +1421,7 @@ lazy_scan_prune(LVRelState *vacrel,
 				recently_dead_tuples;
 	HeapPageFreeze pagefrz;
 	bool		hastup = false;
+	bool		do_freeze;
 	int64		fpi_before = pgWalUsage.wal_fpi;
 	OffsetNumber deadoffsets[MaxHeapTuplesPerPage];
 
@@ -1576,10 +1577,15 @@ lazy_scan_prune(LVRelState *vacrel,
 	 * freeze when pruning generated an FPI, if doing so means that we set the
 	 * page all-frozen afterwards (might not happen until final heap pass).
 	 */
-	if (pagefrz.freeze_required || presult.nfrozen == 0 ||
+	do_freeze = pagefrz.freeze_required ||
 		(presult.all_visible_except_removable && presult.all_frozen &&
-		 fpi_before != pgWalUsage.wal_fpi))
+		 presult.nfrozen > 0 &&
+		 fpi_before != pgWalUsage.wal_fpi);
+
+	if (do_freeze)
 	{
+		TransactionId snapshotConflictHorizon;
+
 		/*
 		 * We're freezing the page.  Our final NewRelfrozenXid doesn't need to
 		 * be affected by the XIDs that are just about to be frozen anyway.
@@ -1587,52 +1593,53 @@ lazy_scan_prune(LVRelState *vacrel,
 		vacrel->NewRelfrozenXid = pagefrz.FreezePageRelfrozenXid;
 		vacrel->NewRelminMxid = pagefrz.FreezePageRelminMxid;
 
-		if (presult.nfrozen == 0)
-		{
-			/*
-			 * We have no freeze plans to execute, so there's no added cost
-			 * from following the freeze path.  That's why it was chosen. This
-			 * is important in the case where the page only contains totally
-			 * frozen tuples at this point (perhaps only following pruning).
-			 * Such pages can be marked all-frozen in the VM by our caller,
-			 * even though none of its tuples were newly frozen here (note
-			 * that the "no freeze" path never sets pages all-frozen).
-			 *
-			 * We never increment the frozen_pages instrumentation counter
-			 * here, since it only counts pages with newly frozen tuples
-			 * (don't confuse that with pages newly set all-frozen in VM).
-			 */
-		}
+		vacrel->frozen_pages++;
+
+		/*
+		 * 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)
+			snapshotConflictHorizon = presult.visibility_cutoff_xid;
 		else
 		{
-			TransactionId snapshotConflictHorizon;
+			/* Avoids false conflicts when hot_standby_feedback in use */
+			snapshotConflictHorizon = pagefrz.cutoffs->OldestXmin;
+			TransactionIdRetreat(snapshotConflictHorizon);
+		}
 
-			vacrel->frozen_pages++;
+		/* Using same cutoff when setting VM is now unnecessary */
+		if (presult.all_visible_except_removable && presult.all_frozen)
+			presult.visibility_cutoff_xid = InvalidTransactionId;
 
-			/*
-			 * We can use visibility_cutoff_xid 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)
-			{
-				/* Using same cutoff when setting VM is now unnecessary */
-				snapshotConflictHorizon = presult.visibility_cutoff_xid;
-				presult.visibility_cutoff_xid = InvalidTransactionId;
-			}
-			else
-			{
-				/* Avoids false conflicts when hot_standby_feedback in use */
-				snapshotConflictHorizon = vacrel->cutoffs.OldestXmin;
-				TransactionIdRetreat(snapshotConflictHorizon);
-			}
+		/* Execute all freeze plans for page as a single atomic action */
+		heap_freeze_execute_prepared(vacrel->rel, buf,
+									 snapshotConflictHorizon,
+									 presult.frozen, presult.nfrozen);
 
-			/* Execute all freeze plans for page as a single atomic action */
-			heap_freeze_execute_prepared(vacrel->rel, buf,
-										 snapshotConflictHorizon,
-										 presult.frozen, presult.nfrozen);
-		}
+	}
+	else if (presult.all_frozen && presult.nfrozen == 0)
+	{
+		/* Page should be all visible except to-be-removed tuples */
+		Assert(presult.all_visible_except_removable);
+
+		/*
+		 * We have no freeze plans to execute, so there's no added cost from
+		 * following the freeze path.  That's why it was chosen. This is
+		 * important in the case where the page only contains totally frozen
+		 * tuples at this point (perhaps only following pruning). Such pages
+		 * can be marked all-frozen in the VM by our caller, even though none
+		 * of its tuples were newly frozen here (note that the "no freeze"
+		 * path never sets pages all-frozen).
+		 *
+		 * We never increment the frozen_pages instrumentation counter here,
+		 * since it only counts pages with newly frozen tuples (don't confuse
+		 * that with pages newly set all-frozen in VM).
+		 */
+		vacrel->NewRelfrozenXid = pagefrz.FreezePageRelfrozenXid;
+		vacrel->NewRelminMxid = pagefrz.FreezePageRelminMxid;
 	}
 	else
 	{
-- 
2.40.1


--ck6erxojvlx23byk
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
	filename="v7-0008-Execute-freezing-in-heap_page_prune.patch"



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

* Re: pg_upgrade check for invalid databases
@ 2024-09-30 21:29  Daniel Gustafsson <[email protected]>
  0 siblings, 2 replies; 26+ messages in thread

From: Daniel Gustafsson @ 2024-09-30 21:29 UTC (permalink / raw)
  To: Tom Lane <[email protected]>; +Cc: Nathan Bossart <[email protected]>; Thomas Krennwallner <[email protected]>; pgsql-hackers

> On 30 Sep 2024, at 16:55, Tom Lane <[email protected]> wrote:

> TBH I'm not finding anything very much wrong with the current
> behavior... this has to be a rare situation, do we need to add
> debatable behavior to make it easier?

One argument would be to make the checks consistent, pg_upgrade generally tries
to report all the offending entries to help the user when fixing the source
database.  Not sure if it's a strong enough argument for carrying code which
really shouldn't see much use though.

--
Daniel Gustafsson







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

* Re: pg_upgrade check for invalid databases
@ 2024-09-30 22:20  Tom Lane <[email protected]>
  parent: Daniel Gustafsson <[email protected]>
  1 sibling, 1 reply; 26+ messages in thread

From: Tom Lane @ 2024-09-30 22:20 UTC (permalink / raw)
  To: Daniel Gustafsson <[email protected]>; +Cc: Nathan Bossart <[email protected]>; Thomas Krennwallner <[email protected]>; pgsql-hackers

Daniel Gustafsson <[email protected]> writes:
>> On 30 Sep 2024, at 16:55, Tom Lane <[email protected]> wrote:
>> TBH I'm not finding anything very much wrong with the current
>> behavior... this has to be a rare situation, do we need to add
>> debatable behavior to make it easier?

> One argument would be to make the checks consistent, pg_upgrade generally tries
> to report all the offending entries to help the user when fixing the source
> database.  Not sure if it's a strong enough argument for carrying code which
> really shouldn't see much use though.

OK, but the consistency argument would be to just report and fail.
I don't think there's a precedent in other pg_upgrade checks for
trying to fix problems automatically.

			regards, tom lane






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

* Re: pg_upgrade check for invalid databases
@ 2024-10-01 00:35  Thomas Krennwallner <[email protected]>
  parent: Daniel Gustafsson <[email protected]>
  1 sibling, 1 reply; 26+ messages in thread

From: Thomas Krennwallner @ 2024-10-01 00:35 UTC (permalink / raw)
  To: Daniel Gustafsson <[email protected]>; Tom Lane <[email protected]>; +Cc: Nathan Bossart <[email protected]>; pgsql-hackers

On 30/09/2024 17.29, Daniel Gustafsson wrote:
>> On 30 Sep 2024, at 16:55, Tom Lane <[email protected]> wrote:
> 
>> TBH I'm not finding anything very much wrong with the current
>> behavior... this has to be a rare situation, do we need to add
>> debatable behavior to make it easier?
> 
> One argument would be to make the checks consistent, pg_upgrade generally tries
> to report all the offending entries to help the user when fixing the source
> database.  Not sure if it's a strong enough argument for carrying code which
> really shouldn't see much use though.
In general, I agree that this situation should be rare for deliberate 
DROP DATABASE interrupted in interactive sessions.

Unfortunately, for (popular) tools that perform automatic "temporary 
database" cleanup, we could recently see an increase in invalid databases.

The additional check for pg_upgrade was made necessary due to several 
unrelated customers having invalid databases that stem from left-over 
Prisma Migrate "shadow databases" [1]. We could not reproduce this 
Prisma Migrate issue yet, as those migrations happened some time ago. 
Maybe this bug really stems from a much older Prisma Migrate version and 
we only see the fallout now. This is still a TODO item.

But it appears that this tool can get interrupted "at the wrong time" 
while it is deleting temporary databases (probably a manual Ctrl-C), and 
clients are unaware that this can then leave behind invalid databases.

Those temporary databases do not cause any harm as they are not used 
anymore. But eventually, PG installations will be upgraded to the next 
major version, and it is only then when those invalid databases 
resurface after pg_upgrade fails to run the checks.

Long story short: interactive DROP DATABASE interrupts are rare (they do 
exist, but customers are usually aware). Automation tools on the other 
hand may run DROP DATABASE and when they get interrupted at the wrong 
time they will then produce several left-over invalid databases. 
pg_upgrade will then fail to run the checks.

[1] 
https://www.prisma.io/docs/orm/prisma-migrate/understanding-prisma-migrate/shadow-database








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

* Re: pg_upgrade check for invalid databases
@ 2024-10-01 07:28  Daniel Gustafsson <[email protected]>
  parent: Tom Lane <[email protected]>
  0 siblings, 1 reply; 26+ messages in thread

From: Daniel Gustafsson @ 2024-10-01 07:28 UTC (permalink / raw)
  To: Tom Lane <[email protected]>; +Cc: Nathan Bossart <[email protected]>; Thomas Krennwallner <[email protected]>; pgsql-hackers

> On 1 Oct 2024, at 00:20, Tom Lane <[email protected]> wrote:
> 
> Daniel Gustafsson <[email protected]> writes:
>>> On 30 Sep 2024, at 16:55, Tom Lane <[email protected]> wrote:
>>> TBH I'm not finding anything very much wrong with the current
>>> behavior... this has to be a rare situation, do we need to add
>>> debatable behavior to make it easier?
> 
>> One argument would be to make the checks consistent, pg_upgrade generally tries
>> to report all the offending entries to help the user when fixing the source
>> database.  Not sure if it's a strong enough argument for carrying code which
>> really shouldn't see much use though.
> 
> OK, but the consistency argument would be to just report and fail.
> I don't think there's a precedent in other pg_upgrade checks for
> trying to fix problems automatically.

Correct, sorry for being unclear.  The consistency argument would be to expand
pg_upgrade to report all invalid databases rather than just the first found;
attempting to fix problems would be a new behavior.

--
Daniel Gustafsson







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

* Re: pg_upgrade check for invalid databases
@ 2024-10-01 07:42  Daniel Gustafsson <[email protected]>
  parent: Thomas Krennwallner <[email protected]>
  0 siblings, 0 replies; 26+ messages in thread

From: Daniel Gustafsson @ 2024-10-01 07:42 UTC (permalink / raw)
  To: Thomas Krennwallner <[email protected]>; +Cc: Tom Lane <[email protected]>; Nathan Bossart <[email protected]>; pgsql-hackers

> On 1 Oct 2024, at 02:35, Thomas Krennwallner <[email protected]> wrote:
> 
> On 30/09/2024 17.29, Daniel Gustafsson wrote:
>>> On 30 Sep 2024, at 16:55, Tom Lane <[email protected]> wrote:
>>> TBH I'm not finding anything very much wrong with the current
>>> behavior... this has to be a rare situation, do we need to add
>>> debatable behavior to make it easier?
>> One argument would be to make the checks consistent, pg_upgrade generally tries
>> to report all the offending entries to help the user when fixing the source
>> database.  Not sure if it's a strong enough argument for carrying code which
>> really shouldn't see much use though.
> In general, I agree that this situation should be rare for deliberate DROP DATABASE interrupted in interactive sessions.
> 
> Unfortunately, for (popular) tools that perform automatic "temporary database" cleanup, we could recently see an increase in invalid databases.
> 
> The additional check for pg_upgrade was made necessary due to several unrelated customers having invalid databases that stem from left-over Prisma Migrate "shadow databases" [1]. We could not reproduce this Prisma Migrate issue yet, as those migrations happened some time ago. Maybe this bug really stems from a much older Prisma Migrate version and we only see the fallout now. This is still a TODO item.
> 
> But it appears that this tool can get interrupted "at the wrong time" while it is deleting temporary databases (probably a manual Ctrl-C), and clients are unaware that this can then leave behind invalid databases.
> 
> Those temporary databases do not cause any harm as they are not used anymore. But eventually, PG installations will be upgraded to the next major version, and it is only then when those invalid databases resurface after pg_upgrade fails to run the checks.

Databases containing transient data no longer needed left by buggy tools is one
thing, but pg_upgrade won't be able to differentiate between those and invalid
databases of legitimate interest.  Allowing pg_upgrade to skip invalid
databases expose the risk of (potentially) valuable data being dropped during
the upgrade due to the user not having realized a rarely-used production
database was invalid.

> Long story short: interactive DROP DATABASE interrupts are rare (they do exist, but customers are usually aware). Automation tools on the other hand may run DROP DATABASE and when they get interrupted at the wrong time they will then produce several left-over invalid databases. pg_upgrade will then fail to run the checks.

Checking and reporting all invalid databases during the check phase seems like
a user-friendly option here, I can agree that the current behaviour isn't great
for users experiencing this issue.

--
Daniel Gustafsson







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

* Re: pg_upgrade check for invalid databases
@ 2024-10-07 19:37  Bruce Momjian <[email protected]>
  parent: Daniel Gustafsson <[email protected]>
  0 siblings, 1 reply; 26+ messages in thread

From: Bruce Momjian @ 2024-10-07 19:37 UTC (permalink / raw)
  To: Daniel Gustafsson <[email protected]>; +Cc: Tom Lane <[email protected]>; Nathan Bossart <[email protected]>; Thomas Krennwallner <[email protected]>; pgsql-hackers

On Tue, Oct  1, 2024 at 09:28:54AM +0200, Daniel Gustafsson wrote:
> > On 1 Oct 2024, at 00:20, Tom Lane <[email protected]> wrote:
> > 
> > Daniel Gustafsson <[email protected]> writes:
> >>> On 30 Sep 2024, at 16:55, Tom Lane <[email protected]> wrote:
> >>> TBH I'm not finding anything very much wrong with the current
> >>> behavior... this has to be a rare situation, do we need to add
> >>> debatable behavior to make it easier?
> > 
> >> One argument would be to make the checks consistent, pg_upgrade generally tries
> >> to report all the offending entries to help the user when fixing the source
> >> database.  Not sure if it's a strong enough argument for carrying code which
> >> really shouldn't see much use though.
> > 
> > OK, but the consistency argument would be to just report and fail.
> > I don't think there's a precedent in other pg_upgrade checks for
> > trying to fix problems automatically.
> 
> Correct, sorry for being unclear.  The consistency argument would be to expand
> pg_upgrade to report all invalid databases rather than just the first found;
> attempting to fix problems would be a new behavior.

Yes, historically pg_upgrade will fail if it finds anything unusual,
mostly because what it does normally is already scary enough.  If users
what pg_upgrade to do cleanups, it would be enabled by a separate flag,
or even a new command-line app.

-- 
  Bruce Momjian  <[email protected]>        https://momjian.us
  EDB                                      https://enterprisedb.com

  When a patient asks the doctor, "Am I going to die?", he means 
  "Am I going to die soon?"






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

* Re: pg_upgrade check for invalid databases
@ 2024-10-07 20:04  Nathan Bossart <[email protected]>
  parent: Bruce Momjian <[email protected]>
  0 siblings, 1 reply; 26+ messages in thread

From: Nathan Bossart @ 2024-10-07 20:04 UTC (permalink / raw)
  To: Bruce Momjian <[email protected]>; +Cc: Daniel Gustafsson <[email protected]>; Tom Lane <[email protected]>; Thomas Krennwallner <[email protected]>; pgsql-hackers

On Mon, Oct 07, 2024 at 03:37:35PM -0400, Bruce Momjian wrote:
> On Tue, Oct  1, 2024 at 09:28:54AM +0200, Daniel Gustafsson wrote:
>> Correct, sorry for being unclear.  The consistency argument would be to expand
>> pg_upgrade to report all invalid databases rather than just the first found;
>> attempting to fix problems would be a new behavior.
> 
> Yes, historically pg_upgrade will fail if it finds anything unusual,
> mostly because what it does normally is already scary enough.  If users
> what pg_upgrade to do cleanups, it would be enabled by a separate flag,
> or even a new command-line app.

While I suspect it's rare that someone CTRL-C's out of an accidental DROP
DATABASE and then runs pg_upgrade before trying to recover the data, I
agree with the principle of having pg_upgrade fail by default for things
like this.  If we did add a new flag, the new invalid database report that
Daniel mentions could say something like "try again with
--skip-invalid-databases to have pg_upgrade automatically drop invalid
databases."

-- 
nathan






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

* Re: pg_upgrade check for invalid databases
@ 2024-10-11 08:00  Daniel Gustafsson <[email protected]>
  parent: Nathan Bossart <[email protected]>
  0 siblings, 1 reply; 26+ messages in thread

From: Daniel Gustafsson @ 2024-10-11 08:00 UTC (permalink / raw)
  To: Nathan Bossart <[email protected]>; +Cc: Bruce Momjian <[email protected]>; Tom Lane <[email protected]>; Thomas Krennwallner <[email protected]>; pgsql-hackers

> On 7 Oct 2024, at 22:04, Nathan Bossart <[email protected]> wrote:
> 
> On Mon, Oct 07, 2024 at 03:37:35PM -0400, Bruce Momjian wrote:
>> On Tue, Oct  1, 2024 at 09:28:54AM +0200, Daniel Gustafsson wrote:
>>> Correct, sorry for being unclear.  The consistency argument would be to expand
>>> pg_upgrade to report all invalid databases rather than just the first found;
>>> attempting to fix problems would be a new behavior.
>> 
>> Yes, historically pg_upgrade will fail if it finds anything unusual,
>> mostly because what it does normally is already scary enough.  If users
>> what pg_upgrade to do cleanups, it would be enabled by a separate flag,
>> or even a new command-line app.
> 
> While I suspect it's rare that someone CTRL-C's out of an accidental DROP
> DATABASE and then runs pg_upgrade before trying to recover the data, I
> agree with the principle of having pg_upgrade fail by default for things
> like this.  If we did add a new flag, the new invalid database report that
> Daniel mentions could say something like "try again with
> --skip-invalid-databases to have pg_upgrade automatically drop invalid
> databases."

If we are teaching pg_upgrade to handle errors, either by skipping or by
fixing, then I believe this is the right way to go about it.  A successful run
should probably also create a report of the databases which were skipped.

--
Daniel Gustafsson







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

* Re: pg_upgrade check for invalid databases
@ 2024-10-13 12:28  Thomas Krennwallner <[email protected]>
  parent: Daniel Gustafsson <[email protected]>
  0 siblings, 1 reply; 26+ messages in thread

From: Thomas Krennwallner @ 2024-10-13 12:28 UTC (permalink / raw)
  To: Daniel Gustafsson <[email protected]>; +Cc: Nathan Bossart <[email protected]>; Bruce Momjian <[email protected]>; Tom Lane <[email protected]>; pgsql-hackers

On Fri, 11 Oct 2024 at 04:01, Daniel Gustafsson <[email protected]> wrote:
>
> > On 7 Oct 2024, at 22:04, Nathan Bossart <[email protected]> wrote:
> >
> > On Mon, Oct 07, 2024 at 03:37:35PM -0400, Bruce Momjian wrote:
> >> On Tue, Oct  1, 2024 at 09:28:54AM +0200, Daniel Gustafsson wrote:
> >>> Correct, sorry for being unclear.  The consistency argument would be to expand
> >>> pg_upgrade to report all invalid databases rather than just the first found;
> >>> attempting to fix problems would be a new behavior.
> >>
> >> Yes, historically pg_upgrade will fail if it finds anything unusual,
> >> mostly because what it does normally is already scary enough.  If users
> >> what pg_upgrade to do cleanups, it would be enabled by a separate flag,
> >> or even a new command-line app.
> >
> > While I suspect it's rare that someone CTRL-C's out of an accidental DROP
> > DATABASE and then runs pg_upgrade before trying to recover the data, I
> > agree with the principle of having pg_upgrade fail by default for things
> > like this.  If we did add a new flag, the new invalid database report that
> > Daniel mentions could say something like "try again with
> > --skip-invalid-databases to have pg_upgrade automatically drop invalid
> > databases."
>
> If we are teaching pg_upgrade to handle errors, either by skipping or by
> fixing, then I believe this is the right way to go about it.  A successful run
> should probably also create a report of the databases which were skipped.

In v2 I've made changes to the patch incorporating the suggestions here:

* Default behaviour is to just fail with a report of all invalid databases

* A new option --skip-invalid-databases will then skip the checks, and
would not transfer any invalid database to the new cluster. A warning
with a report file will then follow after a successful run.

Dropping invalid databases in the old cluster will make invalid
databases unrecoverable, so I opted for a skip over invalid databases
approach that would leave invalid databases in the old cluster.

Apart from a missing --skip-invalid-databases test, does this attempt look OK?


Attachments:

  [text/x-patch] pg18-v2-0001-pg_upgrade-Add-check-for-invalid-databases.patch (17.7K, ../../CADrDHEQoGbXXF1v5fbq0sR3fgA6+ySmM8GtG+3JdyrY4yM2bng@mail.gmail.com/2-pg18-v2-0001-pg_upgrade-Add-check-for-invalid-databases.patch)
  download | inline diff:
From e51f581ddc1158a9fb2840dfc04863618a390887 Mon Sep 17 00:00:00 2001
From: Thomas Krennwallner <[email protected]>
Date: Fri, 20 Sep 2024 17:33:05 -0400
Subject: [PATCH] pg_upgrade: Add check for invalid databases.

Currently, pg_upgrade fails to connect to the first invalid database
it encounters in get_loadable_libraries() and then aborts.  While we
print a fatal message with a hint what should be done, the output is
terse and does not report further invalid databases present in an
installation.

Instead of just exiting on first error, we collect all
pg_database.datconnlimit values in get_db_infos() and order invalid
databases after valid ones in cluster.dbarr. This allows for skipping
over invalid databases in all checks with --skip-invalid-databases, a
new option where we do not transfer such databases from the old
cluster.

Unless we use --skip-invalid-databases, check_for_invalid_dbs()
collects all invalid databases in a report file invalid_databases.txt.
After a successful pg_upgrade run with skipped invalid databases,
create_invalid_databases_report() will collect them in a report file
skipped_invalid_databases.txt for the completion banner output.
---
 doc/src/sgml/ref/pgupgrade.sgml        |  19 ++++
 src/bin/pg_upgrade/check.c             | 131 +++++++++++++++++++++++--
 src/bin/pg_upgrade/info.c              |  20 +++-
 src/bin/pg_upgrade/option.c            |   7 ++
 src/bin/pg_upgrade/pg_upgrade.c        |  18 +++-
 src/bin/pg_upgrade/pg_upgrade.h        |   8 +-
 src/bin/pg_upgrade/t/002_pg_upgrade.pl |   2 +-
 7 files changed, 187 insertions(+), 18 deletions(-)

diff --git a/doc/src/sgml/ref/pgupgrade.sgml b/doc/src/sgml/ref/pgupgrade.sgml
index 4777381dac..a9a6b62e7a 100644
--- a/doc/src/sgml/ref/pgupgrade.sgml
+++ b/doc/src/sgml/ref/pgupgrade.sgml
@@ -253,6 +253,25 @@ PostgreSQL documentation
       </listitem>
      </varlistentry>
 
+     <varlistentry>
+      <term><option>--skip-invalid-databases</option></term>
+      <listitem>
+       <para>
+        If a cluster contains invalid databases, the default behaviour of
+        <application>pg_upgrade</application> is to fail with a report of all
+        such databases found in an installation. Invalid databases, which cannot
+        be connected to anymore, are a consequence of interrupted
+        <command>DROP DATABASE</command> and marked as corrupted databases in
+        <link linkend="catalog-pg-database"><structname>pg_database</structname></link>.<structfield>datconnlimit</structfield>.
+       </para>
+       <para>
+        When using option <option>--skip-invalid-databases</option>,
+        <application>pg_upgrade</application> instead will skip all invalid
+        databases and do not transfer them over to the new cluster.
+       </para>
+      </listitem>
+     </varlistentry>
+
      <varlistentry>
       <term><option>--sync-method=</option><replaceable>method</replaceable></term>
       <listitem>
diff --git a/src/bin/pg_upgrade/check.c b/src/bin/pg_upgrade/check.c
index 12735a4268..e372feff1a 100644
--- a/src/bin/pg_upgrade/check.c
+++ b/src/bin/pg_upgrade/check.c
@@ -12,6 +12,7 @@
 #include "catalog/pg_authid_d.h"
 #include "catalog/pg_class_d.h"
 #include "catalog/pg_collation.h"
+#include "catalog/pg_database.h"
 #include "fe_utils/string_utils.h"
 #include "mb/pg_wchar.h"
 #include "pg_upgrade.h"
@@ -19,6 +20,7 @@
 static void check_new_cluster_is_empty(void);
 static void check_is_install_user(ClusterInfo *cluster);
 static void check_proper_datallowconn(ClusterInfo *cluster);
+static void check_for_invalid_dbs(ClusterInfo *cluster);
 static void check_for_prepared_transactions(ClusterInfo *cluster);
 static void check_for_isn_and_int8_passing_mismatch(ClusterInfo *cluster);
 static void check_for_user_defined_postfix_ops(ClusterInfo *cluster);
@@ -598,6 +600,11 @@ check_and_dump_old_cluster(void)
 	 */
 	get_db_rel_and_slot_infos(&old_cluster);
 
+	/*
+	 * Verify that the old cluster does not contain invalid databases.
+	 */
+	check_for_invalid_dbs(&old_cluster);
+
 	init_tablespaces();
 
 	get_loadable_libraries();
@@ -691,6 +698,11 @@ check_new_cluster(void)
 {
 	get_db_rel_and_slot_infos(&new_cluster);
 
+	/*
+	 * Verify that the new cluster does not contain invalid databases.
+	 */
+	check_for_invalid_dbs(&new_cluster);
+
 	check_new_cluster_is_empty();
 
 	check_loadable_libraries();
@@ -763,7 +775,7 @@ issue_warnings_and_set_wal_level(void)
 
 
 void
-output_completion_banner(char *deletion_script_file_name)
+output_completion_banner(char *deletion_script_file_name, char *skipped_invalid_databases_file_name)
 {
 	PQExpBufferData user_specification;
 
@@ -775,6 +787,14 @@ output_completion_banner(char *deletion_script_file_name)
 		appendPQExpBufferChar(&user_specification, ' ');
 	}
 
+	if (skipped_invalid_databases_file_name)
+	{
+		pg_log(PG_REPORT,
+			   "The following invalid databases were not transferred by pg_upgrade:\n"
+			   "    %s",
+			   skipped_invalid_databases_file_name);
+	}
+
 	pg_log(PG_REPORT,
 		   "Optimizer statistics are not transferred by pg_upgrade.\n"
 		   "Once you start the new server, consider running:\n"
@@ -998,7 +1018,7 @@ create_script_for_old_cluster_deletion(char **deletion_script_file_name)
 
 			fprintf(script, "\n");
 
-			for (dbnum = 0; dbnum < old_cluster.dbarr.ndbs; dbnum++)
+			for (dbnum = 0; dbnum < old_cluster.dbarr.ntotaldbs; dbnum++)
 				fprintf(script, RMDIR_CMD " %c%s%c%u%c\n", PATH_QUOTE,
 						fix_path_separator(os_info.old_tablespaces[tblnum]),
 						PATH_SEPARATOR, old_cluster.dbarr.dbs[dbnum].db_oid,
@@ -1087,6 +1107,96 @@ check_is_install_user(ClusterInfo *cluster)
 	check_ok();
 }
 
+/*
+ *	check_for_invalid_dbs
+ *
+ *	Ensure that all databases are valid as connections to invalid databases
+ *	are not allowed.
+ */
+static void
+check_for_invalid_dbs(ClusterInfo *cluster)
+{
+	int			dbnum;
+	FILE	   *script = NULL;
+	char		output_path[MAXPGPATH];
+
+	prep_status("Checking for invalid databases");
+
+	if (user_opts.skip_invalid_dbs)
+	{
+		pg_log(PG_REPORT, "skipped");
+		return;
+	}
+
+	snprintf(output_path, sizeof(output_path), "%s/%s",
+			 log_opts.basedir,
+			 "invalid_databases.txt");
+
+	/*
+	 * If invalid databases exist in cluster they are ordered after the valid
+	 * ones in dbarr.
+	 */
+	for (dbnum = cluster->dbarr.ndbs; dbnum < cluster->dbarr.ntotaldbs; dbnum++)
+	{
+		if (script == NULL && (script = fopen_priv(output_path, "w")) == NULL)
+			pg_fatal("could not open file \"%s\": %m", output_path);
+		fprintf(script, "%s\n", cluster->dbarr.dbs[dbnum].db_name);
+	}
+
+	if (script)
+	{
+		fclose(script);
+		pg_log(PG_REPORT, "fatal");
+		pg_fatal("Your installation contains invalid databases as a consequence of\n"
+				 "interrupted DROP DATABASE.  They are now marked as corrupted databases\n"
+				 "that cannot be connected to anymore.  Consider removing them using\n"
+				 "    DROP DATABASE ...;\n"
+				 "or run pg_upgrade with --skip-invalid-databases to skip transferring\n"
+				 "invalid databases to the new cluster.\n"
+				 "A list of invalid databases is in the file:\n"
+				 "    %s", output_path);
+	}
+	else
+		check_ok();
+}
+
+
+/*
+ * create_invalid_databases_report()
+ *
+ *	This is particularly useful for listing skipped invalid databases.
+ */
+void
+create_invalid_databases_report(char **skipped_invalid_databases_file_name)
+{
+	int			dbnum;
+	FILE	   *script = NULL;
+
+	if (!user_opts.skip_invalid_dbs || old_cluster.dbarr.ndbs == old_cluster.dbarr.ntotaldbs)
+		return;
+
+	*skipped_invalid_databases_file_name = psprintf("%sskipped_invalid_databases.txt",
+													SCRIPT_PREFIX);
+
+	prep_status("Creating report for skipped invalid databases");
+
+	for (dbnum = old_cluster.dbarr.ndbs; dbnum < old_cluster.dbarr.ntotaldbs; dbnum++)
+	{
+		if (script == NULL && (script = fopen_priv(*skipped_invalid_databases_file_name, "w")) == NULL)
+			pg_fatal("could not open file \"%s\": %m", *skipped_invalid_databases_file_name);
+		fprintf(script, "%s\n", old_cluster.dbarr.dbs[dbnum].db_name);
+	}
+
+	if (script != NULL)
+	{
+		report_status(PG_WARNING, "warning");
+		pg_log(PG_WARNING,
+			   "\nWARNING:  old cluster contains invalid databases which have been skipped");
+		fclose(script);
+	}
+	else
+		check_ok();
+}
 
 /*
  *	check_proper_datallowconn
@@ -1115,10 +1225,15 @@ check_proper_datallowconn(ClusterInfo *cluster)
 
 	conn_template1 = connectToServer(cluster, "template1");
 
-	/* get database names */
+	/*
+	 * get database names; skip invalid databases as they are handled in
+	 * check_for_invalid_dbs()
+	 */
 	dbres = executeQueryOrDie(conn_template1,
 							  "SELECT	datname, datallowconn "
-							  "FROM	pg_catalog.pg_database");
+							  "FROM	pg_catalog.pg_database "
+							  "WHERE datconnlimit != %d",
+							  DATCONNLIMIT_INVALID_DB);
 
 	i_datname = PQfnumber(dbres, "datname");
 	i_datallowconn = PQfnumber(dbres, "datallowconn");
@@ -1982,7 +2097,9 @@ check_old_cluster_subscription_state(void)
 
 	/*
 	 * Check that all the subscriptions have their respective replication
-	 * origin.  This check only needs to run once.
+	 * origin.  This check only needs to run once.  Skip invalid databases,
+	 * this check will only be performed when user_opts.skip_invalid_dbs is
+	 * true.
 	 */
 	conn = connectToServer(&old_cluster, old_cluster.dbarr.dbs[0].db_name);
 	res = executeQueryOrDie(conn,
@@ -1992,7 +2109,9 @@ check_old_cluster_subscription_state(void)
 							"	ON o.roname = 'pg_' || s.oid "
 							"INNER JOIN pg_catalog.pg_database d "
 							"	ON d.oid = s.subdbid "
-							"WHERE o.roname IS NULL;");
+							"WHERE o.roname IS NULL "
+							"   AND d.datconnlimit != %d",
+							DATCONNLIMIT_INVALID_DB);
 	ntup = PQntuples(res);
 	for (int i = 0; i < ntup; i++)
 	{
diff --git a/src/bin/pg_upgrade/info.c b/src/bin/pg_upgrade/info.c
index f83ded89cb..514ed2d612 100644
--- a/src/bin/pg_upgrade/info.c
+++ b/src/bin/pg_upgrade/info.c
@@ -11,6 +11,7 @@
 
 #include "access/transam.h"
 #include "catalog/pg_class_d.h"
+#include "catalog/pg_database.h"
 #include "pg_upgrade.h"
 #include "pqexpbuffer.h"
 
@@ -407,14 +408,16 @@ get_db_infos(ClusterInfo *cluster)
 	PGresult   *res;
 	int			ntups;
 	int			tupnum;
+	int			valid_tups;
 	DbInfo	   *dbinfos;
 	int			i_datname,
+				i_datconnlimit,
 				i_oid,
 				i_spclocation;
 	char		query[QUERY_ALLOC];
 
 	snprintf(query, sizeof(query),
-			 "SELECT d.oid, d.datname, d.encoding, d.datcollate, d.datctype, ");
+			 "SELECT d.oid, d.datname, d.encoding, d.datcollate, d.datctype, d.datconnlimit, ");
 	if (GET_MAJOR_VERSION(cluster->major_version) >= 1700)
 		snprintf(query + strlen(query), sizeof(query) - strlen(query),
 				 "datlocprovider, datlocale, ");
@@ -430,30 +433,36 @@ get_db_infos(ClusterInfo *cluster)
 			 " LEFT OUTER JOIN pg_catalog.pg_tablespace t "
 			 " ON d.dattablespace = t.oid "
 			 "WHERE d.datallowconn = true "
-			 "ORDER BY 1");
+			 "ORDER BY "
+			 " CASE WHEN d.datconnlimit != %d THEN 0 ELSE 1 END, "
+			 " d.oid", DATCONNLIMIT_INVALID_DB);
 
 	res = executeQueryOrDie(conn, "%s", query);
 
 	i_oid = PQfnumber(res, "oid");
 	i_datname = PQfnumber(res, "datname");
+	i_datconnlimit = PQfnumber(res, "datconnlimit");
 	i_spclocation = PQfnumber(res, "spclocation");
 
 	ntups = PQntuples(res);
 	dbinfos = (DbInfo *) pg_malloc0(sizeof(DbInfo) * ntups);
 
-	for (tupnum = 0; tupnum < ntups; tupnum++)
+	for (tupnum = 0, valid_tups = 0; tupnum < ntups; tupnum++)
 	{
 		dbinfos[tupnum].db_oid = atooid(PQgetvalue(res, tupnum, i_oid));
 		dbinfos[tupnum].db_name = pg_strdup(PQgetvalue(res, tupnum, i_datname));
 		snprintf(dbinfos[tupnum].db_tablespace, sizeof(dbinfos[tupnum].db_tablespace), "%s",
 				 PQgetvalue(res, tupnum, i_spclocation));
+		if (atoi(PQgetvalue(res, tupnum, i_datconnlimit)) != DATCONNLIMIT_INVALID_DB)
+			valid_tups++;
 	}
 	PQclear(res);
 
 	PQfinish(conn);
 
 	cluster->dbarr.dbs = dbinfos;
-	cluster->dbarr.ndbs = ntups;
+	cluster->dbarr.ndbs = valid_tups;
+	cluster->dbarr.ntotaldbs = ntups;
 }
 
 
@@ -774,7 +783,7 @@ free_db_and_rel_infos(DbInfoArr *db_arr)
 {
 	int			dbnum;
 
-	for (dbnum = 0; dbnum < db_arr->ndbs; dbnum++)
+	for (dbnum = 0; dbnum < db_arr->ntotaldbs; dbnum++)
 	{
 		free_rel_infos(&db_arr->dbs[dbnum].rel_arr);
 		pg_free(db_arr->dbs[dbnum].db_name);
@@ -782,6 +791,7 @@ free_db_and_rel_infos(DbInfoArr *db_arr)
 	pg_free(db_arr->dbs);
 	db_arr->dbs = NULL;
 	db_arr->ndbs = 0;
+	db_arr->ntotaldbs = 0;
 }
 
 
diff --git a/src/bin/pg_upgrade/option.c b/src/bin/pg_upgrade/option.c
index 6f41d63eed..1937796a72 100644
--- a/src/bin/pg_upgrade/option.c
+++ b/src/bin/pg_upgrade/option.c
@@ -60,6 +60,7 @@ parseCommandLine(int argc, char *argv[])
 		{"copy", no_argument, NULL, 2},
 		{"copy-file-range", no_argument, NULL, 3},
 		{"sync-method", required_argument, NULL, 4},
+		{"skip-invalid-databases", no_argument, NULL, 5},
 
 		{NULL, 0, NULL, 0}
 	};
@@ -69,6 +70,7 @@ parseCommandLine(int argc, char *argv[])
 	DataDirSyncMethod unused;
 
 	user_opts.do_sync = true;
+	user_opts.skip_invalid_dbs = false;
 	user_opts.transfer_mode = TRANSFER_MODE_COPY;
 
 	os_info.progname = get_progname(argv[0]);
@@ -212,6 +214,10 @@ parseCommandLine(int argc, char *argv[])
 				user_opts.sync_method = pg_strdup(optarg);
 				break;
 
+			case 5:
+				user_opts.skip_invalid_dbs = true;
+				break;
+
 			default:
 				fprintf(stderr, _("Try \"%s --help\" for more information.\n"),
 						os_info.progname);
@@ -306,6 +312,7 @@ usage(void)
 	printf(_("  --clone                       clone instead of copying files to new cluster\n"));
 	printf(_("  --copy                        copy files to new cluster (default)\n"));
 	printf(_("  --copy-file-range             copy files to new cluster with copy_file_range\n"));
+	printf(_("  --skip-invalid-databases      skip transferring invalid databases to the new cluster\n"));
 	printf(_("  --sync-method=METHOD          set method for syncing files to disk\n"));
 	printf(_("  -?, --help                    show this help, then exit\n"));
 	printf(_("\n"
diff --git a/src/bin/pg_upgrade/pg_upgrade.c b/src/bin/pg_upgrade/pg_upgrade.c
index 663235816f..b0353327b9 100644
--- a/src/bin/pg_upgrade/pg_upgrade.c
+++ b/src/bin/pg_upgrade/pg_upgrade.c
@@ -41,6 +41,7 @@
 #include <time.h>
 
 #include "catalog/pg_class_d.h"
+#include "catalog/pg_database.h"
 #include "common/file_perm.h"
 #include "common/logging.h"
 #include "common/restricted_token.h"
@@ -83,7 +84,8 @@ char	   *output_files[] = {
 int
 main(int argc, char **argv)
 {
-	char	   *deletion_script_file_name = NULL;
+	char	   *deletion_script_file_name = NULL,
+			   *skipped_invalid_databases_file_name = NULL;
 
 	/*
 	 * pg_upgrade doesn't currently use common/logging.c, but initialize it
@@ -219,6 +221,8 @@ main(int argc, char **argv)
 
 	create_script_for_old_cluster_deletion(&deletion_script_file_name);
 
+	create_invalid_databases_report(&skipped_invalid_databases_file_name);
+
 	issue_warnings_and_set_wal_level();
 
 	pg_log(PG_REPORT,
@@ -226,9 +230,10 @@ main(int argc, char **argv)
 		   "Upgrade Complete\n"
 		   "----------------");
 
-	output_completion_banner(deletion_script_file_name);
+	output_completion_banner(deletion_script_file_name, skipped_invalid_databases_file_name);
 
 	pg_free(deletion_script_file_name);
+	pg_free(skipped_invalid_databases_file_name);
 
 	cleanup_output_dirs();
 
@@ -859,10 +864,15 @@ set_frozenxids(bool minmxid_only)
 							  "SET	datminmxid = '%u'",
 							  old_cluster.controldata.chkpnt_nxtmulti));
 
-	/* get database names */
+	/*
+	 * get database names; skip invalid databases as they are handled in
+	 * check_for_invalid_dbs()
+	 */
 	dbres = executeQueryOrDie(conn_template1,
 							  "SELECT	datname, datallowconn "
-							  "FROM	pg_catalog.pg_database");
+							  "FROM	pg_catalog.pg_database "
+							  "WHERE datconnlimit != %d",
+							  DATCONNLIMIT_INVALID_DB);
 
 	i_datname = PQfnumber(dbres, "datname");
 	i_datallowconn = PQfnumber(dbres, "datallowconn");
diff --git a/src/bin/pg_upgrade/pg_upgrade.h b/src/bin/pg_upgrade/pg_upgrade.h
index 53f693c2d4..8abb1688ca 100644
--- a/src/bin/pg_upgrade/pg_upgrade.h
+++ b/src/bin/pg_upgrade/pg_upgrade.h
@@ -213,7 +213,8 @@ typedef struct
 typedef struct
 {
 	DbInfo	   *dbs;			/* array of db infos */
-	int			ndbs;			/* number of db infos */
+	int			ndbs;			/* number of valid db infos */
+	int			ntotaldbs;		/* total number of db infos */
 } DbInfoArr;
 
 /*
@@ -323,6 +324,8 @@ typedef struct
 	bool		check;			/* check clusters only, don't change any data */
 	bool		live_check;		/* check clusters only, old server is running */
 	bool		do_sync;		/* flush changes to disk */
+	bool		skip_invalid_dbs;	/* skip transferring invalid databases to
+									 * the new cluster */
 	transferMode transfer_mode; /* copy files or link them? */
 	int			jobs;			/* number of processes/threads to use */
 	char	   *socketdir;		/* directory to use for Unix sockets */
@@ -371,10 +374,11 @@ void		check_and_dump_old_cluster(void);
 void		check_new_cluster(void);
 void		report_clusters_compatible(void);
 void		issue_warnings_and_set_wal_level(void);
-void		output_completion_banner(char *deletion_script_file_name);
+void		output_completion_banner(char *deletion_script_file_name, char *skipped_invalid_databases_file_name);
 void		check_cluster_versions(void);
 void		check_cluster_compatibility(void);
 void		create_script_for_old_cluster_deletion(char **deletion_script_file_name);
+void		create_invalid_databases_report(char **skipped_invalid_databases_file_name);
 
 
 /* controldata.c */
diff --git a/src/bin/pg_upgrade/t/002_pg_upgrade.pl b/src/bin/pg_upgrade/t/002_pg_upgrade.pl
index 17af2ce61e..c7da6d27b2 100644
--- a/src/bin/pg_upgrade/t/002_pg_upgrade.pl
+++ b/src/bin/pg_upgrade/t/002_pg_upgrade.pl
@@ -424,7 +424,7 @@ SKIP:
 			$mode, '--check',
 		],
 		1,
-		[qr/invalid/],    # pg_upgrade prints errors on stdout :(
+		[qr/invalid_databases\.txt/],    # pg_upgrade prints errors on stdout :(
 		[qr/^$/],
 		'invalid database causes failure');
 	rmtree($newnode->data_dir . "/pg_upgrade_output.d");
-- 
2.46.2



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

* Re: pg_upgrade check for invalid databases
@ 2024-10-14 16:57  Bruce Momjian <[email protected]>
  parent: Thomas Krennwallner <[email protected]>
  0 siblings, 1 reply; 26+ messages in thread

From: Bruce Momjian @ 2024-10-14 16:57 UTC (permalink / raw)
  To: Thomas Krennwallner <[email protected]>; +Cc: Daniel Gustafsson <[email protected]>; Nathan Bossart <[email protected]>; Tom Lane <[email protected]>; pgsql-hackers

On Sun, Oct 13, 2024 at 08:28:57AM -0400, Thomas Krennwallner wrote:
> In v2 I've made changes to the patch incorporating the suggestions here:
> 
> * Default behaviour is to just fail with a report of all invalid databases
> 
> * A new option --skip-invalid-databases will then skip the checks, and
> would not transfer any invalid database to the new cluster. A warning
> with a report file will then follow after a successful run.
> 
> Dropping invalid databases in the old cluster will make invalid
> databases unrecoverable, so I opted for a skip over invalid databases
> approach that would leave invalid databases in the old cluster.
> 
> Apart from a missing --skip-invalid-databases test, does this attempt look OK?

I don't think there is enough demand for the feature of skipping invalid
databases because we have gotten few reports of such problems, and also
because your case is related to an external tool causing this problem.

What might be acceptable would be to add an option that would make
pg_upgrade more tolerant of problems in many areas, that is a lot more
research and discussion.

-- 
  Bruce Momjian  <[email protected]>        https://momjian.us
  EDB                                      https://enterprisedb.com

  When a patient asks the doctor, "Am I going to die?", he means 
  "Am I going to die soon?"






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

* Re: pg_upgrade check for invalid databases
@ 2024-10-25 11:55  Daniel Gustafsson <[email protected]>
  parent: Bruce Momjian <[email protected]>
  0 siblings, 1 reply; 26+ messages in thread

From: Daniel Gustafsson @ 2024-10-25 11:55 UTC (permalink / raw)
  To: Bruce Momjian <[email protected]>; +Cc: Thomas Krennwallner <[email protected]>; Nathan Bossart <[email protected]>; Tom Lane <[email protected]>; pgsql-hackers

> On 14 Oct 2024, at 18:57, Bruce Momjian <[email protected]> wrote:

> What might be acceptable would be to add an option that would make
> pg_upgrade more tolerant of problems in many areas, that is a lot more
> research and discussion.

I agree that the concept of having pg_upgrade perform (opt-in) skipping and/or
repairs of the old cluster warrants a larger discussion in its own thread.
There has been significant amount of energy spent recently to add structure to
the checks, any new feature should be properly designed for the get-go.

In the meantime, the OP has a good point that it's a tad silly that pg_upgrade
fails hard on invalid databases instead of detecting and reporting like how
other errors are handled.  The attached adds this check and expands the report
wording to cover it.

--
Daniel Gustafsson



Attachments:

  [application/octet-stream] 0001-Find-invalid-databases-during-upgrade-check-stage.patch (5.7K, ../../[email protected]/2-0001-Find-invalid-databases-during-upgrade-check-stage.patch)
  download | inline diff:
From 3608e477cd5d03074f8d5b0fa9d2890cb6944ca5 Mon Sep 17 00:00:00 2001
From: Daniel Gustafsson <[email protected]>
Date: Fri, 25 Oct 2024 13:45:28 +0200
Subject: [PATCH] Find invalid databases during upgrade check stage

Before continuing with the check start by checking that all databases
allow connections to avoid a hard fail without proper error reporting.

Inspired by a larger patch by Thomas Krennwallner.

Discussion: https://postgr.es/m/[email protected]
---
 src/bin/pg_upgrade/check.c             | 36 ++++++++++++++++----------
 src/bin/pg_upgrade/t/002_pg_upgrade.pl |  2 +-
 2 files changed, 23 insertions(+), 15 deletions(-)

diff --git a/src/bin/pg_upgrade/check.c b/src/bin/pg_upgrade/check.c
index 12735a4268..2fce686530 100644
--- a/src/bin/pg_upgrade/check.c
+++ b/src/bin/pg_upgrade/check.c
@@ -18,7 +18,7 @@
 
 static void check_new_cluster_is_empty(void);
 static void check_is_install_user(ClusterInfo *cluster);
-static void check_proper_datallowconn(ClusterInfo *cluster);
+static void check_for_connection_status(ClusterInfo *cluster);
 static void check_for_prepared_transactions(ClusterInfo *cluster);
 static void check_for_isn_and_int8_passing_mismatch(ClusterInfo *cluster);
 static void check_for_user_defined_postfix_ops(ClusterInfo *cluster);
@@ -592,6 +592,12 @@ check_and_dump_old_cluster(void)
 	if (!user_opts.live_check)
 		start_postmaster(&old_cluster, true);
 
+	/*
+	 * First check that all databases allow connections since we'll otherwise
+	 * fail in later stages.
+	 */
+	check_for_connection_status(&old_cluster);
+
 	/*
 	 * Extract a list of databases, tables, and logical replication slots from
 	 * the old cluster.
@@ -607,7 +613,6 @@ check_and_dump_old_cluster(void)
 	 * Check for various failure cases
 	 */
 	check_is_install_user(&old_cluster);
-	check_proper_datallowconn(&old_cluster);
 	check_for_prepared_transactions(&old_cluster);
 	check_for_isn_and_int8_passing_mismatch(&old_cluster);
 
@@ -1089,14 +1094,14 @@ check_is_install_user(ClusterInfo *cluster)
 
 
 /*
- *	check_proper_datallowconn
+ *	check_for_connection_status
  *
  *	Ensure that all non-template0 databases allow connections since they
  *	otherwise won't be restored; and that template0 explicitly doesn't allow
  *	connections since it would make pg_dumpall --globals restore fail.
  */
 static void
-check_proper_datallowconn(ClusterInfo *cluster)
+check_for_connection_status(ClusterInfo *cluster)
 {
 	int			dbnum;
 	PGconn	   *conn_template1;
@@ -1104,6 +1109,7 @@ check_proper_datallowconn(ClusterInfo *cluster)
 	int			ntups;
 	int			i_datname;
 	int			i_datallowconn;
+	int			i_datconnlimit;
 	FILE	   *script = NULL;
 	char		output_path[MAXPGPATH];
 
@@ -1111,23 +1117,25 @@ check_proper_datallowconn(ClusterInfo *cluster)
 
 	snprintf(output_path, sizeof(output_path), "%s/%s",
 			 log_opts.basedir,
-			 "databases_with_datallowconn_false.txt");
+			 "databases_cannot_connect_to.txt");
 
 	conn_template1 = connectToServer(cluster, "template1");
 
 	/* get database names */
 	dbres = executeQueryOrDie(conn_template1,
-							  "SELECT	datname, datallowconn "
+							  "SELECT	datname, datallowconn, datconnlimit "
 							  "FROM	pg_catalog.pg_database");
 
 	i_datname = PQfnumber(dbres, "datname");
 	i_datallowconn = PQfnumber(dbres, "datallowconn");
+	i_datconnlimit = PQfnumber(dbres, "datconnlimit");
 
 	ntups = PQntuples(dbres);
 	for (dbnum = 0; dbnum < ntups; dbnum++)
 	{
 		char	   *datname = PQgetvalue(dbres, dbnum, i_datname);
 		char	   *datallowconn = PQgetvalue(dbres, dbnum, i_datallowconn);
+		char	   *datconnlimit = PQgetvalue(dbres, dbnum, i_datconnlimit);
 
 		if (strcmp(datname, "template0") == 0)
 		{
@@ -1139,10 +1147,10 @@ check_proper_datallowconn(ClusterInfo *cluster)
 		else
 		{
 			/*
-			 * avoid datallowconn == false databases from being skipped on
-			 * restore
+			 * Avoid datallowconn == false databases from being skipped on
+			 * restore, and ensure that no databases are marked invalid (-2).
 			 */
-			if (strcmp(datallowconn, "f") == 0)
+			if ((strcmp(datallowconn, "f") == 0) || strcmp(datconnlimit, "-2") == 0)
 			{
 				if (script == NULL && (script = fopen_priv(output_path, "w")) == NULL)
 					pg_fatal("could not open file \"%s\": %m", output_path);
@@ -1161,11 +1169,11 @@ check_proper_datallowconn(ClusterInfo *cluster)
 		fclose(script);
 		pg_log(PG_REPORT, "fatal");
 		pg_fatal("All non-template0 databases must allow connections, i.e. their\n"
-				 "pg_database.datallowconn must be true.  Your installation contains\n"
-				 "non-template0 databases with their pg_database.datallowconn set to\n"
-				 "false.  Consider allowing connection for all non-template0 databases\n"
-				 "or drop the databases which do not allow connections.  A list of\n"
-				 "databases with the problem is in the file:\n"
+				 "pg_database.datallowconn must be true and pg_database.datconnlimit\n"
+				 "must not be -2.  Your installation contains non-template0 databases\n"
+				 "which cannot be connected to.  Consider allowing connection for all\n"
+				 "non-template0 databases or drop the databases which do not allow\n"
+				 "connections.  A list of databases with the problem is in the file:\n"
 				 "    %s", output_path);
 	}
 	else
diff --git a/src/bin/pg_upgrade/t/002_pg_upgrade.pl b/src/bin/pg_upgrade/t/002_pg_upgrade.pl
index 17af2ce61e..9b51f9e666 100644
--- a/src/bin/pg_upgrade/t/002_pg_upgrade.pl
+++ b/src/bin/pg_upgrade/t/002_pg_upgrade.pl
@@ -424,7 +424,7 @@ SKIP:
 			$mode, '--check',
 		],
 		1,
-		[qr/invalid/],    # pg_upgrade prints errors on stdout :(
+		[qr/datconnlimit/],
 		[qr/^$/],
 		'invalid database causes failure');
 	rmtree($newnode->data_dir . "/pg_upgrade_output.d");
-- 
2.39.3 (Apple Git-146)



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

* Re: pg_upgrade check for invalid databases
@ 2024-11-01 00:36  Bruce Momjian <[email protected]>
  parent: Daniel Gustafsson <[email protected]>
  0 siblings, 1 reply; 26+ messages in thread

From: Bruce Momjian @ 2024-11-01 00:36 UTC (permalink / raw)
  To: Daniel Gustafsson <[email protected]>; +Cc: Thomas Krennwallner <[email protected]>; Nathan Bossart <[email protected]>; Tom Lane <[email protected]>; pgsql-hackers

On Fri, Oct 25, 2024 at 01:55:57PM +0200, Daniel Gustafsson wrote:
> > On 14 Oct 2024, at 18:57, Bruce Momjian <[email protected]> wrote:
> 
> > What might be acceptable would be to add an option that would make
> > pg_upgrade more tolerant of problems in many areas, that is a lot more
> > research and discussion.
> 
> I agree that the concept of having pg_upgrade perform (opt-in) skipping and/or
> repairs of the old cluster warrants a larger discussion in its own thread.
> There has been significant amount of energy spent recently to add structure to
> the checks, any new feature should be properly designed for the get-go.
> 
> In the meantime, the OP has a good point that it's a tad silly that pg_upgrade
> fails hard on invalid databases instead of detecting and reporting like how
> other errors are handled.  The attached adds this check and expands the report
> wording to cover it.

Agreed.

-- 
  Bruce Momjian  <[email protected]>        https://momjian.us
  EDB                                      https://enterprisedb.com

  When a patient asks the doctor, "Am I going to die?", he means 
  "Am I going to die soon?"






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

* Re: pg_upgrade check for invalid databases
@ 2024-11-06 15:03  Daniel Gustafsson <[email protected]>
  parent: Bruce Momjian <[email protected]>
  0 siblings, 0 replies; 26+ messages in thread

From: Daniel Gustafsson @ 2024-11-06 15:03 UTC (permalink / raw)
  To: Bruce Momjian <[email protected]>; +Cc: Thomas Krennwallner <[email protected]>; Nathan Bossart <[email protected]>; Tom Lane <[email protected]>; pgsql-hackers

> On 1 Nov 2024, at 01:36, Bruce Momjian <[email protected]> wrote:
> On Fri, Oct 25, 2024 at 01:55:57PM +0200, Daniel Gustafsson wrote:

>> In the meantime, the OP has a good point that it's a tad silly that pg_upgrade
>> fails hard on invalid databases instead of detecting and reporting like how
>> other errors are handled.  The attached adds this check and expands the report
>> wording to cover it.
> 
> Agreed.

I've applied this part, the discussion on whether or not pg_upgrade should gain
capabilities to skip and/or fix issues should probably be carried over in a new
thread.

--
Daniel Gustafsson







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

end of thread, other threads:[~2024-11-06 15:03 UTC | newest]

Thread overview: 26+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2024-01-07 19:50 [PATCH v3 06/17] lazy_scan_prune reorder freeze execution logic Melanie Plageman <[email protected]>
2024-01-07 19:50 [PATCH v3 06/17] lazy_scan_prune reorder freeze execution logic Melanie Plageman <[email protected]>
2024-01-07 19:50 [PATCH v2 06/17] lazy_scan_prune reorder freeze execution logic Melanie Plageman <[email protected]>
2024-01-07 19:50 [PATCH v3 06/17] lazy_scan_prune reorder freeze execution logic Melanie Plageman <[email protected]>
2024-01-07 19:50 [PATCH v2 06/17] lazy_scan_prune reorder freeze execution logic Melanie Plageman <[email protected]>
2024-01-07 19:50 [PATCH v2 06/17] lazy_scan_prune reorder freeze execution logic Melanie Plageman <[email protected]>
2024-03-19 23:30 [PATCH v4 08/19] lazy_scan_prune reorder freeze execution logic Melanie Plageman <[email protected]>
2024-03-19 23:30 [PATCH v4 08/19] lazy_scan_prune reorder freeze execution logic Melanie Plageman <[email protected]>
2024-03-19 23:30 [PATCH v4 08/19] lazy_scan_prune reorder freeze execution logic Melanie Plageman <[email protected]>
2024-03-25 23:39 [PATCH v9 07/21] lazy_scan_prune reorder freeze execution logic Melanie Plageman <[email protected]>
2024-03-25 23:39 [PATCH v9 07/21] lazy_scan_prune reorder freeze execution logic Melanie Plageman <[email protected]>
2024-03-25 23:39 [PATCH v7 07/16] lazy_scan_prune reorder freeze execution logic Melanie Plageman <[email protected]>
2024-03-25 23:39 [PATCH v7 07/16] lazy_scan_prune reorder freeze execution logic Melanie Plageman <[email protected]>
2024-09-30 21:29 Re: pg_upgrade check for invalid databases Daniel Gustafsson <[email protected]>
2024-09-30 22:20 ` Re: pg_upgrade check for invalid databases Tom Lane <[email protected]>
2024-10-01 07:28   ` Re: pg_upgrade check for invalid databases Daniel Gustafsson <[email protected]>
2024-10-07 19:37     ` Re: pg_upgrade check for invalid databases Bruce Momjian <[email protected]>
2024-10-07 20:04       ` Re: pg_upgrade check for invalid databases Nathan Bossart <[email protected]>
2024-10-11 08:00         ` Re: pg_upgrade check for invalid databases Daniel Gustafsson <[email protected]>
2024-10-13 12:28           ` Re: pg_upgrade check for invalid databases Thomas Krennwallner <[email protected]>
2024-10-14 16:57             ` Re: pg_upgrade check for invalid databases Bruce Momjian <[email protected]>
2024-10-25 11:55               ` Re: pg_upgrade check for invalid databases Daniel Gustafsson <[email protected]>
2024-11-01 00:36                 ` Re: pg_upgrade check for invalid databases Bruce Momjian <[email protected]>
2024-11-06 15:03                   ` Re: pg_upgrade check for invalid databases Daniel Gustafsson <[email protected]>
2024-10-01 00:35 ` Re: pg_upgrade check for invalid databases Thomas Krennwallner <[email protected]>
2024-10-01 07:42   ` Re: pg_upgrade check for invalid databases Daniel Gustafsson <[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