Re: Replication slot drop message is sent after pgstats shutdown.

On Mon, Dec 13, 2021 at 12:11 PM Kyotaro Horiguchi
<horikyota(dot)ntt(at)gmail(dot)com> wrote:
>
> At Fri, 10 Dec 2021 18:13:31 +0900, Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote in
> > I agreed with Andres and Horiguchi-san and attached an updated patch.
>
> Thanks for the new version.
>
> It seems fine, but I have some comments from a cosmetic viewpoint.
>
> + /*
> + * Register callback to make sure cleanup and releasing the replication
> + * slot on exit.
> + */
> + ReplicationSlotInitialize();
>
> Actually all the function does is that but it looks slightly
> inconsistent with the function name. I think we can call it just
> "initialization" here. I think we don't need to change the function
> comment the same way but either will do for me.
>
> +ReplicationSlotBeforeShmemExit(int code, Datum arg)
>
> The name looks a bit too verbose. Doesn't just "ReplicationSlotShmemExit" work?
>
> - * so releasing here is fine. There's another cleanup in ProcKill()
> - * ensuring we'll correctly cleanup on FATAL errors as well.
> + * so releasing here is fine. There's another cleanup in
> + * ReplicationSlotBeforeShmemExit() callback ensuring we'll correctly
> + * cleanup on FATAL errors as well.
>
> I'd prefer "during before_shmem_exit()" than "in
> ReplicationSlotBeforeShmemExit() callback" here. (But the current
> wording is also fine by me.)

Thank you for the comments! Agreed with all comments.

I've attached an updated patch. Please review it.

> The attached detects that bug, but I'm not sure it's worth expending
> test time, or this might be in the server test suit.

Thanks. It's convenient to test this issue but I'm also not sure it's
worth adding to the test suit.

Regards,

--
Masahiko Sawada
EDB: https://www.enterprisedb.com/