Date: Fri, 28 Jan 2022 12:01:38 -0800
From: Nathan Bossart <nathandbossart@gmail.com>
To: Robert Haas <robertmhaas@gmail.com>
Cc: "Bossart, Nathan" <bossartn@amazon.com>,
	David Steele <david@pgmasters.net>,
	Fujii Masao <masao.fujii@oss.nttdata.com>,
	"pgsql-hackers@postgresql.org" <pgsql-hackers@postgresql.org>,
	Michael Paquier <michael@paquier.xyz>,
	Magnus Hagander <magnus@hagander.net>,
	Stephen Frost <sfrost@snowman.net>
Subject: Re: archive modules
Message-ID: <20220128200138.GA3398@nathanxps13>
References: <53a180a7-bf1b-38c5-7fb4-4088dc022b98@pgmasters.net>
 <A30D8D33-8944-4898-BCA8-C77C18258247@amazon.com>
 <4c01ce36-fbed-104a-ef9c-6046343dc845@pgmasters.net>
 <316CAF77-A0AC-49AE-A175-1C1ECF6C1981@amazon.com>
 <71E7A5D1-E418-4516-A21D-BD838BA759CE@amazon.com>
 <0CF39706-6952-49B1-987D-F012CDF5EEC4@amazon.com>
 <90D3CEE4-00C1-450C-80B1-88B69D9318DE@amazon.com>
 <3842CF04-528B-42A5-A12D-BA3970EF1E53@amazon.com>
 <5D92CC59-47C8-4463-9150-44033DDDD0D8@amazon.com>
 <CA+Tgmobow79kHwCa3hJ6oALk7wL6JmCZW9gkDEJw9udvPSVO9A@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: 
 <CA+Tgmobow79kHwCa3hJ6oALk7wL6JmCZW9gkDEJw9udvPSVO9A@mail.gmail.com>
Archived-At: 
 <https://www.postgresql.org/message-id/20220128200138.GA3398%40nathanxps13>
Precedence: bulk

On Fri, Jan 28, 2022 at 02:06:50PM -0500, Robert Haas wrote:
> I've committed 0001 now. I don't see anything particularly wrong with
> the rest of this either, but here are a few comments:

Thanks!

> - I wonder whether it might be better to promote the basic archiving
> module to contrib (as compared with src/test/modules) and try to
> harden it to the extent that such hardening is required. I think a lot
> of people would get good use out of that. It might not be a completely
> baked solution, but a solution doesn't have to be completely baked to
> be a massive improvement over the stupidity endorsed by our current
> documentation.

This has been suggested a few times in this thread, so I'll go ahead and
move it to contrib.  I am clearly outnumbered! :)

I discussed the two main deficiencies I'm aware of with basic_archive
earlier [0].  The first one is the issue with "incovenient" server crashes
(mentioned below).  The second is that there is no handling for multiple
servers writing to the same location since the temporary file is always
named "archtemp."  I thought about a few ways to pick a unique file name
(or at least one that is _probably_ unique), but that began adding a lot of
complexity for something I intended as a test module.  I can spend some
more time on this if you think it's worth fixing for a contrib module.

> - I wonder whether it's a good idea to silently succeed if the file
> exists and has the same contents as the file we're trying to archive.
> ISTR that being necessary behavior for robustness, because what if we
> archive the file and then die before recording the fact that we
> archived it?

Yes.  The only reason I didn't proceed with this earlier is because the
logic became a sizable chunk of the module.  I will add this in the next
revision.

> - If we load a new archive library, should we give the old one a
> callback so it can shut down? And should the archiver considering
> exiting since we can't unload? It isn't necessary but it might be
> nicer.

Good idea.  I'll look into this.

> - I believe we decided some time back to invoke function pointers
> (*like)(this) rather than like(this) for clarity. It was judged that
> something->like(this) was fine because that can only be a function
> pointer, so no need to write (*(something->like))(this), but
> like(this) could make you think that "like" is a plain function rather
> than a function pointer.

Will fix.

[0] https://postgr.es/m/A30D8D33-8944-4898-BCA8-C77C18258247%40amazon.com

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com/