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. 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;
That would be closer to the style of xlogreader.h, for example.
All these choices should be documented in archive_module.h, at the
end.
--
Michael