> On Dec 30, 2025, at 02:57, Masahiko Sawada <[email protected]> wrote:
>
> On Wed, Dec 24, 2025 at 1:07 AM Chao Li <[email protected]> wrote:
>>
>>
>>
>>> On Dec 23, 2025, at 21:20, Tatsuya Kawata <[email protected]>
>>> 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/
