Re: recovery modules

From: Andres Freund <andres(at)anarazel(dot)de>
To: Michael Paquier <michael(at)paquier(dot)xyz>
Cc: Nathan Bossart <nathandbossart(at)gmail(dot)com>, pgsql-hackers(at)postgresql(dot)org
Subject: Re: recovery modules
Date: 2023-01-30 19:48:10
Message-ID: 20230130194810.6fztfgbn32e7qarj@awork3.anarazel.de
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

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

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Andres Freund 2023-01-30 19:49:37 Re: postgres_fdw uninterruptible during connection establishment / ProcSignalBarrier
Previous Message Andres Freund 2023-01-30 19:43:50 Making background psql nicer to use in tap tests