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
