Re: recovery modules

Hi,

On 2023-02-09 11:39:17 -0800, Nathan Bossart wrote:
> rebased for cfbot

I think this nearly ready. Michael, are you planning to commit this?

Personally I'd probably squash these into a single commit.

> diff --git a/doc/src/sgml/archive-modules.sgml b/doc/src/sgml/archive-modules.sgml
> index ef02051f7f..2db1b19216 100644
> --- a/doc/src/sgml/archive-modules.sgml
> +++ b/doc/src/sgml/archive-modules.sgml
> @@ -47,23 +47,30 @@
> normal library search path is used to locate the library. To provide the
> required archive module callbacks and to indicate that the library is
> actually an archive module, it needs to provide a function named
> - <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 an
> + <structname>ArchiveModuleCallbacks</structname> struct filled with the
> + callback function pointers for individual actions.

I'd probably mention that this should typically be of server lifetime / a
'static const' struct. Tableam documents this as follows:

The result of the function must be a pointer to a struct of type
<structname>TableAmRoutine</structname>, which contains everything that the
core code needs to know to make use of the table access method. The return
value needs to be of server lifetime, which is typically achieved by
defining it as a <literal>static const</literal> variable in global
scope

> +
> + <note>
> + <para>
> + <varname>archive_library</varname> is only loaded in the archiver process.
> + </para>
> + </note>
> </sect1>

That's not really related to any of the changes here, right?

I'm not sure it's a good idea to document that. We e.g. probably should allow
the library to check that the configuration is correct, at postmaster start,
rather than later, at runtime.

> <sect1 id="archive-module-callbacks">
> @@ -73,6 +80,20 @@ typedef void (*ArchiveModuleInit) (struct ArchiveModuleCallbacks *cb);
> The server will call them as required to process each individual WAL file.
> </para>
>
> + <sect2 id="archive-module-startup">
> + <title>Startup Callback</title>
> + <para>
> + 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 to have a state, it
> + can use <structfield>state->private_data</structfield> to store it.

s/needs to have a state/has state/?

> @@ -83,7 +104,7 @@ typedef void (*ArchiveModuleInit) (struct ArchiveModuleCallbacks *cb);
> assumes the module is configured.
>
> <programlisting>
> -typedef bool (*ArchiveCheckConfiguredCB) (void);
> +typedef bool (*ArchiveCheckConfiguredCB) (ArchiveModuleState *state);
> </programlisting>
>
> If <literal>true</literal> is returned, the server will proceed with

Hm. I wonder if ArchiveCheckConfiguredCB() should actually work without the
state. We're not really doing anything yet, at that point, so it shouldn't
really need state?

The reason I'm wondering is that I think we should consider calling this from
the GUC assignment hook, at least in postmaster. Which would make it more
convenient to not have state, I think?

> @@ -128,7 +149,7 @@ typedef bool (*ArchiveFileCB) (const char *file, const char *path);
> these situations.
>
> <programlisting>
> -typedef void (*ArchiveShutdownCB) (void);
> +typedef void (*ArchiveShutdownCB) (ArchiveModuleState *state);
> </programlisting>
> </para>
> </sect2>

Perhaps mention that this needs to free state it allocated in the
ArchiveModuleState, or it'll be leaked?

Greetings,

Andres Freund