On Fri, Oct 19, 2018 at 06:43:18PM -0400, Tom Lane wrote: > Andrew Dunstan <andrew.duns...@2ndquadrant.com> writes: >> I don't think just reverting it is really acceptable. > > +several. I do not mind somebody writing and installing a better fix. > I do object to turning the buildfarm red again.
I did not expect this thread to turn into a war zone. Anyway, there are a couple of things I agree with on this thread: - I agree with Andres point here: https://postgr.es/m/20181019171747.4uithw2sjkt6m...@alap3.anarazel.de A blacklist is fundamentally more difficult to maintain as there are way more things added in a data folder which do not have data checksums than things which have checksums. So using a blacklist approach looks unmaintainable in the long term. Future patches related to enabling online checksum verification make me worry if we keep the code like that. I can also easily imagine that anybody willing to use the pluggable storage API would like to put new files in tablespace-related data folders, relying on "base/" being the default system tablespace - I agree with Stephen's point that we should decide if a file has checksums or not in a single place, and that we should use the same logic for base backups and pg_verify_checksums. - I agree with not doing a simple revert to not turn the buildfarm red again. This is annoying for animal maintainers. Andrew has done a very nice work in disabling manually those tests temporarily. - The base backup logic deciding if a file has checksums looks broken to me: it misses files generated by EXEC_BACKEND, and any instance of Postgres using an extension with custom files and data checksums has its backups broken. cstore_fdw has been mentioned above, and I recall that Heroku for example enables data checksums. If you combine both, it basically means that such an instance cannot take base backups anymore while it was able to do so with pre-10 with default options. That's not cool. So what I think we ought to do is the following: - Start a new thread, this one about TAP tests is not adapted. - Add in src/common/relpath.c the API from d55241af called isRelFileName(), make use of it in the base backup code, and basically remove is_checksummed_file() and the checksum skip list. -- Michael
signature.asc
Description: PGP signature
