Re: Vacuum statistics

Hi Andrey, thank you for taking a look and for the review!

On 15.03.2026 20:18, Andrey Borodin wrote:
>> On 13 Mar 2026, at 18:04, Alena Rybakina<lena.ribackina@yandex.ru> wrote:
> I've decided to take a look into v31.
>
> Overall idea of tracking VM dynamics seems good to me.
Thank you!
> But the column naming for rev_all_visible_pages and rev_all_frozen_pages
> seems strange to me. I've skimmed the thread but could not figure out what
> "rev_" stands for. Revisions? Revolutions? Reviews?

We meant "revision", but after looking at our documentation I realized
the confusion - the term is not explained there.

I've renamed them to visible_pages_vm_cleared and frozen_pages_vm_cleared.

Does this naming make more sense?

> Is there a reason why you break "SELECT * FROM pg_stat_all_tables" for
> an existing software? IMO even if we want these columns in this exact view
> - they ought to be appended to the end of the column list.
No reason, I fixed this. Thanks for pointing it out.
> Some nits about the code.
>
> my $interval   = 0.015;
> sleep($interval); <--- sleep takes integer AFAIK?
>
> Maybe just use poll_query_until()?
>
> $start_time seems unused.
>
> I don't think src/test/recovery/t/ is good for the test. It has nothing to
> do with recovery. Maybe somewhere in src/test/modules?

I reconsidered the test and moved it to the regression tests (at the end 
of vacuum.sql).

With pg_stat_force_next_flush() they seem stable enough without using 
waiting functions.

> This change is not needed at all:
> - proargtypes => 'oid8 int8', prosrc => 'hashoid8extended' },
> + proargtypes => 'oid8 int8', prosrc => 'hashoid8extended' },
>
> s/'statistics: number of times the all-visible pages in the visibility map
> was removed for pages of table'/'statistics: number of times the all-visible
> pages in the visibility map were cleared for pages of this table'/g
>
> I would appreciate some braces in
> if (map[mapByte] >> mapOffset & flags & VISIBILITYMAP_ALL_VISIBLE)
> Probably the code is correct, but I write in languages with different parsers
> and do not trust in grammar priorities. Is it something like following?
> if (map[mapByte] & ((VISIBILITYMAP_ALL_VISIBLE & flags) << mapOffset))
> We check (map[mapByte] & mask) in this if statement which is flags << mapOffset btw...
Fixed.
> That's all what catches my eye this time. Thank you!

Thank you for the review!

-----------
Best regards,
Alena Rybakina