Re: Reviewing freeze map code

On 2016-06-06 14:24:14 -0400, Stephen Frost wrote:
> * Robert Haas (robertmhaas(at)gmail(dot)com) wrote:
> > On Mon, Jun 6, 2016 at 11:44 AM, Andres Freund <andres(at)anarazel(dot)de> wrote:
> > >> In terms of diagnostic tools, you can get the VM bits and
> > >> page-level bits using the pg_visibility extension; I wrote it
> > >> precisely because of concerns like the ones you raise here. If you
> > >> want to cross-check the page-level bits against the tuple-level bits,
> > >> you can do that with the pageinspect extension. And if you do those
> > >> things, you can actually find out whether stuff is broken.
> > >
> > > That's WAY out ouf reach of any "normal users". Adding a vacuum option
> > > is doable, writing complex queries is not.
> >
> > Why would they have to write the complex query? Wouldn't they just
> > need to run that we wrote for them?

Then write that query. Verify that that query performs halfway
reasonably fast. Document that it should be run against databases after
subjecting them to tests. That'd address my concern as well.

> > I mean, I'm not 100% dead set against this option you want, but in all
> > honestly, I would never, ever tell anyone to use it. Unleashing
> > VACUUM on possibly-damaged data is just asking it to decide to prune
> > away tuples you don't want gone. I would try very hard to come up
> > with something to give that user that was only going to *read* the
> > possibly-damaged data with as little chance of modifying or erasing it
> > as possible.

I'm more concerned about actually being able to verify that the freeze
logic does actually something meaningful, in situation where we'd *NOT*
expect any problems. If we're not trusting vacuum in that situation,
well ...

> I certainly agree with this.
>
> We need a read-only utility which checks that the system is in a correct
> and valid state. There are a few of those which have been built for
> different pieces, I believe, and we really should have one for the
> visibility map, but I don't think it makes sense to imply in any way
> that VACUUM can or should be used for that.

Meh. This is vacuum behaviour that *has existed* up to this point. You
essentially removed it. Sure, I'm all for adding a verification
tool. But that's just pie in the skie at this point. We have a complex,
data loss threatening feature, which just about nobody can verify at
this point. That's crazy.

Greetings,

Andres Freund