> On Dec 23, 2025, at 07:24, Masahiko Sawada <[email protected]> wrote:
>
> On Sun, Dec 21, 2025 at 9:09 AM Tatsuya Kawata
> <[email protected]> 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 context to 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 is the 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 %.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:
```
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/
