Re: POC: enable logical decoding when wal_level = 'replica' without a server restart

Hi Sawada-San.

A couple of review questions/comments for patch v24.

======
src/backend/commands/publicationcmds.c

1.
- if (wal_level != WAL_LEVEL_LOGICAL)
+ if (!IsLogicalDecodingEnabled())
ereport(WARNING,
(errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
- errmsg("\"wal_level\" is insufficient to publish logical changes"),
- errhint("Set \"wal_level\" to \"logical\" before creating subscriptions.")));
+ errmsg("logical decoding must be enabled to publish logical changes"),
+ errhint("Before creating subscriptions, set \"wal_level\" >=
\"logical\" or create a logical replication slot when \"wal_level\" =
\"replica\".")));

The hint still says "or create a logical replication slot...", which I
thought I showed to be redundant info. It makes the user think that
they might have to do something when, AFAIK, they don't.

My suggestion is just to tweak the current master hin, like:
errhint("Set \"wal_level\" >= \"replica\" before creating subscriptions.")));

======
src/backend/replication/logical/logicalctl.c

DisableLogicalDecodingIfNecessary:

2.
+ /*
+ * Sanity check as we cannot disable logical decoding while holding a
+ * logical slot.
+ */
+ Assert(!MyReplicationSlot);

File header comment says, "While activation occurs synchronously right
after creating the first logical slot, deactivation happens
asynchronously through the checkpointer..."

Q. Is that new sanity-check Assert safe?

IOW, isn't it possible that some logical slot got created after the
checkpointer was requested to deactivate logical replication, but
before DisableLogicalDecodingIfNecessary() was called?

Or should that code be more like:

if (MyReplicationSlot)
return;

======
Kind Regards,
Peter Smith.
Fujitsu Australia