At Mon, 17 Oct 2022 13:51:52 +0900, Michael Paquier <mich...@paquier.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
