Re: Reviewing freeze map code

Hi,

On 2016-06-06 17:00:19 -0400, Robert Haas wrote:
> 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.

True. What I mean is that you can't force the checks (and some that I
think should be added) to occur anymore. Once a page is frozen it'll be
kinda hard to predict whether vacuum touches it (due to the skip logic).

> 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;

Right, but not the second.

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

I think it's a mixed bag. I do think that WARNINGS are a lot easier to
understand for a casual user/tester; rather than having to write/copy
queries which return results where you don't know what the expected
result is. I agree that it's better to have that in a non-modifying way
- although I'm afraid atm it's not really possible to do a
HeapTupleSatisfies* without modifications :(.

> 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.

I think it's pretty important (and not hard) to add a check for
(all_frozen_according_to_vm && has_unfrozen_tuples). Checking for
VM_ALL_FROZEN && !VM_ALL_VISIBLE looks worthwhile as well, especially as
we could check that always, without a measurable overhead. But the
former primarily makes sense if we have a way to force the check to
occur in a way that's not dependant on the state of neighbouring pages.

Greetings,

Andres Freund