Re: recovery modules

Hi,

On 2023-01-30 16:51:38 +0900, Michael Paquier wrote:
> On Fri, Jan 27, 2023 at 10:27:29PM -0800, Nathan Bossart wrote:
> > Here is a work-in-progress patch set for adjusting the archive modules
> > interface.  Is this roughly what you had in mind?
> 
> I have been catching up with what is happening here.  I can get
> behind the idea to use the term "callbacks" vs "context" for clarity,
> to move the callback definitions into their own header, and to add
> extra arguments to the callback functions for some private data.
> 
> -void
> -_PG_archive_module_init(ArchiveModuleCallbacks *cb)
> +const ArchiveModuleCallbacks *
> +_PG_archive_module_init(void **arg)
>  {
>     AssertVariableIsOfType(&_PG_archive_module_init, ArchiveModuleInit);
>  
> -   cb->check_configured_cb = basic_archive_configured;
> -   cb->archive_file_cb = basic_archive_file;
> +   (*arg) = (void *) AllocSetContextCreate(TopMemoryContext,
> +                                           "basic_archive",
> +                                           ALLOCSET_DEFAULT_SIZES);
> +
> +   return &basic_archive_callbacks;

> Now, I find this part, where we use a double pointer to allow the
> module initialization to create and give back a private area, rather
> confusing, and I think that this could be bug-prone, as well.

I don't think _PG_archive_module_init() should actually allocate a memory
context and do other similar initializations. Instead it should just return
'const ArchiveModuleCallbacks*', typically a single line.

Allocations etc should happen in one of the callbacks. That way we can
actually have multiple instances of a module.


>  Once
> you incorporate some data within the set of callbacks, isn't this
> stuff close to a "state" data, or just something that we could call
> only an "ArchiveModule"?  Could it make more sense to have
> _PG_archive_module_init return a structure with everything rather than
> a separate in/out argument?  Here is the idea, simply:
> typedef struct ArchiveModule {
> 	ArchiveCallbacks *routines;
> 	void *private_data;
> 	/* Potentially more here, like some flags? */
> } ArchiveModule;

I don't like this much. This still basically ends up with the module callbacks
not being sufficient to instantiate an archive module.

Greetings,

Andres Freund