Re: recovery modules

On Tue, Jan 31, 2023 at 03:30:13PM -0800, Nathan Bossart wrote:
> Okay, here is a new patch set with the aforementioned adjustments and
> documentation updates.

So, it looks like you have addressed the feedback received here, as
of:
- Rename of Context to Callback.
- Move of the definition into their own header.
- Introduction of a callback for the startup initialization.
- Pass down a private state to each callback.

I have a few minor comments.

+/*
+ * Since the logic for archiving via a shell command is in the core server
+ * and does not need to be loaded via a shared library, it has a special
+ * initialization function.
+ */
+extern const ArchiveModuleCallbacks *shell_archive_init(void);
Storing that in archive_module.h is not incorrect, still feels a bit
unnatural. I would have used a separate header for clarity. It may
not sound like a big deal, but we may want this separation if
archive_module.h is used in some frontend code in the future. Perhaps
that will never be the case, but I've seen many fancy (as in useful)
proposals in the past when it comes to such things.

static bool
-shell_archive_configured(void)
+shell_archive_configured(void *private_state)
{
return XLogArchiveCommand[0] != '\0';
Maybe check that in this context private_state should be NULL? The
other two callbacks could use an assert, as well.

- <function>_PG_archive_module_init</function>. This function is passed a
- struct that needs to be filled with the callback function pointers for
- individual actions.
+ <function>_PG_archive_module_init</function>. This function must return a
+ struct filled with the callback function pointers for individual actions.

Worth mentioning the name of the structure, as of "This function must
return a structure ArchiveModuleCallbacks filled with.."

+ The <function>startup_cb</function> callback is called shortly after the
+ module is loaded. This callback can be used to perform any additional
+ initialization required. If the archive module needs a state, it should
+ return a pointer to the state. This pointer will be passed to each of the
+ module's other callbacks via the <literal>void *private_state</literal>
+ argument.

Not sure about the complexity of two sentences here. This could
simply be:
This function can return a pointer to an area of memory dedicated to
the state of the archive module loaded. This pointer is passed to
each of the module's other callbacks as the argument
<literal>private_state</literal>.

Side note: it looks like there is nothing in archive-modules.sgml
telling that these modules are only loaded by the archiver process.
--
Michael