Re: recovery modules

Thanks for reviewing.

On Thu, Feb 16, 2023 at 11:29:56AM -0800, Andres Freund wrote:
> On 2023-02-15 10:44:07 -0800, Nathan Bossart wrote:
>> @@ -144,10 +170,12 @@ basic_archive_configured(void)
>>   * Archives one file.
>>   */
>>  static bool
>> -basic_archive_file(const char *file, const char *path)
>> +basic_archive_file(ArchiveModuleState *state, const char *file, const char *path)
>>  {
>>  	sigjmp_buf	local_sigjmp_buf;
> 
> Not related the things changed here, but this should never have been pushed
> down into individual archive modules. There's absolutely no way that we're
> going to keep this up2date and working correctly in random archive
> modules. And it would break if archive modules are ever called outside of
> pgarch.c.

Yeah.  IIRC I did briefly try to avoid this, but the difficulty was that
each module will have its own custom cleanup logic.  There's no requirement
that a module creates an exception handler, but I imagine that any
sufficiently complex one will.  In any case, I agree that it's worth trying
to pull this out of the individual modules.

>> +static void
>> +basic_archive_shutdown(ArchiveModuleState *state)
>> +{
>> +	BasicArchiveData *data = (BasicArchiveData *) (state->private_data);
> 
> The parens around (state->private_data) are imo odd.

Oops, removed.

>> +	basic_archive_context = data->context;
>> +	Assert(CurrentMemoryContext != basic_archive_context);
>> +
>> +	if (MemoryContextIsValid(basic_archive_context))
>> +		MemoryContextDelete(basic_archive_context);
> 
> I guess I'd personally be paranoid and clean data->context after
> this. Obviously doesn't matter right now, but at some later date it could be
> that we'd error out after this point, and re-entered the shutdown callback.

Done.

>> +/*
>> + * Archive module callbacks
>> + *
>> + * These callback functions should be defined by archive libraries and returned
>> + * via _PG_archive_module_init().  ArchiveFileCB is the only required callback.
>> + * For more information about the purpose of each callback, refer to the
>> + * archive modules documentation.
>> + */
>> +typedef void (*ArchiveStartupCB) (ArchiveModuleState *state);
>> +typedef bool (*ArchiveCheckConfiguredCB) (ArchiveModuleState *state);
>> +typedef bool (*ArchiveFileCB) (ArchiveModuleState *state, const char *file, const char *path);
>> +typedef void (*ArchiveShutdownCB) (ArchiveModuleState *state);
>> +
>> +typedef struct ArchiveModuleCallbacks
>> +{
>> +	ArchiveStartupCB startup_cb;
>> +	ArchiveCheckConfiguredCB check_configured_cb;
>> +	ArchiveFileCB archive_file_cb;
>> +	ArchiveShutdownCB shutdown_cb;
>> +} ArchiveModuleCallbacks;
> 
> If you wanted you could just define the callback types in the struct now, as
> we don't need asserts for the types.

This crossed my mind.  I thought it was nice to have a declaration for each
callback that we can copy into the docs, but I'm sure we could do without
it, too.

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