Re: [PATCH] Add memory usage reporting to VACUUM VERBOSE

(please avoid top-posting[1] on this mailing list)

On Tue, Nov 18, 2025 at 7:15 AM 河田達也 <kawatatatsuya0913(at)gmail(dot)com> wrote:
>
> Hi Sawada-san,
>
> Thank you very much for your review and helpful comments on my previous patch.
> I have revised the patch following your suggestions:
>
> ・Report total memory usage rather than peak memory, which is more meaningful for users.
> ・Removed the reset counter, since index scan counts already reflect memory resets.
> ・Updated the code so that memory usage is recorded at the proper points, avoiding unnecessary per-item checks.
> ・Removed the final peak memory check, as dead_items are already cleared at that point.
>
> All previous test cases have been re-run and verified to work correctly with both serial and parallel VACUUM scenarios.
> Please find the updated patch attached. I look forward to any further feedback.

Thank you for updating the patch!

Here are some review comments for the v2 patch:

+ /* Report memory usage for dead_items tracking */
+ vac_work_mem = AmAutoVacuumWorkerProcess() &&
+ autovacuum_work_mem != -1 ?
+ autovacuum_work_mem : maintenance_work_mem;

We can use vacrel->dead_items_info->max_bytes instead of calculating it again.

---
+ appendStringInfo(&buf,
+ _("total memory usage: %.2f MB of %.2f MB
allowed\n"),
+ (double) vacrel->total_dead_items_bytes /
(1024.0 * 1024.0),
+ (double) vac_work_mem / 1024.0
+ );

How about the following message style?

memory usage: total 0.69 MB used across 1 index scans (max 64.00 MB at once)

---
+/*
+ * Add current memory usage to the running total before resetting.
+ * This tracks cumulative memory across all index vacuum cycles.
+ */
+ if (vacrel->dead_items != NULL)
+ {
+ Size final_bytes = TidStoreMemoryUsage(vacrel->dead_items);
+
+ vacrel->total_dead_items_bytes += final_bytes;
+ }

The comments need to be indented. I'd recommend running
src/tools/pgindent/pgindent to format the codes before creating the
patch.

The above change doesn't cover some cases where index vacuuming is
disabled (see the first if statement in the same function). It happens
for example when the user specified INDEX_CLEANUP=off, we used the
bypassing index vacuuming optimization, or failsafe was dynamically
triggered. The proposed change covers the bypassing optimization but
doesn't for the other two cases, which seems inconsistent and needs to
be avoided. Another case we need to consider is where the table
doesn't have an index, where we don't collect dead items to the
TidStore. In this case, we always report that 0 bytes are used. Do we
want to report the memory usage also in this case?

---
@@ -3553,6 +3581,7 @@ dead_items_alloc(LVRelState *vacrel, int nworkers)
{
vacrel->dead_items = parallel_vacuum_get_dead_items(vacrel->pvs,

&vacrel->dead_items_info);
+
return;
}
}
@@ -3598,6 +3627,7 @@ dead_items_add(LVRelState *vacrel, BlockNumber
blkno, OffsetNumber *offsets,
static void
dead_items_reset(LVRelState *vacrel)
{
+
if (ParallelVacuumIsActive(vacrel))
{
parallel_vacuum_reset_dead_items(vacrel->pvs);

There are unnecessary line additions.

Regards,

[1] https://wiki.postgresql.org/wiki/Mailing_Lists#Mailing_List_Culture

--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com