Hi Sawada-san, Thank you very much for your detailed review and suggestions for the v2 patch. Also, thank you for pointing out the top-posting issue. I'll follow the mailing list style from now on.
---
>+ /* Report memory usage for dead_items tracking */
>+ vac_work_mem = AmAutoVacuumWorkerProcess() &&
>+ autovacuum_work_mem != -1 ?
>+ autovacuum_work_mem : maintenance_work_mem;
>
>We can use vacrel->dead_items_info->max_bytes instead of calculating it
again.
I changed it to retrieve the value from max_bytes.
However, I encountered a segmentation fault during parallel VACUUM, so I
modified the code to store the value before resetting it.
I would appreciate any suggestions if there is a better approach.
---
>How about the following message style?
>
>memory usage: total 0.69 MB used across 1 index scans (max 64.00 MB at
once)
I would like to confirm one point:
Does “1 index scans” refer to (1) the number of reset cycles during the
VACUUM process, or (2) the number of indexes attached to the target
relation?
In my understanding, the latter seems more useful to users, so in the v3
patch I implemented it as the number of vacuumed indexes.
If this interpretation is not correct, I will adjust it accordingly.
---
>+/*
>+ * Add current memory usage to the running total before resetting.
>+ * This tracks cumulative memory across all index vacuum cycles.
>+ */
>+ if (vacrel->dead_items != NULL)
>+ {
>+ Size final_bytes = TidStoreMemoryUsage(vacrel->dead_items);
>+
>+ vacrel->total_dead_items_bytes += final_bytes;
>+ }
>
>The comments need to be indented. I'd recommend running
>src/tools/pgindent/pgindent to format the codes before creating the
>patch.
>
>The above change doesn't cover some cases where index vacuuming is
>disabled (see the first if statement in the same function). It happens
>for example when the user specified INDEX_CLEANUP=off, we used the
>bypassing index vacuuming optimization, or failsafe was dynamically
>triggered. The proposed change covers the bypassing optimization but
>doesn't for the other two cases, which seems inconsistent and needs to
>be avoided. Another case we need to consider is where the table
>doesn't have an index, where we don't collect dead items to the
>TidStore. In this case, we always report that 0 bytes are used. Do we
>want to report the memory usage also in this case?
As for tracking memory usage, I separated the internal accounting from what
is ultimately reported to the user,
and therefore the implementation is slightly redundant, recording the
memory usage at several decision points.
For the display behavior:
Even with INDEX_CLEANUP = off, or when do_index_vacuuming = false due to
failsafe, memory usage may not be strictly zero.
Therefore, I believe it is still appropriate to report the memory usage in
these cases.
On the other hand, when the relation has no indexes at all, no memory is
allocated for dead_items, so printing a memory usage line seems unnecessary.
In the v3 patch, I adjusted the code so that no message is emitted in this
specific case.
I also removed the unintended blank lines and ran pgindent as you suggested.
I appreciate your continued feedback, and I would be grateful for another
review of the v3 patch.
Best regards,
Tatsuya Kawata
v3-0001-Add-memory-usage-reporting-to-VACUUM-VERBOSE.patch
Description: Binary data
