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

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

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.

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center