Re: recovery modules

On Mon, Feb 13, 2023 at 04:37:10PM +0900, Michael Paquier wrote:
> +   basic_archive_context = data->context;
> +   if (CurrentMemoryContext == basic_archive_context)
> +       MemoryContextSwitchTo(TopMemoryContext);
> +
> +   if (MemoryContextIsValid(basic_archive_context))
> +       MemoryContextDelete(basic_archive_context);
> 
> This is a bit confusing, because it means that we enter in the
> shutdown callback with one context, but exit it under
> TopMemoryContext.  Are you sure that this will be OK when there could
> be multiple callbacks piled up with before_shmem_exit()?  shmem_exit()
> has nothing specific to memory contexts.

Well, we can't free the memory context while we are in it, so we have to
switch to another one.  I agree that this is a bit confusing, though.

On second thought, I'm not sure it's important to make sure the state is
freed in the shutdown callback.  It's only called just before the archiver
process exits, so we're not really at risk of leaking anything.  I suppose
we might not always restart the archiver in this case, but I also don't
anticipate that behavior changing in the near future.  I think this
callback is more useful for things like shutting down background workers.

I went ahead and removed the shutdown callback from basic_archive and the
note about leaking from the documentation.

> Is putting the new headers in src/include/postmaster/ the best move in
> the long term?  Perhaps that's fine, but I was wondering whether a new
> location like archive/ would make more sense.  pg_arch.h being in the
> postmaster section is fine.

I moved them to archive/.

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