Re: Reviewing freeze map code

On Mon, Jun 6, 2016 at 4:27 PM, Andres Freund <andres@anarazel.de> wrote:
> On 2016-06-06 16:18:19 -0400, Stephen Frost wrote:
>> That could be as simple as a query with the right things installed, or
>> it might be an independent tool, but not having any way to check isn't
>> good.  That said, trying to make VACUUM do that doesn't make sense to me
>> either.
>
> The point is that VACUUM *has* these types of checks. And had so for
> many years:
>                 else if (all_visible_according_to_vm && !PageIsAllVisible(page)
>                                  && VM_ALL_VISIBLE(onerel, blkno, &vmbuffer))
>                 {
>                         elog(WARNING, "page is not marked all-visible but visibility map bit is set in relation
\"%s\"page %u",
 
>                                  relname, blkno);
>                         visibilitymap_clear(onerel, blkno, vmbuffer);
>                 }
>                 ...
>                 else if (PageIsAllVisible(page) && has_dead_tuples)
>                 {
>                         elog(WARNING, "page containing dead tuples is marked as all-visible in relation \"%s\" page
%u",
>                                  relname, blkno);
>                         PageClearAllVisible(page);
>                         MarkBufferDirty(buf);
>                         visibilitymap_clear(onerel, blkno, vmbuffer);
>                 }
>
> the point is that, after the introduction of the freeze bit, there's no
> way to reach them anymore (and they're missing a useful extension of
> these warnings, but ...); these warnings have caught bugs.  I don't
> think it'd advocate for the vacuum option otherwise.

So a couple of things:

1. I think it is pretty misleading to say that those checks aren't
reachable any more.  It's not like we freeze every page when we mark
it all-visible.  In most cases, I think that what will happen is that
the page will be marked all-visible and then, because it is
all-visible, skipped by subsequent vacuums, so that it doesn't get
marked all-frozen until a few hundred million transactions later.  Of
course there will be some cases when a page gets marked all-visible
and all-frozen at the same time, but I don't see why we should expect
that to be the norm.

2. With the new pg_visibility extension, you can actually check the
same thing that first warning checks like this:

select * from pg_visibility('t1'::regclass) where all_visible and not
pd_all_visible;

IMHO, that's a substantial improvement over running VACUUM and
checking whether it spits out a WARNING.

The second one, you can't currently trigger for all-frozen pages.  The
query I just sent in my other email could perhaps be adapted to that
purpose, but maybe this is a good-enough reason to add VACUUM
(even_frozen_pages).

3. If you think there are analogous checks that I should add for the
frozen case, or that you want to add yourself, please say what they
are specifically.  I *did* think about it when I wrote that code and I
didn't see how to make it work.  If I had, I would have added them.
The whole point of review here is, hopefully, to illuminate what
should have been done differently - if I'd known how to do it better,
I would have done so.  Provide an idea, or better yet, provide a
patch.  If you see how to do it, coding it up shouldn't be the hard
part.

Thanks,

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company