Hi Tatsuya-san, I just reviewed and tested v6, and got a major concern and a few small comments.
> On Nov 27, 2025, at 22:59, 河田達也 <[email protected]> wrote: > > Hi Sawada-san, > I rebased and applied the changes you suggested. > The updated v6 is attached. > Best regards, > Tatsuya Kawata > <v6-0001-Add-memory-usage-reporting-to-VACUUM-VERBOSE.patch> 1. Major concern: In this patch, you only increase vacrel->total_dead_items_bytes in dead_items_reset(), however, dead_items_reset() is not always called, that way I often see usage is 0. We should also increate the counter in heap_vacuum_rel() where you now get dead_items_max_bytes. My dirty fix is like below: ``` diff --git a/src/backend/access/heap/vacuumlazy.c b/src/backend/access/heap/vacuumlazy.c index ef2c72edf5d..635e3cf8346 100644 --- a/src/backend/access/heap/vacuumlazy.c +++ b/src/backend/access/heap/vacuumlazy.c @@ -862,6 +862,8 @@ heap_vacuum_rel(Relation rel, const VacuumParams params, */ dead_items_max_bytes = vacrel->dead_items_info->max_bytes; + vacrel->total_dead_items_bytes += TidStoreMemoryUsage(vacrel->dead_items); + /* * Free resources managed by dead_items_alloc. This ends parallel mode in * passing when necessary. ``` I verified the fix with a simple procedure: ``` drop table if exists vacuum_test; create table vacuum_test (id int); insert into vacuum_test select generate_series(1, 100000); delete from vacuum_test WHERE id % 2 = 0; vacuum (verbose) vacuum_test; ``` Without my fix, memory usage was reported as 0: ``` memory usage: 0.00 MB in total, with dead-item storage reset 0 times (limit was 64.00 MB) ``` And with my fix, memory usage is greater than 0: ``` memory usage: 0.02 MB in total, with dead-item storage reset 0 times (limit was 64.00 MB) ``` 2 ``` + * Save dead items max_bytes before cleanup for reporting the memory usage + * as the dead_items_info is freed in parallel vacuum cases during + * cleanup. + */ + dead_items_max_bytes = vacrel->dead_items_info->max_bytes; ``` The comment says "the dead_items_info is freed in parallel vacuum”, should we check if vacrel->dead_items_info != NULL before deferencing max_bytes? 3 ``` + appendStringInfo(&buf, + ngettext("memory usage: %.2f MB in total, with dead-item storage reset %d time (limit was %.2f MB)\n", + "memory usage: %.2f MB in total, with dead-item storage reset %d times (limit was %.2f MB)\n", ``` I just feel “limit was xxx MB” is still not clear enough. How about be explicit, like: Memory usage: 0.2 MB in total, memory allocated across 0 dead-item storage resets in total: 64.00 MB Or Memory usage: 0.2 MB in total, with dead-item storage reset %d time, total allocated: 64.00 MB Best regards, -- Chao Li (Evan) HighGo Software Co., Ltd. https://www.highgo.com/
