public inbox for [email protected]
help / color / mirror / Atom feed[PATCH v2 06/17] lazy_scan_prune reorder freeze execution logic
19+ messages / 4 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; 19+ 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] 19+ 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; 19+ 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] 19+ 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; 19+ 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] 19+ 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; 19+ 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] 19+ 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; 19+ 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] 19+ 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; 19+ 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] 19+ 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; 19+ 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] 19+ 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; 19+ 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] 19+ 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; 19+ 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] 19+ 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; 19+ 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] 19+ 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; 19+ 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] 19+ 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; 19+ 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] 19+ 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; 19+ 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] 19+ messages in thread
* Re: Spinlock can be released twice in procsignal.c
@ 2025-02-26 04:56 Michael Paquier <[email protected]>
2025-02-26 06:57 ` Re: Spinlock can be released twice in procsignal.c Maxim Orlov <[email protected]>
0 siblings, 1 reply; 19+ messages in thread
From: Michael Paquier @ 2025-02-26 04:56 UTC (permalink / raw)
To: Andrey Borodin <[email protected]>; +Cc: Maksim.Melnikov <[email protected]>; PostgreSQL Hackers <[email protected]>; Heikki Linnakangas <[email protected]>; Jelte Fennema-Nio <[email protected]>
On Wed, Feb 26, 2025 at 09:08:53AM +0500, Andrey Borodin wrote:
> Looks like the oversight in 9d9b9d4. IMO the fix is correct.
if (pg_atomic_read_u32(&slot->pss_pid) != 0)
{
- SpinLockRelease(&slot->pss_mutex);
elog(LOG, "process %d taking over ProcSignal slot %d, but it's not empty",
MyProcPid, MyProcNumber);
}
This fix is not correct. No system function calls (well basically
most of them) or even more no PostgreSQL-specific calls should happen
while holding a spinlock. elog() is a good example of what not to do.
One example: imagine a palloc failure while holding this spinlock in
this elog().
The code should be restructured so as we read pss_pid while holding
the spinlock, release the spinlock, and then LOG. Based on the
structure of this routine, you could just assign a boolean to decide
if something should be logged and delay the LOG until the spinlock is
released, because we don't intend an early exit like in
CleanupProcSignalState(). In ~16, ProcSignalInit() is able to LOG
things early, but the ProcSignal is forced even if pss_pid is set, so
delaying the LOG to be generated after updating ProcSignal does not
matter.
--
Michael
Attachments:
[application/pgp-signature] signature.asc (833B, ../../[email protected]/2-signature.asc)
download
^ permalink raw reply [nested|flat] 19+ messages in thread
* Re: Spinlock can be released twice in procsignal.c
2025-02-26 04:56 Re: Spinlock can be released twice in procsignal.c Michael Paquier <[email protected]>
@ 2025-02-26 06:57 ` Maxim Orlov <[email protected]>
2025-02-26 08:25 ` Re: Spinlock can be released twice in procsignal.c Andrey Borodin <[email protected]>
0 siblings, 1 reply; 19+ messages in thread
From: Maxim Orlov @ 2025-02-26 06:57 UTC (permalink / raw)
To: Michael Paquier <[email protected]>; +Cc: Andrey Borodin <[email protected]>; Maksim.Melnikov <[email protected]>; PostgreSQL Hackers <[email protected]>; Heikki Linnakangas <[email protected]>; Jelte Fennema-Nio <[email protected]>
On Wed, 26 Feb 2025 at 07:56, Michael Paquier <[email protected]> wrote:
>
> This fix is not correct. No system function calls (well basically
> most of them) or even more no PostgreSQL-specific calls should happen
> while holding a spinlock. elog() is a good example of what not to do.
> One example: imagine a palloc failure while holding this spinlock in
> this elog().
Indeed. PFA the correct one.
--
Best regards,
Maxim Orlov.
Attachments:
[application/octet-stream] v2-0001-Avoid-double-spinlock-release.patch (1.6K, ../../CACG=ezbScO5O2CTZYy-Rqs-VHQ_f3aaRWL6VQZpyjC4uMVVDoA@mail.gmail.com/3-v2-0001-Avoid-double-spinlock-release.patch)
download | inline diff:
From ce41749e7df8339b5c1c90a42336f45d440028bb Mon Sep 17 00:00:00 2001
From: Maxim Orlov <[email protected]>
Date: Wed, 26 Feb 2025 09:42:43 +0300
Subject: [PATCH v2] Avoid double spinlock release
---
src/backend/storage/ipc/procsignal.c | 14 ++++++++------
1 file changed, 8 insertions(+), 6 deletions(-)
diff --git a/src/backend/storage/ipc/procsignal.c b/src/backend/storage/ipc/procsignal.c
index 7401b6e625e..6e9f9c0f2e5 100644
--- a/src/backend/storage/ipc/procsignal.c
+++ b/src/backend/storage/ipc/procsignal.c
@@ -167,6 +167,7 @@ ProcSignalInit(bool cancel_key_valid, int32 cancel_key)
{
ProcSignalSlot *slot;
uint64 barrier_generation;
+ bool logit;
if (MyProcNumber < 0)
elog(ERROR, "MyProcNumber not set");
@@ -176,12 +177,7 @@ ProcSignalInit(bool cancel_key_valid, int32 cancel_key)
/* sanity check */
SpinLockAcquire(&slot->pss_mutex);
- if (pg_atomic_read_u32(&slot->pss_pid) != 0)
- {
- SpinLockRelease(&slot->pss_mutex);
- elog(LOG, "process %d taking over ProcSignal slot %d, but it's not empty",
- MyProcPid, MyProcNumber);
- }
+ logit = pg_atomic_read_u32(&slot->pss_pid) != 0;
/* Clear out any leftover signal reasons */
MemSet(slot->pss_signalFlags, 0, NUM_PROCSIGNALS * sizeof(sig_atomic_t));
@@ -208,6 +204,12 @@ ProcSignalInit(bool cancel_key_valid, int32 cancel_key)
SpinLockRelease(&slot->pss_mutex);
+ if (logit)
+ {
+ elog(LOG, "process %d taking over ProcSignal slot %d, but it's not empty",
+ MyProcPid, MyProcNumber);
+ }
+
/* Remember slot location for CheckProcSignal */
MyProcSignalSlot = slot;
--
2.48.1
^ permalink raw reply [nested|flat] 19+ messages in thread
* Re: Spinlock can be released twice in procsignal.c
2025-02-26 04:56 Re: Spinlock can be released twice in procsignal.c Michael Paquier <[email protected]>
2025-02-26 06:57 ` Re: Spinlock can be released twice in procsignal.c Maxim Orlov <[email protected]>
@ 2025-02-26 08:25 ` Andrey Borodin <[email protected]>
2025-02-26 09:33 ` Re: Spinlock can be released twice in procsignal.c Maxim Orlov <[email protected]>
0 siblings, 1 reply; 19+ messages in thread
From: Andrey Borodin @ 2025-02-26 08:25 UTC (permalink / raw)
To: Maxim Orlov <[email protected]>; +Cc: Michael Paquier <[email protected]>; Maksim.Melnikov <[email protected]>; PostgreSQL Hackers <[email protected]>; Heikki Linnakangas <[email protected]>; Jelte Fennema-Nio <[email protected]>
Michael, thanks a lot for the detailed explanation.
> On 26 Feb 2025, at 11:57, Maxim Orlov <[email protected]> wrote:
>
> Indeed. PFA the correct one.
>
> <v2-0001-Avoid-double-spinlock-release.patch>
I’d suggest to give some more descriptive name to “logit” and expand comment “/* sanity check */“. This comment was easier to understand when elog() was near, but now IMO we can have few words about what is going on.
Best regards, Andrey Borodin.
^ permalink raw reply [nested|flat] 19+ messages in thread
* Re: Spinlock can be released twice in procsignal.c
2025-02-26 04:56 Re: Spinlock can be released twice in procsignal.c Michael Paquier <[email protected]>
2025-02-26 06:57 ` Re: Spinlock can be released twice in procsignal.c Maxim Orlov <[email protected]>
2025-02-26 08:25 ` Re: Spinlock can be released twice in procsignal.c Andrey Borodin <[email protected]>
@ 2025-02-26 09:33 ` Maxim Orlov <[email protected]>
2025-02-27 00:46 ` Re: Spinlock can be released twice in procsignal.c Michael Paquier <[email protected]>
0 siblings, 1 reply; 19+ messages in thread
From: Maxim Orlov @ 2025-02-26 09:33 UTC (permalink / raw)
To: Andrey Borodin <[email protected]>; +Cc: Michael Paquier <[email protected]>; Maksim.Melnikov <[email protected]>; PostgreSQL Hackers <[email protected]>; Heikki Linnakangas <[email protected]>; Jelte Fennema-Nio <[email protected]>
On Wed, 26 Feb 2025 at 11:26, Andrey Borodin <[email protected]> wrote:
>
> I’d suggest to give some more descriptive name to “logit” and expand
> comment “/* sanity check */“. This comment was easier to understand when
> elog() was near, but now IMO we can have few words about what is going on.
Done. Except for a new name for "logit" variable. Unfortunately, I can't
think of anything sane. As an example I looked at sequence.c. The same name
is used there. I will gladly change this name to whatever you want if it
still look misleading or incomplete for you in some way or another. Just
write the name that you think is correct.
--
Best regards,
Maxim Orlov.
Attachments:
[application/octet-stream] v3-0001-Avoid-double-spinlock-release.patch (2.0K, ../../CACG=ezbD9sryj+8SjGXPC0PHB-z_6NW6J9RSfXqtXRDFXPv+uQ@mail.gmail.com/3-v3-0001-Avoid-double-spinlock-release.patch)
download | inline diff:
From d896d372b8acad33d278b9f6d131eb6db8cd5fe2 Mon Sep 17 00:00:00 2001
From: Maxim Orlov <[email protected]>
Date: Wed, 26 Feb 2025 09:42:43 +0300
Subject: [PATCH v3] Avoid double spinlock release
---
src/backend/storage/ipc/procsignal.c | 21 ++++++++++++++-------
1 file changed, 14 insertions(+), 7 deletions(-)
diff --git a/src/backend/storage/ipc/procsignal.c b/src/backend/storage/ipc/procsignal.c
index 7401b6e625..390e6da868 100644
--- a/src/backend/storage/ipc/procsignal.c
+++ b/src/backend/storage/ipc/procsignal.c
@@ -167,6 +167,7 @@ ProcSignalInit(bool cancel_key_valid, int32 cancel_key)
{
ProcSignalSlot *slot;
uint64 barrier_generation;
+ bool logit; /* log if we are taking over a used slot */
if (MyProcNumber < 0)
elog(ERROR, "MyProcNumber not set");
@@ -174,14 +175,13 @@ ProcSignalInit(bool cancel_key_valid, int32 cancel_key)
elog(ERROR, "unexpected MyProcNumber %d in ProcSignalInit (max %d)", MyProcNumber, NumProcSignalSlots);
slot = &ProcSignal->psh_slot[MyProcNumber];
- /* sanity check */
SpinLockAcquire(&slot->pss_mutex);
- if (pg_atomic_read_u32(&slot->pss_pid) != 0)
- {
- SpinLockRelease(&slot->pss_mutex);
- elog(LOG, "process %d taking over ProcSignal slot %d, but it's not empty",
- MyProcPid, MyProcNumber);
- }
+ /*
+ * Sanity check. We should warn is we are taking over a used slot. Since we
+ * are holding spin lock at this point, we cannot elog from here. So, we
+ * have to delay the log until the spin lock is released.
+ */
+ logit = pg_atomic_read_u32(&slot->pss_pid) != 0;
/* Clear out any leftover signal reasons */
MemSet(slot->pss_signalFlags, 0, NUM_PROCSIGNALS * sizeof(sig_atomic_t));
@@ -208,6 +208,13 @@ ProcSignalInit(bool cancel_key_valid, int32 cancel_key)
SpinLockRelease(&slot->pss_mutex);
+ /* Spin lock is released, now we can log. */
+ if (logit)
+ {
+ elog(LOG, "process %d taking over ProcSignal slot %d, but it's not empty",
+ MyProcPid, MyProcNumber);
+ }
+
/* Remember slot location for CheckProcSignal */
MyProcSignalSlot = slot;
--
2.43.0
^ permalink raw reply [nested|flat] 19+ messages in thread
* Re: Spinlock can be released twice in procsignal.c
2025-02-26 04:56 Re: Spinlock can be released twice in procsignal.c Michael Paquier <[email protected]>
2025-02-26 06:57 ` Re: Spinlock can be released twice in procsignal.c Maxim Orlov <[email protected]>
2025-02-26 08:25 ` Re: Spinlock can be released twice in procsignal.c Andrey Borodin <[email protected]>
2025-02-26 09:33 ` Re: Spinlock can be released twice in procsignal.c Maxim Orlov <[email protected]>
@ 2025-02-27 00:46 ` Michael Paquier <[email protected]>
2025-02-27 06:11 ` Re: Spinlock can be released twice in procsignal.c Maxim Orlov <[email protected]>
0 siblings, 1 reply; 19+ messages in thread
From: Michael Paquier @ 2025-02-27 00:46 UTC (permalink / raw)
To: Maxim Orlov <[email protected]>; +Cc: Andrey Borodin <[email protected]>; Maksim.Melnikov <[email protected]>; PostgreSQL Hackers <[email protected]>; Heikki Linnakangas <[email protected]>; Jelte Fennema-Nio <[email protected]>
On Wed, Feb 26, 2025 at 12:33:16PM +0300, Maxim Orlov wrote:
> Done. Except for a new name for "logit" variable. Unfortunately, I can't
> think of anything sane. As an example I looked at sequence.c. The same name
> is used there. I will gladly change this name to whatever you want if it
> still look misleading or incomplete for you in some way or another. Just
> write the name that you think is correct.
"old_pss_pid", because this is an old pss_pid value from a previous
slot. Keeping the value around after releasing the slot is also
useful for debugging, so rather than a boolean it is possible to use a
uint32. The "sanity check" comment was also a bit out-of-order
because it was written just before acquiring the spinlock.
Tweaked a bit the whole, and the result looked OK so applied on HEAD.
--
Michael
Attachments:
[application/pgp-signature] signature.asc (833B, ../../[email protected]/2-signature.asc)
download
^ permalink raw reply [nested|flat] 19+ messages in thread
* Re: Spinlock can be released twice in procsignal.c
2025-02-26 04:56 Re: Spinlock can be released twice in procsignal.c Michael Paquier <[email protected]>
2025-02-26 06:57 ` Re: Spinlock can be released twice in procsignal.c Maxim Orlov <[email protected]>
2025-02-26 08:25 ` Re: Spinlock can be released twice in procsignal.c Andrey Borodin <[email protected]>
2025-02-26 09:33 ` Re: Spinlock can be released twice in procsignal.c Maxim Orlov <[email protected]>
2025-02-27 00:46 ` Re: Spinlock can be released twice in procsignal.c Michael Paquier <[email protected]>
@ 2025-02-27 06:11 ` Maxim Orlov <[email protected]>
0 siblings, 0 replies; 19+ messages in thread
From: Maxim Orlov @ 2025-02-27 06:11 UTC (permalink / raw)
To: Michael Paquier <[email protected]>; +Cc: Andrey Borodin <[email protected]>; Maksim.Melnikov <[email protected]>; PostgreSQL Hackers <[email protected]>; Heikki Linnakangas <[email protected]>; Jelte Fennema-Nio <[email protected]>
Great! Looks very good. Thanks a lot!
--
Best regards,
Maxim Orlov.
^ permalink raw reply [nested|flat] 19+ messages in thread
end of thread, other threads:[~2025-02-27 06:11 UTC | newest]
Thread overview: 19+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
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-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 v3 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 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-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]>
2025-02-26 04:56 Re: Spinlock can be released twice in procsignal.c Michael Paquier <[email protected]>
2025-02-26 06:57 ` Re: Spinlock can be released twice in procsignal.c Maxim Orlov <[email protected]>
2025-02-26 08:25 ` Re: Spinlock can be released twice in procsignal.c Andrey Borodin <[email protected]>
2025-02-26 09:33 ` Re: Spinlock can be released twice in procsignal.c Maxim Orlov <[email protected]>
2025-02-27 00:46 ` Re: Spinlock can be released twice in procsignal.c Michael Paquier <[email protected]>
2025-02-27 06:11 ` Re: Spinlock can be released twice in procsignal.c Maxim Orlov <[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