On Thu, 26 Nov 2020 at 00:55, Fujii Masao <masao.fu...@oss.nttdata.com> wrote:
>
> + * A heap scan need not return tuples for the
> last page it has
> + * scanned. To ensure that heap_blks_scanned
> is equivalent to
> + * total_heap_blks after the table scan
> phase, this parameter
> + * is manually updated to the correct value
> when the table scan
> + * finishes.
>
> So it's better to update this comment a bit? For example,
>
> If the scanned last pages are empty, it's possible to go to the next
> phase
> while heap_blks_scanned != heap_blks_total. To ensure that they are
> equivalet after the table scan phase, this parameter is manually updated
> to the correct value when the table scan finishes.
>
PFA a patch with updated message and comment. I've reworded the
messages to specifically mention empty pages and the need for setting
heap_blks_scanned = total_heap_blks explicitly.
Feel free to update the specifics, other than that I think this is a
complete fix for the issues at hand.
Regards,
Matthias van de Meent
From 66af670588b88b7fb4eb78aa6251609d10651e6e Mon Sep 17 00:00:00 2001
From: Matthias van de Meent <boekew...@gmail.com>
Date: Fri, 20 Nov 2020 16:23:59 +0100
Subject: [PATCH v3] Fix CLUSTER progress reporting of number of blocks
scanned.
The heapScan need not start at block 0, so heapScan->rs_cblock need not be the
correct value for amount of blocks scanned. A more correct value is
((heapScan->rs_cblock - heapScan->rs_startblock + heapScan->rs_nblocks) %
heapScan->rs_nblocks), as it accounts for the wraparound and the initial
offset of the heapScan.
Additionally, if the last pages of the scan were empty, the heap scan does
not return tuples from the last scanned page. This means that when
table_scan_getnextslot returns false, indicating the end of the table scan, we
must manually update the heap_blks_scanned parameter to the number of blocks
in the heap scan to ensure that heap_blks_scanned == heap_blks_total when
starting the next phase.
---
src/backend/access/heap/heapam_handler.c | 29 ++++++++++++++++++++++--
1 file changed, 27 insertions(+), 2 deletions(-)
diff --git a/src/backend/access/heap/heapam_handler.c b/src/backend/access/heap/heapam_handler.c
index dcaea7135f..720d92cac9 100644
--- a/src/backend/access/heap/heapam_handler.c
+++ b/src/backend/access/heap/heapam_handler.c
@@ -698,6 +698,7 @@ heapam_relation_copy_for_cluster(Relation OldHeap, Relation NewHeap,
Datum *values;
bool *isnull;
BufferHeapTupleTableSlot *hslot;
+ BlockNumber prev_cblock = InvalidBlockNumber;
/* Remember if it's a system catalog */
is_system_catalog = IsSystemRelation(OldHeap);
@@ -793,14 +794,38 @@ heapam_relation_copy_for_cluster(Relation OldHeap, Relation NewHeap,
else
{
if (!table_scan_getnextslot(tableScan, ForwardScanDirection, slot))
+ {
+ /*
+ * If the last pages of the scan were empty, we would go to
+ * the next phase while heap_blks_scanned != heap_blks_total.
+ * Instead, to ensure that heap_blks_scanned is equivalent to
+ * total_heap_blks after the table scan phase, this parameter
+ * is manually updated to the correct value when the table scan
+ * finishes.
+ */
+ pgstat_progress_update_param(PROGRESS_CLUSTER_HEAP_BLKS_SCANNED,
+ heapScan->rs_nblocks);
break;
+ }
/*
* In scan-and-sort mode and also VACUUM FULL, set heap blocks
* scanned
+ *
+ * Note that heapScan may start at an offset and wrap around, i.e.
+ * rs_startblock may be >0, and rs_cblock may end with a number
+ * below rs_startblock. To prevent showing this wraparound to the
+ * user, we offset rs_cblock by rs_startblock (modulo rs_nblocks).
*/
- pgstat_progress_update_param(PROGRESS_CLUSTER_HEAP_BLKS_SCANNED,
- heapScan->rs_cblock + 1);
+ if (prev_cblock != heapScan->rs_cblock)
+ {
+ pgstat_progress_update_param(PROGRESS_CLUSTER_HEAP_BLKS_SCANNED,
+ (heapScan->rs_cblock +
+ heapScan->rs_nblocks -
+ heapScan->rs_startblock
+ ) % heapScan->rs_nblocks + 1);
+ prev_cblock = heapScan->rs_cblock;
+ }
}
tuple = ExecFetchSlotHeapTuple(slot, false, NULL);
--
2.20.1
