Re: pgsql: Add TAP tests for pg_verify_checksums - Mailing list pgsql-hackers

Greetings,

* Andres Freund (andres@anarazel.de) wrote:
> On 2018-10-19 15:57:28 -0400, Stephen Frost wrote:
> > Perhaps it doesn't today but surely one goal of pg_verify_checksum is to
> > be able to run it on a running cluster eventually.
>
> I was saying *precisely* that above.  I give up.

I'm glad we agree on that (I thought we did before, in fact, so it was
odd to see it coming up again).

> > > pg_basebackup's case is *not* really comparable because basebackup.c
> > > already performed a lot of filtering before noChecksumFiles is applied.
> >
> > This all really just points out that we should have the code for
> > handling this somewhere common that both pg_verify_checksum and
> > pg_basebackup can leverage without duplicating all of it.
>
> I never argued against that.  My point is that your argument that they
> started out the same isn't true.

I overstated the relationship, clearly, but it hardly matters, as you
say..

> > The specific case that started all of this certainly looks pretty
> > clearly like a case that both need to deal with.
>
> Yep.

... and it's in the part of the code that was actually copied and was
the same.

> > > FWIW, as far as I can tell, pg_verify_checksum appears to be broken in a
> > > lot of (unfortunately) pretty normal scenarios right now. Not just on
> > > windows. Besides the narrow window of crashing while a .tmp file is
> > > present (and then shutting down normally after a crash restart), it also
> > > has the much of wider window of crashing while *any* backend has
> > > temporary files/relations.  As crash-restarts don't release temp files,
> > > they'll still be present after the crash, and a single graceful shutdown
> > > afterwards will leave them in place. No?
> >
> > We do go through and do some cleanup at crash/restart
>
> We don't clean up temp files tho.
>
>  * NOTE: we could, but don't, call this during a post-backend-crash restart
>  * cycle.  The argument for not doing it is that someone might want to examine
>  * the temp files for debugging purposes.  This does however mean that
>  * OpenTemporaryFile had better allow for collision with an existing temp
>  * file name.

Then, yes, we should go through and fix pg_verify_checksums to work
correctly in this case, likely using more-or-less the same code that
pg_basebackup has.  After that, we can add the remaining code to check
the last checkpoint and skip pages which have a more recent LSN...

> > As you say, some of those are likely to have checksums, which should be
> > handled by pg_basebackup and pg_verify_checksums, and that goes back to
> > the point I was making up-thread that we want to make sure an account
> > for everything.  With this pattern-based approach, we could easily end
> > up forgetting to add the correct new pattern into pg_verify_checksums.
>
> If you're adding checksums for something, you better test it I don't buy
> this.  In contrast it's much more likely that there's a file that's
> short-lived that you won't easily test against pg_verify_checksum
> running in that moment.

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.

As for saying that someone is obviously going to use pg_verify_checksums
to check their checksum code- I simply don't agree that we should be
happy to rely on that assumption when the only reason for any of this is
that some extension decided to do something that wasn't supported and
likely has issues in other parts of the code anyway (pg_basebackup would
happily copy those files too, even though there's obviously no code for
making sure it's a consistent copy or any such in pg_basebackup or
core).

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

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

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.

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.

Thanks!

Stephen