Re: pgsql: Add TAP tests for pg_verify_checksums

Hi,

On 2018-10-19 16:35:46 -0400, Stephen Frost wrote:
> The only justification for *not* doing this is that some extension
> author might have dumped files into our tablespace directory, something
> we've never claimed to support nor generally worried about in all the
> time that I can recall before this.

No, it's really not the only reason. As I said, testing is much less
likely to find cases where we're checksumming a short-lived file even
though we shouldn't, than making sure that checksummed files are
actually checksummed.

> > > Playing this further and assuming that extensions dropping files into
> > > tablespace directories is acceptable, what are we supposed to do when
> > > there's some direct conflict between a file that we want to create in a
> > > PG tablespace directory and a file that some extension dropped there?
> > > Create yet another subdirectory which we call
> > > "THIS_IS_REALLY_ONLY_FOR_PG"?
> >
> > Then it's a buggy extension. And we error out.
>
> Extensions writing into directories they shouldn't be makes them buggy
> even if the core code isn't likely to write to a particular filename,
> imv.

I'll just stop talking to you about this for now.

> > > What about two different extensions wanting to create files with the
> > > same names in the tablespace directories..?
> > >
> > > Experimentation is fine, of course, this is open source code and people
> > > should feel free to play with it, but we are not obligated to avoid
> > > breaking things when an extension author, through their experimentation,
> > > does something which is clearly not supported, like dropping files into
> > > PG's tablespace directories. Further, when it comes to our user's data,
> > > we should be taking a strict approach and accounting for everything,
> > > something that this whitelist-of-patterns-based approach to finding
> > > files to verify the checksums on doesn't do.
> >
> > It's not economically feasible to work on extensions that will only be
> > usable a year down the road.
>
> I listed out multiple other solutions to this problem which you
> summarily ignored.

Except that none of them really achieves the goals you can achieve by
having the files in the database (like DROP DATABASE working, for
starters).

> It's unfortunate that those other solutions weren't used and that,
> instead, this extension decided to drop files into PG's tablespace
> directories, but that doesn't mean we should condone or implicitly
> support that action.

This just seems pointlessly rigid. Our extension related APIs aren't
generally very good or well documented. Most non-trivial extensions that
add interesting things to the postgres ecosystem are going to need a few
not so pretty things to get going.

> I stand by my position that this patch should be reverted (with no
> offense or ill-will towards Michael, of course, I certainly appreciate
> his efforts to address the issues with pg_verify_checksums) and that we
> should move more of this code to identify files to verify the checksums
> on into libpgcommon and then use it from both pg_basebackup and
> pg_verify_checksums, to the extent possible, but that we make sure to
> account for all of the files in our tablespace and database directories.

What does that do, except break things that currently work? Sure, work
on a patch that fixes the architectural concerns, but what's the point
in reverting it until that's done?

> To the extent that this is an issue for extension authors, perhaps it
> would encourage them to work with us to have supported mechanisms
> instead of using hacks like dropping files into our tablespace
> directories and such instead of using another approach to manage files
> across multiple filesystems. I'd be kind of surprised if they really
> had an issue doing that and hopefully everyone would feel better about
> what these extensions are doing once they start using a mechanism that
> we actually support.

Haribabu has a patch, but it's on-top of pluggable storage, so not
exactly a small prerequisite.

Greetings,

Andres Freund