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

> On Dec 30, 2025, at 02:57, Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote:
>
> On Wed, Dec 24, 2025 at 1:07 AM Chao Li <li(dot)evan(dot)chao(at)gmail(dot)com> wrote:
>>
>>
>>
>>> On Dec 23, 2025, at 21:20, Tatsuya Kawata <kawatatatsuya0913(at)gmail(dot)com> wrote:
>>>
>>> Hi Sawada-san and Chao-san,
>>>
>>> Thank you both for your continued reviews and feedback on this patch.
>>>
>>>> The patch mostly looks good to me. I've made some cosmetic changes to
>>>> the comments (as well as the commit message) and attached the updated
>>>> patch. Please review it.
>>> Thank you. I have no objections.
>>>
>>>
>>>> My last nitpick on v9:
>>>>
>>>> ```
>>>> + 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",
>>>> ```
>>>>
>>>> Instead of “dead-item”, I would suggest “dead item” (without dash), because in the existing code use “dead item”, for example:
>>>>
>>>> ```
>>> I've addressed your feedback.
>>>
>>> Attached is v10 with these changes incorporated.
>>> I really appreciate reviewing this patch throughout the iterations.
>>>
>>> Regards,
>>> Tatsuya Kawata
>>> <v10-0001-Add-dead-items-memory-usage-to-VACUUM-VERBOSE-an.patch>
>>
>> V10 looks good to me.
>>
>
> Thank you for reviewing the patch!
>
> After thinking about the verbose log message, I think we can improve
> the verbose message to clarify the memory usage report more. For
> example, if users get the message like:
>
> memory usage: 102.77 MB in total, with dead item storage reset 520
> times (limit was 0.12 MB)
>
> They might confuse that vacuum used 102.77 MB memory in total in spite
> of the limit being 0.12 MB. So how about rewording the message to?
>
> memory usage: allocated 102.77 MB total, 520 dead item storage resets
> (limit 0.12 MB each)
>
> The rest of the changes look good to me.
>
> Regards,
>
> --
> Masahiko Sawada
> Amazon Web Services: https://aws.amazon.com

Thanks for continuing to polish the patch. I spent some more time testing it as well.

I ran a test with: "SET maintenance_work_mem = '512kB’;”

Then I created a table with a large amount of data, deleted most of it to generate dead tuples spread across many pages, and finally ran: "VACUUM (VERBOSE, ANALYZE) vac_test;”

The output included:

```
memory usage: 94.50 MB in total, with dead item storage reset 126 times (limit was 0.50 MB)
```

In this output:

* “limit was 0.50 MB” is clear, since maintenance_work_mem was set to 512 KB.
* “with dead item storage reset 126 times” is also clear and useful.
* “94.50 MB in total” is the part I feel ambiguous without reading the code.

With a 512 KB limit, the dead item storage never exceeded 512 KB at any point. The reported “total” comes from summing TidStoreMemoryUsage() at each dead-item reset plus the final snapshot. This means the same memory is reused and counted repeatedly, so the number does not represent total allocation or actual memory consumption. In that sense, “total” easily reads as “allocated”, which is misleading.

Specifically, the patch does:
```
total_dead_items_bytes += TidStoreMemoryUsage(vacrel->dead_items);
```

TidStoreMemoryUsage() returns the current snapshot of the TidStore’s memory footprint, and this value is accumulated on every dead-item reset. So total_dead_items_bytes is really an accumulation of TidStore sizes across resets, not total memory usage in the usual sense. From this perspective, “memory usage” also feels a bit too broad.

Given that, I’d suggest wording along these lines:
```
dead item storage: accumulated 94.50 MB across 126 resets (limit 0.50 MB)
```

This seems to reflect more precisely what the counter is measuring while keeping the information useful to users.

Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/