Re: archive modules

On Sat, Jan 29, 2022 at 09:01:41PM -0800, Nathan Bossart wrote:
> On Sat, Jan 29, 2022 at 04:31:48PM -0800, Nathan Bossart wrote:
>> On Sat, Jan 29, 2022 at 12:50:18PM -0800, Nathan Bossart wrote:
>>> Here is a new revision.  I've moved basic_archive to contrib, hardened it
>>> as suggested, and added shutdown support for archive modules.
>> 
>> cfbot was unhappy with v14, so here's another attempt.  One other change I
>> am pondering is surrounding pgarch_MainLoop() with PG_TRY/PG_FINALLY so
>> that we can also call the shutdown callback in the event of an ERROR.  This
>> might be necessary for an archive module that uses background workers.
> 
> Ugh.  Apologies for the noise.  cfbot still isn't happy, so here's yet
> another attempt.  This new patch set also ensures the shutdown callback is
> called when the archiver process exits.

If basic_archive is to be in contrib, we probably want to avoid restarting
the archiver every time the module ERRORs.  I debated trying to add a
generic exception handler that all archive modules could use, but I suspect
many will have unique cleanup requirements.  Plus, AFAICT restarting the
archiver isn't terrible, it just causes most of the normal retry logic to
be skipped.

I also looked into rewriting basic_archive to avoid ERRORs and return false
for all failures, but this was rather tedious.  Instead, I just introduced
a custom exception handler for basic_archive's archive callback.  This
allows us to ERROR as necessary (which helps ensure that failures show up
in the server logs), and the archiver can treat it like a normal failure
and avoid restarting.

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