Re: recovery modules

On Thu, Feb 09, 2023 at 12:18:55PM -0800, Andres Freund wrote:
> On 2023-02-09 11:39:17 -0800, Nathan Bossart wrote:
> Personally I'd probably squash these into a single commit.

done

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

done

>> + <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.

removed

>> + <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/?

done

>> <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?

I agree that this callback should typically not need the state, but I'm not
sure whether it fits into the assignment hook for archive_library. This
callback is primarily meant for situations when you have archiving enabled,
but your module isn't configured yet (e.g., archive_command is empty). In
this case, we keep the WAL around, but we don't try to archive it until
this hook returns true. It's up to each module to define that criteria. I
can imagine someone introducing a GUC in their archive module that
temporarily halts archiving via this callback. In that case, calling it
via a GUC assignment hook probably won't work. In fact, checking whether
archive_command is empty in that hook might not work either.

>> <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?

done

I left this out originally because the archiver exits shortly after calling
this. However, if you have DSM segments or something, it's probably wise
to make sure those are cleaned up. And I suppose we might not always exit
immediately after this callback, so establishing the habit of freeing the
state could be a good idea. In addition to updating this part of the docs,
I wrote a shutdown callback for basic_archive that frees its state.

--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com