Re: recovery modules

Hi,

On 2023-02-15 10:44:07 -0800, Nathan Bossart wrote:
> On Wed, Feb 15, 2023 at 03:38:21PM +0900, Michael Paquier wrote:
> > I may tweak a bit the comments, but nothing more.  And I don't think I
> > have more to add.  Andres, do you have anything you would like to
> > mention?

Just some minor comments below. None of them needs to be addressed.


> @@ -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.


> +static void
> +basic_archive_shutdown(ArchiveModuleState *state)
> +{
> +	BasicArchiveData *data = (BasicArchiveData *) (state->private_data);

The parens around (state->private_data) are imo odd.

> +	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.


> +
> +/*
> + * 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.

Greetings,

Andres Freund