Re: pgsql: Add TAP tests for pg_verify_checksums

Greetings,

* Andres Freund (andres(at)anarazel(dot)de) wrote:
> On 2018-10-19 10:36:59 -0400, Stephen Frost wrote:
> > * Michael Paquier (michael(at)paquier(dot)xyz) wrote:
> > > On Wed, Oct 17, 2018 at 05:30:05PM -0400, Andrew Dunstan wrote:
> > > > Fine by me.
> > >
> > > Thanks. This is now committed after some tweaks to the comments, a bit
> > > earlier than I thought first.
> >
> > I just saw this committed and I'm trying to figure out why we are
> > creating yet-another-list when it comes to deciding what should be
> > checksum'd and what shouldn't be.
>
> I'm not sure it's fair to blame Michael here. He's picking up slack,
> because the original authors of the tool didn't even bother to reply to
> the issue for quite a while. pg_verify_checksum was obviously never
> tested on windows, it just didn't have tests showing that.

I wasn't really going for 'blame' here, just that we've now got two
different methods for doing the same thing- and those will give
different answers; presumably this issue impacts pg_basebackup too,
since the logic there wasn't changed.

> > I also categorically disagree with the notion that it's ok for
> > extensions to dump files into our tablespace diretories or that we
> > should try to work around random code dumping extra files in the
> > directories which we maintain- it's not like this additional code will
> > actually protect us from that, after all, and it's foolish to think we
> > really could protect against that.
>
> I'm unconvinced. There already are extensions doing so, and it's not
> like we've given them any sort of reasonable alternative. You can't just
> create relations in a "registered" / "supported" kinda way right now, so
> uh, yea, extension gotta make do. And that has worked for many years.

I don't agree that any of this is an argument for accepting random code
writing into arbitrary places in the data directory or tablespace
directories as being something which we support or which we write code
to work-around.

If this is a capability that extension authors need then I'm all for
having a way for them to have that capability in a clearly defined and
documented way- and one which we could actually write code to handle and
work with.

> Also, as made obvious here, it's pretty clear that there's platform
> differences about which files exists and which don't, so it's not that a
> blacklist approach automatically is more maintainable.

Platform differences are certainly something we can manage and we have
code in a few different places for doing exactly that. A platform
difference is a heck of a lot more reasonable than trying to guess at
what random code that we've never seen before has done and try to work
around that.

> And I fail to see why a blacklist is architecturally better. There's
> plenty cases where we might want to create temporary files,
> non-checksummed data or such that we'd need a teach a blacklist about,
> but there's far fewer cases where add new checksummed files. Basically
> never since checksums have been introduced. And if checksums were
> introduced for something new, say slrus, we'd ceertainly use
> pg_verify_checksum during development.

In cases where we need something temporary, we're going to need to be
prepared to clean those up and we had better make it very plain that
they're temporary and easy to write code for. Anything we aren't
prepared to blow away on a crash-restart should be checksum'd and in an
ideal world, we'd be looking to reduce the blacklist to only those
things which are temporary. Of course, we may need different code for
calculating the checksum of different types of files, which moves it
from really being a 'blacklist' to a list of file-types and their
associated checksum'ing mechansim.

Thanks!

Stephen