Re: fix archive module shutdown callback

At Mon, 17 Oct 2022 13:51:52 +0900, Michael Paquier <michael(at)paquier(dot)xyz> wrote in
> 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

I guess that the "callback" there means the callback-caller function
(call_archive_module_shutdown_callback), which in turn is set as a
callback...

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

I thought that Bharath's point is to use before_shmem_exit() instead
of PG_ENSURE_ERROR_CLEANUP(). The place doesn't seem significant but
if we use before_shmem_exit(), it would be cleaner to place it
adjecent to on_sheme_exit() call.

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

Sure. What the shutdown callback can do differs by shared memory
access.

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

+1 for inserting DEBUG1.

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

I tend to agree to this.

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center