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

On Thu, Nov 20, 2025 at 6:16 AM 河田達也 <kawatatatsuya0913@gmail.com> wrote:
>
> Hi Sawada-san, Chao-san,
>
> Thank you very much for your helpful feedback and testing.
>
> > We can use vacrel->dead_items_info->max_bytes instead of calculating it again.
> I fixed it as you mentioned.
>
>
> > I meant (1) actually. I don't think the number of indexes attached to
> > the target relation is not relevant with the memory usage.
> >
> > We could add a new counter to track how many times we had to reset the
> > dead items storage because it was full. The existing num_index_scans
> > counter only shows how many times we performed index vacuuming. This
> > means that when index vacuuming is disabled, we still collect dead
> > items but num_index_scans shows 0. Adding this new counter would help
> > users understand how often the dead items storage reaches capacity,
> > even when index vacuuming is turned off.
> Thank you for your detailed explanation; I finally understand.
> I added a new counter to track the number of dead items storage resets caused by reaching capacity.
>
>
> > How about updating total bytes in dead_items_reset()?
> Thank you. I fixed it as you mentioned.
>
>
> > Another case we need to consider is where the table doesn't have an index...
> I fixed it as you mentioned.
>
>
> > Where “max_bytes” is "/* the maximum bytes TidStore can use */“, but I think we
> > actually want to show max memories ever consumed during vacuuming, right?
> Thank you for pointing out the confusing “max” wording.
> The word  "max” is not the peak memory *actually used* during VACUUM.
> It represents the upper limit of memory that the TidStore is allowed to use at once
> (derived from maintenance_work_mem or autovacuum_work_mem).
> So the current output may look misleading, especially when total usage is 0 but the limit is non-zero.
>
> To make this more explicit, I updated the message wording to avoid implying that
> this is the maximum consumption, and instead describe it as the configured memory limit.
> The updated style is:
> ```
> memory usage: total 1.02 MB used across 15 index scan(s) (max mem space is limited 0.06 MB at once)
> ```
> I believe this wording better reflects the actual meaning of max_bytes and avoids
> the interpretation that it is a peak usage value.

I think we should not use "index scans" since the number doesn't
actually represent the number of index scans performed. How about
something like:

memory usage: 1.02 MB in total, with dead-item storage reset 15 times
(limit was 0.06 MB)

Also when it comes to the plural, we can use errmsg_plural() instead.

>
>
> Regarding pgindent, running pgindent produced a large number of unrelated changes across the file,
> not only around the lines modified. Since it seemed inappropriate to include those unrelated diffs,
> I picked and applied only the necessary indentation changes for this patch.
>
> I apologize for not mentioning this in the v3 submission.
> I will revisit my pgindent setup to ensure it matches the project’s expected behavior.
>
> I have posted v4 incorporating all these changes.
> Thank you again for your guidance.
>

Thank you for updating the patch! Here are some minor comments for v4 patch:

+       dead_items_max_bytes = 0;
        /*
         * Get cutoffs that determine which deleted tuples are considered DEAD,
         * not just RECENTLY_DEAD, and which XIDs/MXIDs to freeze.
Then determine

We can initialize dead_items_max_bytes when declared.

---
+       /* Track total memory usage for dead_items */
+       if (vacrel->dead_items != NULL)
+       {
+               vacrel->total_dead_items_bytes +=
TidStoreMemoryUsage(vacrel->dead_items);
+       }

Does it need to check if vacrel->dead_items is non-NULL? If it could
be NULL, the following operations like TidStoreDestroy() should fail
anyway.

Regards,

--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com