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