Re: fix archive module shutdown callback

On Sun, Oct 16, 2022 at 01:39:14PM +0530, Bharath Rupireddy wrote:
> Is the shutdown callback meant to be called only after the archive
> library is loaded? The documentation [1] says that it just gets called
> before the archiver process exits. If this is true, can we just place
> before_shmem_exit(call_archive_module_shutdown_callback, 0); in
> PgArchiverMain() after on_shmem_exit(pgarch_die, 0);?

I am not sure to understand what you mean here.  The shutdown callback
is available once the archiver process has loaded the library defined
in archive_library (assuming it is itself in shared_preload_libraries)
and you cannot call something that does not exist yet.  So, yes, you
could define the call to before_shmem_exit() a bit earlier because
that would be a no-op until the library is loaded, but at the end that
would be just registering a callback that would do nothing useful in a
larger window, aka until the library is loaded.

FWIW, I think that the documentation should clarify that the shutdown
callback is called before shmem exit.  That's important.

Another thing is that the shutdown callback is used by neither
shell_archive.c nor basic_archive.  We could do something about that,
actually, say by plugging an elog(DEBUG1) in shutdown_cb for
shell_archive.c to inform that the archiver is going down?
basic_archive could do that, but we already use shell_archive.c in a
bunch of tests, and this would need just log_min_messages=debug1 or
lower, so..

> Also, I've noticed other 3 threads and CF entries all related to
> 'archive modules' feature. IMO, it could be better to have all of them
> under a single thread and single CF entry to reduce
> reviewers/committers' efforts and seek more thoughts about all of the
> fixes.

I don't mind discussing each point separately as the first thread
dealing with archive modules is already very long, so the current way
of doing things makes sure to attract the correct type of attention
for each problem, IMO.
--
Michael