public inbox for [email protected]
help / color / mirror / Atom feed[PATCH v2 06/17] lazy_scan_prune reorder freeze execution logic
16+ messages / 3 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; 16+ 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] 16+ 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; 16+ 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] 16+ 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; 16+ 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] 16+ 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; 16+ 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] 16+ 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; 16+ 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] 16+ 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; 16+ 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] 16+ 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; 16+ 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] 16+ 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; 16+ 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] 16+ 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; 16+ 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] 16+ 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; 16+ 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] 16+ 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; 16+ 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] 16+ 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; 16+ 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] 16+ 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; 16+ 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] 16+ messages in thread
* Re: Streaming relation data out of order
@ 2025-04-09 07:17 Thomas Munro <[email protected]>
0 siblings, 1 reply; 16+ messages in thread
From: Thomas Munro @ 2025-04-09 07:17 UTC (permalink / raw)
To: Robert Haas <[email protected]>; +Cc: pgsql-hackers
Here is a more fleshed out version of this concept patch, now that we
have lots of streams wired up to try it with. Bitmap Heap Scan seemed
to be a good candidate.
postgres=# create table t (a int unique, b int unique);
CREATE TABLE
postgres=# insert into t select generate_series(1, 100000),
generate_series(1, 100000);
INSERT 0 100000
postgres=# select ctid from t where (a between 1 and 8000 or b between
1 and 8000) and ctid::text like '%,1)';
ctid
--------
(0,1)
(1,1)
(2,1)
... pages in order ...
(33,1)
(34,1)
(35,1)
(36 rows)
postgres=# select pg_buffercache_evict(bufferid) from pg_buffercache
where relfilenode = pg_relation_filenode('t'::regclass) and
relblocknumber % 3 != 2;
... some pages being kicked out to create a mixed hit/miss scan ...
postgres=# select ctid from t where (a between 1 and 8000 or b between
1 and 8000) and ctid::text like '%,1)';
ctid
--------
(0,1)
(1,1)
(2,1)
... order during early distance ramp-up ...
(12,1)
(20,1) <-- chaos reigns
(23,1)
(17,1)
(26,1)
(13,1)
(29,1)
...
(34,1)
(36 rows)
One weird issue, not just with reordering, is that read_stream.c's
distance cools off too easily with some simple test patterns. Start
at 1, double on miss, subtract one on hit, repeat, and you can be
trapped alternating between 1 and 2 when you'd certainly benefit from
IO concurrency and also reordering. It may need a longer memory.
That seemed like too artificial a problem to worry about for v18, but
it's why I used relblocknumber % 3 != 2 and not eg relblocknumber % 2
!= 1 above.
Attachments:
[text/x-patch] v2-0001-Move-read-stream-modulo-arithmetic-into-functions.patch (5.2K, ../../CA+hUKG+53X=26HwQrykdzwtt-j+jWGU3WRa8VEM7AFyyQK-Atg@mail.gmail.com/2-v2-0001-Move-read-stream-modulo-arithmetic-into-functions.patch)
download | inline diff:
From 3b460cb102c8af49a0af0d3b08bf86a436564fd3 Mon Sep 17 00:00:00 2001
From: Thomas Munro <[email protected]>
Date: Sat, 15 Feb 2025 14:47:25 +1300
Subject: [PATCH v2 1/4] Move read stream modulo arithmetic into functions.
Several places have open-coded circular index arithmetic. Make some
common functions for better readability and consistent assertion
checking. This avoids adding yet more open-coding in later patches.
---
src/backend/storage/aio/read_stream.c | 93 +++++++++++++++++++++------
1 file changed, 74 insertions(+), 19 deletions(-)
diff --git a/src/backend/storage/aio/read_stream.c b/src/backend/storage/aio/read_stream.c
index 0e7f5557f5c..0d3dd65bfed 100644
--- a/src/backend/storage/aio/read_stream.c
+++ b/src/backend/storage/aio/read_stream.c
@@ -216,6 +216,67 @@ read_stream_unget_block(ReadStream *stream, BlockNumber blocknum)
stream->buffered_blocknum = blocknum;
}
+/*
+ * Increment buffer index, wrapping around at queue size.
+ */
+static inline void
+read_stream_advance_buffer(ReadStream *stream, int16 *index)
+{
+ Assert(*index >= 0);
+ Assert(*index < stream->queue_size);
+
+ *index += 1;
+ if (*index == stream->queue_size)
+ *index = 0;
+}
+
+/*
+ * Increment buffer index by n, wrapping around at queue size.
+ */
+static inline void
+read_stream_advance_buffer_n(ReadStream *stream, int16 *index, int16 n)
+{
+ Assert(*index >= 0);
+ Assert(*index < stream->queue_size);
+ Assert(n <= stream->io_combine_limit);
+
+ *index += n;
+ if (*index >= stream->queue_size)
+ *index -= stream->queue_size;
+
+ Assert(*index >= 0);
+ Assert(*index < stream->queue_size);
+}
+
+/*
+ * Decrement buffer index, wrapping around at queue size.
+ */
+static inline void
+read_stream_retreat_buffer(ReadStream *stream, int16 *index)
+{
+ Assert(*index >= 0);
+ Assert(*index < stream->queue_size);
+
+ if (*index == 0)
+ *index = stream->queue_size - 1;
+ else
+ *index -= 1;
+}
+
+/*
+ * Increment IO index, wrapping around at queue size.
+ */
+static inline void
+read_stream_advance_io(ReadStream *stream, int16 *index)
+{
+ Assert(*index >= 0);
+ Assert(*index < stream->max_ios);
+
+ *index += 1;
+ if (*index == stream->max_ios)
+ *index = 0;
+}
+
/*
* Start as much of the current pending read as we can. If we have to split it
* because of the per-backend buffer limit, or the buffer manager decides to
@@ -353,8 +414,7 @@ read_stream_start_pending_read(ReadStream *stream)
* Look-ahead distance will be adjusted after waiting.
*/
stream->ios[io_index].buffer_index = buffer_index;
- if (++stream->next_io_index == stream->max_ios)
- stream->next_io_index = 0;
+ read_stream_advance_io(stream, &stream->next_io_index);
Assert(stream->ios_in_progress < stream->max_ios);
stream->ios_in_progress++;
stream->seq_blocknum = stream->pending_read_blocknum + nblocks;
@@ -390,11 +450,8 @@ read_stream_start_pending_read(ReadStream *stream)
sizeof(stream->buffers[0]) * overflow);
}
- /* Compute location of start of next read, without using % operator. */
- buffer_index += nblocks;
- if (buffer_index >= stream->queue_size)
- buffer_index -= stream->queue_size;
- Assert(buffer_index >= 0 && buffer_index < stream->queue_size);
+ /* Move to the location of start of next read. */
+ read_stream_advance_buffer_n(stream, &buffer_index, nblocks);
stream->next_buffer_index = buffer_index;
/* Adjust the pending read to cover the remaining portion, if any. */
@@ -432,12 +489,12 @@ read_stream_look_ahead(ReadStream *stream)
/*
* See which block the callback wants next in the stream. We need to
* compute the index of the Nth block of the pending read including
- * wrap-around, but we don't want to use the expensive % operator.
+ * wrap-around.
*/
- buffer_index = stream->next_buffer_index + stream->pending_read_nblocks;
- if (buffer_index >= stream->queue_size)
- buffer_index -= stream->queue_size;
- Assert(buffer_index >= 0 && buffer_index < stream->queue_size);
+ buffer_index = stream->next_buffer_index;
+ read_stream_advance_buffer_n(stream,
+ &buffer_index,
+ stream->pending_read_nblocks);
per_buffer_data = get_per_buffer_data(stream, buffer_index);
blocknum = read_stream_get_block(stream, per_buffer_data);
if (blocknum == InvalidBlockNumber)
@@ -940,12 +997,12 @@ read_stream_next_buffer(ReadStream *stream, void **per_buffer_data)
*/
if (stream->per_buffer_data)
{
+ int16 index;
void *per_buffer_data;
- per_buffer_data = get_per_buffer_data(stream,
- oldest_buffer_index == 0 ?
- stream->queue_size - 1 :
- oldest_buffer_index - 1);
+ index = oldest_buffer_index;
+ read_stream_retreat_buffer(stream, &index);
+ per_buffer_data = get_per_buffer_data(stream, index);
#if defined(CLOBBER_FREED_MEMORY)
/* This also tells Valgrind the memory is "noaccess". */
@@ -963,9 +1020,7 @@ read_stream_next_buffer(ReadStream *stream, void **per_buffer_data)
stream->pinned_buffers--;
/* Advance oldest buffer, with wrap-around. */
- stream->oldest_buffer_index++;
- if (stream->oldest_buffer_index == stream->queue_size)
- stream->oldest_buffer_index = 0;
+ read_stream_advance_buffer(stream, &stream->oldest_buffer_index);
/* Prepare for the next call. */
read_stream_look_ahead(stream);
--
2.39.5
[text/x-patch] v2-0002-Improve-read_stream.c-program-flow.patch (2.3K, ../../CA+hUKG+53X=26HwQrykdzwtt-j+jWGU3WRa8VEM7AFyyQK-Atg@mail.gmail.com/3-v2-0002-Improve-read_stream.c-program-flow.patch)
download | inline diff:
From ff29856d80c1c3778cc99a35ed8f2ba8c642d37f Mon Sep 17 00:00:00 2001
From: Thomas Munro <[email protected]>
Date: Wed, 9 Apr 2025 15:16:31 +1200
Subject: [PATCH v2 2/4] Improve read_stream.c program flow.
Commit 55918f79 removed the space-based reason for calling
read_stream_look_ahead() at the top and bottom of
read_stream_next_buffer().
Commit 7ea8cd15 removed the need to call it with a different flag in
each spot.
We can delete a couple of dozen lines and just call it at the top now.
That's much tidier, but also required for a later patch that reorders
the queue.
---
src/backend/storage/aio/read_stream.c | 29 +++++----------------------
1 file changed, 5 insertions(+), 24 deletions(-)
diff --git a/src/backend/storage/aio/read_stream.c b/src/backend/storage/aio/read_stream.c
index 0d3dd65bfed..a98213d4df2 100644
--- a/src/backend/storage/aio/read_stream.c
+++ b/src/backend/storage/aio/read_stream.c
@@ -909,28 +909,12 @@ read_stream_next_buffer(ReadStream *stream, void **per_buffer_data)
}
#endif
- if (unlikely(stream->pinned_buffers == 0))
+ /* Look ahead and check for end of stream. */
+ read_stream_look_ahead(stream);
+ if (stream->pinned_buffers == 0)
{
- Assert(stream->oldest_buffer_index == stream->next_buffer_index);
-
- /* End of stream reached? */
- if (stream->distance == 0)
- return InvalidBuffer;
-
- /*
- * The usual order of operations is that we look ahead at the bottom
- * of this function after potentially finishing an I/O and making
- * space for more, but if we're just starting up we'll need to crank
- * the handle to get started.
- */
- read_stream_look_ahead(stream);
-
- /* End of stream reached? */
- if (stream->pinned_buffers == 0)
- {
- Assert(stream->distance == 0);
- return InvalidBuffer;
- }
+ Assert(stream->distance == 0);
+ return InvalidBuffer;
}
/* Grab the oldest pinned buffer and associated per-buffer data. */
@@ -1022,9 +1006,6 @@ read_stream_next_buffer(ReadStream *stream, void **per_buffer_data)
/* Advance oldest buffer, with wrap-around. */
read_stream_advance_buffer(stream, &stream->oldest_buffer_index);
- /* Prepare for the next call. */
- read_stream_look_ahead(stream);
-
#ifndef READ_STREAM_DISABLE_FAST_PATH
/* See if we can take the fast path for all-cached scans next time. */
if (stream->ios_in_progress == 0 &&
--
2.39.5
[text/x-patch] v2-0003-Add-READ_STREAM_OUT_OF_ORDER.patch (10.9K, ../../CA+hUKG+53X=26HwQrykdzwtt-j+jWGU3WRa8VEM7AFyyQK-Atg@mail.gmail.com/4-v2-0003-Add-READ_STREAM_OUT_OF_ORDER.patch)
download | inline diff:
From d4a86ee55c0a1d4e77a5de06df9f625e37863371 Mon Sep 17 00:00:00 2001
From: Thomas Munro <[email protected]>
Date: Sat, 6 Apr 2024 13:28:28 +1300
Subject: [PATCH v2 3/4] Add READ_STREAM_OUT_OF_ORDER.
If the stream consumer authorizes it, already cached buffers can jump
the queue and be emitted in LIFO order if there are IOs running. This
gives the storage more time to complete IOs, and the consumer something
to work on immediately.
---
src/backend/storage/aio/read_stream.c | 214 +++++++++++++++++++++++---
src/include/storage/read_stream.h | 8 +
2 files changed, 202 insertions(+), 20 deletions(-)
diff --git a/src/backend/storage/aio/read_stream.c b/src/backend/storage/aio/read_stream.c
index a98213d4df2..e90db1546fd 100644
--- a/src/backend/storage/aio/read_stream.c
+++ b/src/backend/storage/aio/read_stream.c
@@ -105,6 +105,7 @@ struct ReadStream
bool batch_mode; /* READ_STREAM_USE_BATCHING */
bool advice_enabled;
bool temporary;
+ bool out_of_order_enabled;
/*
* One-block buffer to support 'ungetting' a block number, to resolve flow
@@ -154,6 +155,27 @@ get_per_buffer_data(ReadStream *stream, int16 buffer_index)
stream->per_buffer_data_size * buffer_index;
}
+/*
+ * To help catch bugs, wipe per-buffer data when it shouldn't be accessed
+ * again by client code. This has no effect unless CLOBBER_FREED_MEMORY or
+ * USE_VALGRIND is defined. After clobbering, this per-buffer data object
+ * must be explicitly marked as undefined or defined before it is accessed
+ * again.
+ */
+static void
+clobber_per_buffer_data(ReadStream *stream, int16 buffer_index)
+{
+#ifdef CLOBBER_FREED_MEMORY
+ /* This also tells Valgrind the memory is "noaccess". */
+ wipe_mem(get_per_buffer_data(stream, buffer_index),
+ stream->per_buffer_data_size);
+#else
+ /* Tell it ourselves. */
+ VALGRIND_MAKE_MEM_NOACCESS(get_per_buffer_data(stream, buffer_index),
+ stream->per_buffer_data_size);
+#endif
+}
+
/*
* General-use ReadStreamBlockNumberCB for block range scans. Loops over the
* blocks [current_blocknum, last_exclusive).
@@ -277,6 +299,158 @@ read_stream_advance_io(ReadStream *stream, int16 *index)
*index = 0;
}
+/*
+ * Relocate a single valid buffer that has been stored in FIFO position to
+ * LIFO position so it will be consumed next.
+ *
+ * When there is no per-buffer data, reordering the buffer queue itself is
+ * cheap, as we just relocate a buffer and adjust an index. Per-buffer data
+ * is more expensive: we often have more of those ahead of us corresponding to
+ * the blocks of the current pending read, and their size is user-controlled
+ * but expected to be very small. (It's not yet clear if a more complex data
+ * structure is worth the overheads to avoid this.)
+ */
+static void
+read_stream_reorder(ReadStream *stream)
+{
+ int16 gap_index = stream->next_buffer_index;
+ int16 new_index = stream->oldest_buffer_index; /* retreated below */
+ int16 per_buffer_data_size = stream->per_buffer_data_size;
+ int16 per_buffer_data_count = 0;
+ int16 forwarded_buffers = stream->forwarded_buffers;
+
+ Assert(BufferIsValid(stream->buffers[gap_index]));
+ Assert(stream->ios_in_progress > 0);
+ Assert(stream->out_of_order_enabled);
+
+ /* Move "gap" entry to "new" entry. */
+ read_stream_retreat_buffer(stream, &new_index);
+ Assert(new_index >= stream->initialized_buffers ||
+ stream->buffers[new_index] == InvalidBuffer);
+ stream->buffers[new_index] = stream->buffers[gap_index];
+ stream->buffers[gap_index] = InvalidBuffer;
+ stream->oldest_buffer_index = new_index;
+
+ /* If there are forwarded buffers (rare), then we'll have to shift them. */
+ if (forwarded_buffers > 0)
+ {
+ int16 zap_index = gap_index + forwarded_buffers;
+
+ Assert(forwarded_buffers < stream->io_combine_limit);
+
+ /* Shift to fill in the gap created above, and zap the final one. */
+ memmove(&stream->buffers[gap_index],
+ &stream->buffers[gap_index + 1],
+ sizeof(stream->buffers[gap_index]) * stream->forwarded_buffers);
+ stream->buffers[zap_index] = InvalidBuffer;
+
+ /*
+ * If some of them are in the overflow zone then we also need to shift
+ * the copies that begin at index 0 and zap their trailing element
+ * too.
+ */
+ if (zap_index >= stream->queue_size)
+ {
+ int16 copies = zap_index - stream->queue_size;
+
+ memmove(&stream->buffers[0], &stream->buffers[1], copies);
+ stream->buffers[copies] = InvalidBuffer;
+ }
+ }
+
+ /*
+ * If there is per-buffer data, we also need to relocate one element from
+ * the "gap" to the "new" index, to keep it in sync with the buffer index.
+ */
+ if (per_buffer_data_size > 0)
+ {
+ void *src = get_per_buffer_data(stream, gap_index);
+ void *dst = get_per_buffer_data(stream, new_index);
+
+ /*
+ * We don't know how much of the per-buffer data was filled in by the
+ * callback (that's a private matter between the callback and the
+ * consumer). We falsely claim that we know the source is entirely
+ * initialized to avoid Valgrind errors from memcpy(), and also mark
+ * the destination as defined, because it was previously unused and
+ * clobbered ("noaccess").
+ */
+ VALGRIND_MAKE_MEM_DEFINED(src, per_buffer_data_size);
+ VALGRIND_MAKE_MEM_DEFINED(dst, per_buffer_data_size);
+ memcpy(dst, src, per_buffer_data_size);
+
+ /*
+ * We also have per-buffer data for every block of the current pending
+ * read, filled in by the callback while it was being built up.
+ */
+ per_buffer_data_count = stream->pending_read_nblocks;
+
+ /*
+ * We might also have one extra object beyond that, if we had to
+ * "unget" a block that couldn't be combined.
+ */
+ if (stream->buffered_blocknum != InvalidBlockNumber)
+ per_buffer_data_count++;
+
+ Assert(forwarded_buffers < stream->io_combine_limit);
+ Assert(forwarded_buffers <= per_buffer_data_count);
+ }
+
+ /* Are there additional per-buffer objects to shift? */
+ if (per_buffer_data_count > 0)
+ {
+ int16 remaining = per_buffer_data_count;
+ int16 contiguous;
+ void *src;
+ void *dst;
+
+ /*
+ * This first memmove() should often shift all of them. (See note
+ * above about Valgrind vs partial initialization.)
+ */
+ contiguous = Min(remaining, stream->queue_size - (gap_index + 1));
+ dst = get_per_buffer_data(stream, gap_index);
+ src = get_per_buffer_data(stream, gap_index + 1);
+ VALGRIND_MAKE_MEM_DEFINED(src, contiguous * per_buffer_data_size);
+ memmove(dst, src, contiguous * per_buffer_data_size);
+ remaining -= contiguous;
+
+ /*
+ * If it doesn't, then we need to deal with wraparound. Start by
+ * rotating one object from the physical start to the physical end.
+ */
+ if (remaining > 0)
+ {
+ dst = get_per_buffer_data(stream, stream->queue_size - 1);
+ src = get_per_buffer_data(stream, 0);
+ VALGRIND_MAKE_MEM_DEFINED(src, per_buffer_data_size);
+ memcpy(dst, src, per_buffer_data_size);
+ remaining -= 1;
+
+ /*
+ * If any are left after that, they must be contiguous at the
+ * physical start of the queue, so shift them too.
+ */
+ if (remaining > 0)
+ {
+ dst = get_per_buffer_data(stream, 0);
+ src = get_per_buffer_data(stream, 1);
+ VALGRIND_MAKE_MEM_DEFINED(src, remaining * per_buffer_data_size);
+ memmove(dst, src, remaining * per_buffer_data_size);
+ }
+ }
+ }
+
+ /* Clobber the final per-buffer-data object. */
+ if (per_buffer_data_size > 0)
+ {
+ int16 zap_index = gap_index;
+
+ read_stream_advance_buffer_n(stream, &zap_index, per_buffer_data_count);
+ clobber_per_buffer_data(stream, zap_index);
+ }
+}
+
/*
* Start as much of the current pending read as we can. If we have to split it
* because of the per-backend buffer limit, or the buffer manager decides to
@@ -450,9 +624,21 @@ read_stream_start_pending_read(ReadStream *stream)
sizeof(stream->buffers[0]) * overflow);
}
- /* Move to the location of start of next read. */
- read_stream_advance_buffer_n(stream, &buffer_index, nblocks);
- stream->next_buffer_index = buffer_index;
+ if (stream->out_of_order_enabled &&
+ stream->ios_in_progress > 0 &&
+ !need_wait &&
+ nblocks == 1)
+ {
+ /* Promote cached block to LIFO order, jumping in front of IOs. */
+ read_stream_reorder(stream);
+ }
+ else
+ {
+ /* Advance to the position of next read, maintaining FIFO order. */
+ read_stream_advance_buffer_n(stream,
+ &stream->next_buffer_index,
+ nblocks);
+ }
/* Adjust the pending read to cover the remaining portion, if any. */
stream->pending_read_blocknum += nblocks;
@@ -709,6 +895,9 @@ read_stream_begin_impl(int flags,
stream->advice_enabled = true;
#endif
+ if (flags & READ_STREAM_OUT_OF_ORDER)
+ stream->out_of_order_enabled = true;
+
/*
* Setting max_ios to zero disables AIO and advice-based pseudo AIO, but
* we still need to allocate space to combine and run one I/O. Bump it up
@@ -925,7 +1114,6 @@ read_stream_next_buffer(ReadStream *stream, void **per_buffer_data)
buffer = stream->buffers[oldest_buffer_index];
if (per_buffer_data)
*per_buffer_data = get_per_buffer_data(stream, oldest_buffer_index);
-
Assert(BufferIsValid(buffer));
/* Do we have to wait for an associated I/O first? */
@@ -971,8 +1159,6 @@ read_stream_next_buffer(ReadStream *stream, void **per_buffer_data)
stream->buffers[stream->queue_size + oldest_buffer_index] =
InvalidBuffer;
-#if defined(CLOBBER_FREED_MEMORY) || defined(USE_VALGRIND)
-
/*
* The caller will get access to the per-buffer data, until the next call.
* We wipe the one before, which is never occupied because queue_size
@@ -981,23 +1167,11 @@ read_stream_next_buffer(ReadStream *stream, void **per_buffer_data)
*/
if (stream->per_buffer_data)
{
- int16 index;
- void *per_buffer_data;
+ int16 index = oldest_buffer_index;
- index = oldest_buffer_index;
read_stream_retreat_buffer(stream, &index);
- per_buffer_data = get_per_buffer_data(stream, index);
-
-#if defined(CLOBBER_FREED_MEMORY)
- /* This also tells Valgrind the memory is "noaccess". */
- wipe_mem(per_buffer_data, stream->per_buffer_data_size);
-#elif defined(USE_VALGRIND)
- /* Tell it ourselves. */
- VALGRIND_MAKE_MEM_NOACCESS(per_buffer_data,
- stream->per_buffer_data_size);
-#endif
+ clobber_per_buffer_data(stream, index);
}
-#endif
/* Pin transferred to caller. */
Assert(stream->pinned_buffers > 0);
diff --git a/src/include/storage/read_stream.h b/src/include/storage/read_stream.h
index 9b0d65161d0..5af2f1d0ec7 100644
--- a/src/include/storage/read_stream.h
+++ b/src/include/storage/read_stream.h
@@ -63,6 +63,14 @@
*/
#define READ_STREAM_USE_BATCHING 0x08
+/*
+ * Blocks are usually streamed in FIFO order. This flag allows reordering,
+ * for consumers that can deal with out-of-order buffers. Whenever IOs are
+ * running, any already-cached buffers found in the look-ahead window jump
+ * directly to the front of the queue, ready to be consumed immediately.
+ */
+#define READ_STREAM_OUT_OF_ORDER 0x10
+
struct ReadStream;
typedef struct ReadStream ReadStream;
--
2.39.5
[text/x-patch] v2-0004-Use-READ_STREAM_OUT_OF_ORDER-for-Bitmap-Heap-Scan.patch (994B, ../../CA+hUKG+53X=26HwQrykdzwtt-j+jWGU3WRa8VEM7AFyyQK-Atg@mail.gmail.com/5-v2-0004-Use-READ_STREAM_OUT_OF_ORDER-for-Bitmap-Heap-Scan.patch)
download | inline diff:
From e458382a471831a759885a2dcdbe58422d2c365a Mon Sep 17 00:00:00 2001
From: Thomas Munro <[email protected]>
Date: Wed, 9 Apr 2025 16:10:33 +1200
Subject: [PATCH v2 4/4] Use READ_STREAM_OUT_OF_ORDER for Bitmap Heap Scan.
Bitmap Heap Scan can process blocks in any order, so we should
prioritize cached data while IOs are running.
---
src/backend/access/heap/heapam.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.c
index ed2e3021799..cd4f8967a27 100644
--- a/src/backend/access/heap/heapam.c
+++ b/src/backend/access/heap/heapam.c
@@ -1205,6 +1205,7 @@ heap_beginscan(Relation relation, Snapshot snapshot,
else if (scan->rs_base.rs_flags & SO_TYPE_BITMAPSCAN)
{
scan->rs_read_stream = read_stream_begin_relation(READ_STREAM_DEFAULT |
+ READ_STREAM_OUT_OF_ORDER |
READ_STREAM_USE_BATCHING,
scan->rs_strategy,
scan->rs_base.rs_rd,
--
2.39.5
^ permalink raw reply [nested|flat] 16+ messages in thread
* Re: Streaming relation data out of order
@ 2025-04-09 19:35 Andres Freund <[email protected]>
parent: Thomas Munro <[email protected]>
0 siblings, 1 reply; 16+ messages in thread
From: Andres Freund @ 2025-04-09 19:35 UTC (permalink / raw)
To: Thomas Munro <[email protected]>; +Cc: Robert Haas <[email protected]>; pgsql-hackers
Hi,
On 2025-04-09 19:17:00 +1200, Thomas Munro wrote:
> One weird issue, not just with reordering, is that read_stream.c's
> distance cools off too easily with some simple test patterns. Start
> at 1, double on miss, subtract one on hit, repeat, and you can be
> trapped alternating between 1 and 2 when you'd certainly benefit from
> IO concurrency and also reordering. It may need a longer memory.
Just having a hit in itself really is no reason to reduce the distance
IMO. It's completely normal to have a few hits when doing readahead, but a few
hits in a row doesn't mean the IO latency of the subsequent miss doesn't
matter anymore. But if our entire lookahead only is hits, we actually are in
a pattern where there no misses for the moment, and can reduce the distance.
Another issue is that we, afaict, overweigh hits rather substantially due to
IO combining. A single one block hit (hits are never bigger), decreases IO
distance. But a read of size N only affects distance once, not N times.
I.e. what I am suggesting is something like:
1) Increase distance by * 2 + read_size on a miss
This would help us to increase with distance = 1, where * 2 only increases
distance by 1, just as -1 obviously reduces it by 1.
It'd also somewhat address the over-weighing of hits compared to misses.
2) Only reduce distance if we have no outstanding IOs
> Subject: [PATCH v2 1/4] Move read stream modulo arithmetic into functions.
>
> Several places have open-coded circular index arithmetic. Make some
> common functions for better readability and consistent assertion
> checking. This avoids adding yet more open-coding in later patches.
> ---
> src/backend/storage/aio/read_stream.c | 93 +++++++++++++++++++++------
> 1 file changed, 74 insertions(+), 19 deletions(-)
>
> diff --git a/src/backend/storage/aio/read_stream.c b/src/backend/storage/aio/read_stream.c
> index 0e7f5557f5c..0d3dd65bfed 100644
> --- a/src/backend/storage/aio/read_stream.c
> +++ b/src/backend/storage/aio/read_stream.c
> @@ -216,6 +216,67 @@ read_stream_unget_block(ReadStream *stream, BlockNumber blocknum)
> stream->buffered_blocknum = blocknum;
> }
>
> +/*
> + * Increment buffer index, wrapping around at queue size.
> + */
> +static inline void
> +read_stream_advance_buffer(ReadStream *stream, int16 *index)
> +{
> + Assert(*index >= 0);
> + Assert(*index < stream->queue_size);
> +
> + *index += 1;
> + if (*index == stream->queue_size)
> + *index = 0;
> +}
> +/*
> + * Increment buffer index by n, wrapping around at queue size.
> + */
> +static inline void
> +read_stream_advance_buffer_n(ReadStream *stream, int16 *index, int16 n)
> +{
> + Assert(*index >= 0);
> + Assert(*index < stream->queue_size);
> + Assert(n <= stream->io_combine_limit);
> +
> + *index += n;
> + if (*index >= stream->queue_size)
> + *index -= stream->queue_size;
> +
> + Assert(*index >= 0);
> + Assert(*index < stream->queue_size);
> +}
Not sure if that's it's possible to have a large enough queue_size, but this
doesn't look like it's safe against *index + n > PG_INT16_MAX. The value would
wrap into the negative.
I think all these variables really ought to be unsigned. None of them ever
could be negative afaict. Afaict that would lead to the correct behaviour when
wapping around.
Greetings,
Andres Freund
^ permalink raw reply [nested|flat] 16+ messages in thread
* Re: Streaming relation data out of order
@ 2025-04-11 00:50 Thomas Munro <[email protected]>
parent: Andres Freund <[email protected]>
0 siblings, 0 replies; 16+ messages in thread
From: Thomas Munro @ 2025-04-11 00:50 UTC (permalink / raw)
To: Andres Freund <[email protected]>; +Cc: Robert Haas <[email protected]>; pgsql-hackers
On Thu, Apr 10, 2025 at 7:35 AM Andres Freund <[email protected]> wrote:
> 1) Increase distance by * 2 + read_size on a miss
>
> This would help us to increase with distance = 1, where * 2 only increases
> distance by 1, just as -1 obviously reduces it by 1.
>
> It'd also somewhat address the over-weighing of hits compared to misses.
>
> 2) Only reduce distance if we have no outstanding IOs
Interesting. That's similar in some ways to one of my own attempts:
I had an extra thing called sustain, and that had to be counted down
to zero before distance started decrementing. I didn't want the
distance itself to get higher than it already did by doubling once per
(combined) IO, I just wanted to defer the decay. (Names from an
earlier abandoned attempt to use synthesiser terminology: attack,
decay, sustain, release; cute but useless.) I tried a few different
recipes, but just adding read sizes to sustain works out pretty close
to your pleasingly simple rule 2: sustain until we can't see any IOs
in the lookahead window. But I also tried adding a bit more for
longer sustain, sustain += read_size * 2 or whatever, so you don't
give up your window so easily on a run of hits, and it also achieve
escape velocity from the gravity of 1. IDK, I hadn't formed any
strong opinions yet and need to test more stuff, hence lack of
concrete patches. Thanks, this was helpful, I'll play around with it!
> > +/*
> > + * Increment buffer index by n, wrapping around at queue size.
> > + */
> > +static inline void
> > +read_stream_advance_buffer_n(ReadStream *stream, int16 *index, int16 n)
> > +{
> > + Assert(*index >= 0);
> > + Assert(*index < stream->queue_size);
> > + Assert(n <= stream->io_combine_limit);
> > +
> > + *index += n;
> > + if (*index >= stream->queue_size)
> > + *index -= stream->queue_size;
> > +
> > + Assert(*index >= 0);
> > + Assert(*index < stream->queue_size);
> > +}
>
> Not sure if that's it's possible to have a large enough queue_size, but this
> doesn't look like it's safe against *index + n > PG_INT16_MAX. The value would
> wrap into the negative.
No, because the queue is sized such that indexes into this overflow
space also fit under INT16_MAX:
/*
* If starting a multi-block I/O near the end of the queue, we might
* temporarily need extra space for overflowing buffers before they are
* moved to regular circular position. This is the maximum extra space we
* could need.
*/
queue_overflow = io_combine_limit - 1;
The patch clearly lacks a comment to explain that mystery. Will add. Thanks!
> I think all these variables really ought to be unsigned. None of them ever
> could be negative afaict.
Maybe... I tend to default to signed variables when there isn't a good
reason to prefer unsigned. But I have also been mulling a change here
since:
commit 06fb5612c970b3af95aca3db5a955669b07537ca
Author: Thomas Munro <[email protected]>
Date: Wed Mar 19 15:23:12 2025 +1300
Increase io_combine_limit range to 1MB.
Unfortunately uint16 still needs to worry about UINT16_MAX
(effective_io_concurrency = 1000 * io_combine_limit = 128 = 128,000
buffers + a few, still one bit short; before the above commit the cap
was 32,000 buffers + a few, which is why int16 was enough when the
code was written). Maybe we should just go directly to int, or uint32
if you insist, and stop worrying about overflow. I think we agreed
that 128,000 buffers = 1000MB of in-progress I/O seemed unrealistic
and 32K ought to be enough for anybody™, but really we're talking
about saving a handful of bytes in struct ReadStream in exchange for a
risk of bugs. Will write a patch to try that soon...
^ permalink raw reply [nested|flat] 16+ messages in thread
end of thread, other threads:[~2025-04-11 00:50 UTC | newest]
Thread overview: 16+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2024-01-07 19:50 [PATCH v3 06/17] lazy_scan_prune reorder freeze execution logic Melanie Plageman <[email protected]>
2024-01-07 19:50 [PATCH v3 06/17] lazy_scan_prune reorder freeze execution logic Melanie Plageman <[email protected]>
2024-01-07 19:50 [PATCH v2 06/17] lazy_scan_prune reorder freeze execution logic Melanie Plageman <[email protected]>
2024-01-07 19:50 [PATCH 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-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 v9 07/21] lazy_scan_prune reorder freeze execution logic Melanie Plageman <[email protected]>
2024-03-25 23:39 [PATCH v7 07/16] lazy_scan_prune reorder freeze execution logic Melanie Plageman <[email protected]>
2024-03-25 23:39 [PATCH v9 07/21] lazy_scan_prune reorder freeze execution logic Melanie Plageman <[email protected]>
2025-04-09 07:17 Re: Streaming relation data out of order Thomas Munro <[email protected]>
2025-04-09 19:35 ` Re: Streaming relation data out of order Andres Freund <[email protected]>
2025-04-11 00:50 ` Re: Streaming relation data out of order Thomas Munro <[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