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

On Wed, Dec 11, 2024 at 2:18 PM Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote:
>
> On Thu, Dec 5, 2024 at 4:32 PM Melanie Plageman
> <melanieplageman(at)gmail(dot)com> wrote:
> >
> > On Mon, Dec 2, 2024 at 9:28 AM Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> > >
> > >
> > > All that said, if you really want this broken out into three
> > > categories rather than two, I'm not overwhelmingly opposed to that. It
> > > is always possible that more detail could be useful; it is just
> > > difficult to decide where the likelihood is high enough to justify
> > > adding more output.
>
> I agree that it will be unimportant from Melanie's work in this area.
> Also, I agree that if semi-aggressive vacuum has its own new logging
> message about what it's done, this new message doesn't necessarily
> need to be detailed. But looking at the proposed patches, there seems
> to be no such new logging message so far. Showing three categories
> makes sense to me independent of semi-aggressive vacuum work. If we
> figure out it's better to merge some parts of this new message to
> semi-aggressive vacuum's logs, we can adjust them later.

Yes, actually after some review feedback on the eager vacuum patch set
from Andres, I switched my approach and these counters are no longer a
dependency. (I still require the second patch in this set).

However, through this discussion, I've come to think that the VM
logging is useful. As such I plan to commit attached v5 (which
addresses review feedback and incorporates Tomas' wording suggestion).

> > 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.
>
> The output makes sense to me. The patch mostly looks good to me. Here
> is one minor comment:
>
> if ((old_vmbits & VISIBILITYMAP_ALL_VISIBLE) == 0 &&
> (old_vmbits & VISIBILITYMAP_ALL_FROZEN) == 0)
>
> Maybe it would be better to rewrite it to:
>
> if ((old_vmbits & (VISIBILITYMAP_ALL_VISIBLE | VISIBILITYMAP_ALL_FROZEN)) == 0)

Thanks for noticing. Actually Bilal pointed out downthread that I
actually don't need to check if the old_vmbits were set neither
all-visible nor all-frozen in the first branch -- if they were not
all-visible then they would not have been all-frozen (barring some VM
corruption -- but then we would have bigger problems).

I've changed the code accordingly.

- Melanie