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

Hi Chao-san,

Thank you very much for your testing and review!

> 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:

I believe the key point is how to display memory usage when index vacuum is
skipped.
In the v6 implementation, memory usage was always shown as 0 when there
were no indexes.
However, more accurately, memory is allocated during initialization even
when index vacuum is not executed, so it seems appropriate to display
memory consumption as you suggested.
To avoid double-counting memory usage, I've modified the location and
conditions where this processing is added.

> 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?

In the case of parallel vacuum, the reference to dead_items_info is
released in the subsequent dead_items_cleanup() call, so it cannot be NULL
at this point.
I've updated the comment to make it clearer which function call causes the
memory to become inaccessible.

> 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

I've revised it as follows. What do you think?
memory usage: 0.02 MB in total, with dead-item storage reset 0 times
(memory allocated: 64.00 MB)

I applied the changes above.
The updated v7 is attached.
I look forward to your feedback.

Best regards,
Tatsuya Kawata