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