Re: Count and log pages set all-frozen by vacuum

Hi,

Thank you for working on this!

On Fri, 6 Dec 2024 at 03:32, Melanie Plageman <melanieplageman(at)gmail(dot)com> wrote:
>
> Here's an example to exercise the new log message:
>
> create table foo (a int, b int) with (autovacuum_enabled = false);
> insert into foo select generate_series(1,1000), 1;
> delete from foo where a > 500;
> vacuum (verbose) foo;
> -- visibility map: 5 pages newly set all-visible, of which 2 set
> all-frozen. 0 all-visible pages newly set all-frozen.
> insert into foo select generate_series(1,500), 1;
> vacuum (verbose, freeze) foo;
> --visibility map: 3 pages newly set all-visible, of which 3 set
> all-frozen. 2 all-visible pages newly set all-frozen.

0001 and 0002 LGTM.

I have a small comment about 0003:

+ /*
+ * If the page wasn't already set all-visible and all-frozen in
+ * the VM, count it as newly set for logging.
+ */
+ if ((old_vmbits & VISIBILITYMAP_ALL_VISIBLE) == 0 &&
+ (old_vmbits & VISIBILITYMAP_ALL_FROZEN) == 0)
+ {
+ vacrel->vm_new_visible_pages++;
+ vacrel->vm_new_visible_frozen_pages++;
+ }

+ /*
+ * If the page wasn't already set all-visible and all-frozen in the
+ * VM, count it as newly set for logging.
+ */
+ if ((old_vmbits & VISIBILITYMAP_ALL_VISIBLE) == 0 &&
+ (old_vmbits & VISIBILITYMAP_ALL_FROZEN) == 0)
+ {
+ vacrel->vm_new_visible_pages++;
+ if (presult.all_frozen)
+ vacrel->vm_new_visible_frozen_pages++;
+ }

I think there is no need to check VISIBILITYMAP_ALL_FROZEN in these if
conditions. If the page is not all-visible; it can not be all-frozen,
right?

--
Regards,
Nazir Bilal Yavuz
Microsoft