Re: [PATCH] Add memory usage reporting to VACUUM VERBOSE - Mailing list pgsql-hackers

> On Dec 23, 2025, at 07:24, Masahiko Sawada <sawada.mshk@gmail.com> wrote:
>
> On Sun, Dec 21, 2025 at 9:09 AM Tatsuya Kawata
> <kawatatatsuya0913@gmail.com> wrote:
>>
>> Hi Sawada-san,
>>
>> Thank you for your review on the v7 patch!
>> I have updated the patch based on your feedback.
>>
>>> IIUC this line aims to add the initial data related to TidStore including underlying radix tree and the bump
contextto the total memory usage. I'm not sure why we do that only when no dead items are collected. If we add these
sizes,should we do that also when the TidStore is reset due to being full and there are no dead items in the subsequent
blocks,no? I guess it would make more sense to consider adding TidStoreMemoryUsage() also before cleaning up the
TidStore.
>> You're right and my previous patch is not correct.
>> I adopted Chao's suggestion from the previous thread.
>>
>>> I think it's not correct that we say "memory allocated: 64.00MB" in this case because we don't actually allocate
64MB.Since we're using a TidStore for dead items storage, we incrementally allocate its space when needed. 
>> I agree. I reverted to your earlier suggested wording "limit was %.2f MB" which more accurately describes that this
isthe configured memory limit, not the actual allocated amount. The updated message format is: 
>> memory usage: 0.02 MB in total, with dead-item storage reset 0 times (limit was 64.00 MB)
>>
>
> Thank you for updating the 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.
>
> Regards,
>
> --
> Masahiko Sawada
> Amazon Web Services: https://aws.amazon.com
> <v9-0001-Add-dead-items-memory-usage-to-VACUUM-VERBOSE-and.patch>

My last nitpick on v9:

```
+            appendStringInfo(&buf,
+                             ngettext("memory usage: %.2f MB in total, with dead-item storage reset %d time (limit was
%.2fMB)\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:

```
            if (vacrel->do_index_vacuuming)
            {
                if (vacrel->nindexes == 0 || vacrel->num_index_scans == 0)
                    appendStringInfoString(&buf, _("index scan not needed: "));
                else
                    appendStringInfoString(&buf, _("index scan needed: "));

                msgfmt = _("%u pages from table (%.2f%% of total) had %" PRId64 " dead item identifiers removed\n");
            }
            else
            {
                if (!VacuumFailsafeActive)
                    appendStringInfoString(&buf, _("index scan bypassed: "));
                else
                    appendStringInfoString(&buf, _("index scan bypassed by failsafe: "));

                msgfmt = _("%u pages from table (%.2f%% of total) have %" PRId64 " dead item identifiers\n");
            }

```

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