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

Hi Sawada-San.

FWIW, I also thought it was a good idea suggested by Bertrand [1] to
"hide" everything behind the slot create/delete, and thereby eliminate
the need for user intervention using those new
pg_activate/deactivate_logical_decoding functions.

But, one concern doing it this way is how to prevent a user
(accidentally?) getting themselves into a different replication mode
without realising it? Or say they change the mode and then "forget"
that they did that.

For example, If wal_level=replica and then somebody does CREATE
PUBLICATION/SUBSCRIPTION won't that implicitly enable the logical
decoding and then leave it enabled until all the logical replication
slots are eventually dropped? Now, when the user examines wal_level it
is still going to show 'replica', which could be misleading --- e.g.
how will they even know that they can't really trust that anymore, and
they must also check the pg_get_logical_decoding_status?

At least, there should be some big NOTICE/WARNING logs written
whenever the logical decoding status is getting implicitly changed by
logical replication slot create/delete..

~~~

Meanwhile, although it is premature to give detailed code review
comments for what is still a PoC, during my reading of your patch I
made copious notes for myself, so I thought I might as well post what
I have regardless. See the details below in this post. Ignore, or do
with them as you wish. But, hopefully, a lot of the following feedback
will still be relevant regardless of any design changes.

//////////

Commit message

1.
Patch description is missing

======
GENERAL

2. git apply warning

[postgres(at)CentOS7-x64 oss_postgres_misc]$ git apply
../patches_misc/PoC_online_activate_logical_decoding.patch
../patches_misc/PoC_online_activate_logical_decoding.patch:93: new
blank line at EOF.
+
warning: 1 line adds whitespace errors.

~~~

3. Copyright date

All the new files of this patch have the wrong copyright date (2024
instead of 2025).

~~~

4. Missing documentation

There are missing docs changes, but that is to be expected while the
design is in flux.

~~~

5. Terminology

There are multiple places where similar terms like "active" and
"enabled" and "ready" are being used apparently interchangeably making
it quite confusing. Perhaps it was your intention to convey the idea
that the logical mode might be "enabled" but is not yet "active", but
I didn't think that was the case. Mostly it just seemed arbitrary to
me.

IMO the state starting out as "disabled" should end up as "enabled"
(not "ready").

Meanwhile, the API to change the status is currently called
"activate/deactivate" instead of "enable/disable" (??)

Meanwhile, many/most of the error messages talk about "enabled" (not
"activated" or "ready"). e.g. "logical decoding needs to be enabled to
publish logical changes"

Meanwhile, macros are called 'IsLogicalDecodingActive'; why isn't that
called 'IsLogicalDecodingEnabled'.

~

OTOH, maybe you chose the term "active" because master already had the
macro XLogLogicalInfoActive(). But in that case maybe it should be
saying inactive/active everywhere instead of disabled/enabled/ready...

~~~

6. Typedefs

There are some new typedefs introduced in the new files, So these
should be updated in the typedefs.list file.

~~~

7. Tri-state logical decoding status

Currently there is a tri-state logical decoding status: "disabled" ->
"xlog-logicalinfo" -> "ready"

AFAICT that middle state is just transitory. It is easier for me to
visualize it like:
logical decoding status: "disabled" -> "pending" -> "enabled".

IIUC the "pending" part is when it is just waiting for any running
transaction to complete.

IMO it becomes simpler if you do not make any distinction between WAL
logging and logical decoding ability. E.g. Do not allow logical WAL
logging for that 'middle' transitory state; only allow it after
logical decoding becomes fully activated.

IIUC, it would just a small change to do this:

CURRENT (pseudocode):
XLogLogicalInfoEnabled(void) { return XLogLogicalInfoCtl->status >=
LOGICAL_DECODING_STATUS_XLOG_LOGICALINFO; }

SUGGESTION (pseudocode)
XLogLogicalInfoEnabled(void) { return XLogLogicalInfoCtl->status ==
LOGICAL_DECODING_STATUS_READY; }

======
contrib/test_decoding/t/002_online_activation.pl

8.
+# Copyright (c) 2021-2024, PostgreSQL Global Development Group

/2024/2025/

~~~

9.
+# check if a logical replication slot cannot be craeted while logical

/check/Check/

/cannot/can/

/craeted/created/

~~~

10.
+ok( $stderr =~ /ERROR: logical decoding requires "wal_level" >= "logical"/,
+ "logical replication slot cannot be created while logical
decoding is disableed");

/disableed/disabled/

~~~

11.
+# Activate logical info logging.
+$node->safe_psql('postgres',
+ 'SELECT pg_activate_logical_decoding()');
+$ret = $node->safe_psql('postgres', 'SELECT pg_get_logical_decoding_status()');
+is($ret, 'ready', 'logical decoding gets activated');

Perhaps there should be another "show wal_level" done here to show
that even after logical decoding is enabled, the wal_level remains the
same.

~~~

12.
+$node->safe_psql(
+ 'postgres',
+ q[
+CREATE TABLE test (a int primary key, b int);
+INSERT INTO test values (1, 100), (2, 200);
+UPDATE test SET b = b + 10 WHERE a = 1;
+ ]);
+
+$ret = $node->safe_psql(
+ 'postgres',
+ q[SELECT data FROM
pg_logical_slot_get_changes('regression_slot1', NULL, NULL,
'include-xids', '0', 'skip-empty-xacts', '1');]
+ );
+is($ret, 'BEGIN
+table public.test: INSERT: a[integer]:1 b[integer]:100
+table public.test: INSERT: a[integer]:2 b[integer]:200
+COMMIT
+BEGIN
+table public.test: UPDATE: a[integer]:1 b[integer]:110
+COMMIT', 'logical decoding works fine');
+

There seems to be some comment missing for all this part. I expected
it to say something like "Check that logical decoding is working OK".

======
src/backend/access/heap/heapam.c

13.
- * This is only used in wal_level >= WAL_LEVEL_LOGICAL, and only for catalog
- * tuples.
+ * This is only used when XLogLogicalInfoActive() is true, and only for
+ * catalog tuples.

I felt it would be better to Assert this in the code, instead of just
mentioning it in the function comment.

======
src/backend/access/rmgrdesc/xlogdesc.c

14.
+ else if (info == XLOG_LOGICAL_DECODING_STATUS)
+ {
+ bool logical_decoding;
+
+ memcpy(&logical_decoding, rec, sizeof(bool));
+ appendStringInfoString(buf, logical_decoding ? "true" : "false");
+ }

Should there be some textual context for this bool value, like wal_level has?

SUGGESTION
appendStringInfoString(buf, "logical decoding %s", logical_decoding ?
"true" : "false");

~~~

15.
I noticed there are lots of other descriptions in this function that
are showing the value of 'wal_level' via the static function call:
get_wal_level_string(wal_level).

But, since the wal_level now might be out-of-step with the
logcal_decoding boolean value, shouldn't you be showing the logical
decoding state everywhere too?

e.g. you might have a combination like:
wal_level replica
logical decoding true

======
src/backend/access/transam/xlog.c

BootStrapXLOG:

16.
+ checkPoint.logicalDecoding = false;
checkPoint.wal_level = wal_level;

Shouldn't the default for 'logicalDecoding' be consistent with the
assigned wal_level? It seems strange if it is possible to have a
situation where wal_level = logical but simultaneously logicalDecoding
= false.

~~~

xlog_redo:

17.
+ else if (info == XLOG_LOGICAL_DECODING_STATUS)
+ {
+ bool enabled;

A less generic variable name like "logical_decoding_enabled" might be better.

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

CreatePublication:

18.
- if (wal_level != WAL_LEVEL_LOGICAL)
+ if (!XLogLogicalInfoActive())
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 needs to be enabled to publish logical changes"),
+ errhint("Set \"wal_level\" to \"logical\" or activate logical
decoding before creating subscriptions.")));

18a.
/needs to be enabled/must be enabled/

~

18b.
The hint seems less useful than it could be because it does not say
*how* the user can activate logical decoding.

SUGGESTION
Set \"wal_level\" to \"logical\" or use pg_activate_logical_decoding
before creating subscriptions.

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

CheckLogicalDecodingRequirements:

19.
ereport(ERROR,
(errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
- errmsg("logical decoding on standby requires \"wal_level\" >=
\"logical\" on the primary")));
+ errmsg("logical decoding on standby requires to enable logical
decoding on the primary")));
19a
/requires to enable logical decoding/requires logical decoding to be enabled/

~

19b.
This message is like what was in CreatePublication, so might be better
split into an errmsg and errhint.

e.g.
errmsg("logical decoding on standby requires logical decoding to be
enabled on the primary");
errhint("Set \"wal_level\" to \"logical\" or use
pg_activate_logical_decoding.");

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

20. General

Would it be better if all the exposed functions from here used the
common "LogicalXLog" prefix?

These ones do:
- LogicalXlogShmemSize
- LogicalXlogShmemInit

These ones don't:
- StartupLogicalDecodingStatus
- UpdateLogicalDecodingStatus
- XLogLogicalInfoEnabled (this one is backwards)
- IsLogicalDecodingActive

~~~

21.
+ * logicalxlog.c
+ * This module contains the codes for enabling or disabling to include
+ * logical information into WAL records and logical decoding.

/codes/code/

Also, this is worded as if you can set those states independently, but
AFAICT the "logical information into WAL" is just a transition-state
to the LOGICAL_DECODING_STATUS_READY.

SUGGESTION
This module contains code to enable/disable and fetch the logical
decoding status.

~~~

22.
+ * Copyright (c) 2012-2024, PostgreSQL Global Development Group

/2024/2025/

~~~

23.
+XLogLogicalInfoCtlData *XLogLogicalInfoCtl = NULL;

Add blank line above this.

~~~

LogicalXlogShmemSize:

24.
+/*
+ * Initialization of shared memory.
+ */
+
+Size
+LogicalXlogShmemSize(void)
+{
+ return sizeof(XLogLogicalInfoCtlData);
+}

Unnecessary blank line after the function comment.

~~~

LogicalXlogShmemInit:

25.
+void
+LogicalXlogShmemInit(void)

Missing function comment.

~~~

StartupLogicalDecodingStatus:

26.
+/*
+ * This must be called ONCE during postmaster or standalone-backend startup,
+ * before calling StartupReplicationSlots().
+ */
+void
+StartupLogicalDecodingStatus(bool enabled_at_last_checkpoint)

The function comment does not actually say what this function does.
Maybe add a first sentence like:
"Assign the initial LogicalDecodingStatus value."

~~~

27.
+ /*
+ * On standbys, we always start with the status in the last checkpoint
+ * record. If changes of wal_level or logical decoding status is sent from
+ * the primary, we will enable or disable the logical decoding while
+ * replaying the WAL record and invalidate slots if necessary.
+ */

/is sent from/are sent from/

/invalidate/invalidating/

~~~

28.
+ if (!StandbyMode)
+ {
+ /*
+ * Disable logical decoding if replication slots are not available.
+ */
+ if (max_replication_slots == 0)
+ status = LOGICAL_DECODING_STATUS_DISABLED;
+
+ /*
+ * If previously the logical decoding was active but the server
+ * restarted with wal_level < 'replica', we disable the logical
+ * decoding.
+ */
+ if (wal_level < WAL_LEVEL_REPLICA)
+ status = LOGICAL_DECODING_STATUS_DISABLED;
+
+ /*
+ * Setting wal_level to 'logical' immediately enables logical decoding
+ * and WAL-logging logical info.
+ */
+ if (wal_level >= WAL_LEVEL_LOGICAL)
+ status = LOGICAL_DECODING_STATUS_READY;
+ }

28a.
The comment is a bit confusing because it is only talking about
Standby, whereas the code block is all for !StandbyMode.

It would be more easy to read if refactored to put the comment in an
empty code block like below:

if (StandByMode)
{
/*
* On standbys, we always start with the status in the last checkpoint
* record...
* ...
*/
}
else
{
<code>
}

~

28b.
What if the first condition (max_replication_slots is 0) and last
condition (WAL_LEVEL_LOGICAL) are true at the same time? Currently
this initializes the status as READY. Is it OK?

~~~

UpdateLogicalDecodingStatus:

29.
+void
+UpdateLogicalDecodingStatus(bool activate)
+{
+ XLogLogicalInfoCtl->status = activate
+ ? LOGICAL_DECODING_STATUS_READY
+ : LOGICAL_DECODING_STATUS_DISABLED;
+}

29a.
Missing function comment.

~

29b.
IMO the parameter should be in past tense, but I also think should be
called "enabled" (same as what it was called in the extern in
logicalxlog.h)

~~~

XLogLogicalInfoEnabled:

30.
+/*
+ * Is WAL-Logging logical info enabled?
+ */
+bool inline
+XLogLogicalInfoEnabled(void)
+{
+ /*
+ * Use volatile pointer to make sure we make a fresh read of the shared
+ * variable.
+ */
+ volatile XLogLogicalInfoCtlData *ctl = XLogLogicalInfoCtl;
+
+ return ctl->status >= LOGICAL_DECODING_STATUS_XLOG_LOGICALINFO;
+}

How necessary is this function?

I thought that LOGICAL_DECODING_STATUS_XLOG_LOGICALINFO is more like
just a transitory state between LOGICAL_DECODING_STATUS_DISABLED and
LOGICAL_DECODING_STATUS_READY. So in that case, isn't it simpler just
to wait for the READY? e.g. remove this function and just use
IsLogicalDecodingActive() instead?

~~~

do_activate_logical_decoding:

31.
Missing function comment.

~~~

32.
+ /*
+ * Get the latest status of logical info logging. If it's already
+ * activated, quick return.
+ */
+ SpinLockAcquire(&XLogLogicalInfoCtl->mutex);
+ if (XLogLogicalInfoCtl->status >= LOGICAL_DECODING_STATUS_XLOG_LOGICALINFO)
+ {
+ SpinLockRelease(&XLogLogicalInfoCtl->mutex);
+ return;
+ }

32a.
I didn't think the comment should mention "logical info logging" here.

SUGGESTION
Quick return if logical decoding is already activated (or activation
was already requested and is pending).

~

32b.
Related to this, it seems simpler to check condition "if
(XLogLogicalInfoCtl->status > LOGICAL_DECODING_STATUS_DISABLED)"

~~~

33.
+ PG_ENSURE_ERROR_CLEANUP(logical_decoding_activation_abort_callback, 0);
+ {
...
+ }
+ PG_END_ENSURE_ERROR_CLEANUP(logical_decoding_activation_abort_callback, 0);

IIUC, this whole chunk of code lies between the status of
"xlog-logicalinfo" and "ready".

I think it needs a comment something like:
/* Confirm that all in-progress transactions have completed before
logical decoding can be enabled. */

~~~

logical_decoding_activation_abort_callback:

34.
Missing function comment.

~~~

35.
Since this is a callback for the function
do_activate_logical_decoding(), I thought the function should be
slightly renamed to match the caller.

/logical_decoding_activation_abort_callback/activate_logical_decoding_abort_callback/

~~~

36.
IIUC, the status could have reached
LOGICAL_DECODING_STATUS_XLOG_LOGICALINFO before we encountered some
problem trying to get to the READY status, and so reset everything
back to DISABLED. So some (unusable?) logical WAL records could have
been written while in that intermediate state? It is another reason
why I was unsure about the need for the earlier XLogLogicalInfoEnabled
function.

Won't it be simpler to just have a DISABLED and ENABLED state and just
keep the transition phase for internal logic? IOW, make
XLogLogicalInfoEnabled() return true only when READY/ENABLED.

~~~
pg_activate_logical_decoding:

37.
Missing function comment.

~~~

38.
+ if (IsTransactionState() &&
+ GetTopTransactionIdIfAny() != InvalidTransactionId)
+ ereport(ERROR,
+ (errcode(ERRCODE_ACTIVE_SQL_TRANSACTION),
+ errmsg("cannot activate logical decoding in transaction that has
performed writes")));
+
+

Double blank lines.

~~~

39.
+ SpinLockAcquire(&(XLogLogicalInfoCtl->mutex));
+ status = XLogLogicalInfoCtl->status;
+ SpinLockRelease(&(XLogLogicalInfoCtl->mutex));
+
+ if (status == LOGICAL_DECODING_STATUS_READY)
+ PG_RETURN_VOID();

Should have some comment like: "If logical decoding is already enabled
then there is nothing to do."

OTOH, can't all this code be removed, since the top-of-loop code will
do the same thing anyway (I didn't think the extra sleep
prepare/cancel that it is doing was something to worry about).

~~~

40.
+ ConditionVariableSleep(&XLogLogicalInfoCtl->cv,
+ WAIT_EVENT_LOGICAL_DECODING_ACTIVATION);
+
+ continue;

Remove the blank line.

~~~

41.
+ /* Activate logical decoding */
+ do_activate_logical_decoding();

The comment is not helpful because it is practically the same as the
function name.

~~~

pg_deactivate_logical_decoding:

42.
Missing function comment.

~~~

43.
+ /*
+ * Hold ReplicationSlotAllocationLock to prevent slots from newly
+ * being created while deactivating the logical decoding.
+ */
+ LWLockAcquire(ReplicationSlotAllocationLock, LW_SHARED);

/newly being created/being created/

~~~

44.
+ for (int i = 0; i < max_replication_slots; i++)
+ {
+ ReplicationSlot *s = &ReplicationSlotCtl->replication_slots[i];
+
+ if (!s->in_use)
+ continue;
+
+ if (SlotIsPhysical(s))
+ continue;
+
+ if (s->data.invalidated != RS_INVAL_NONE)
+ continue;
+
+ valid_logical_slots++;
+ }

IMO it is more natural here to check "if (!SlotIsLogical(s))" instead
of "if (SlotIsPhysical(s))".

~~~

45.
+ if (valid_logical_slots > 0)
+ ereport(ERROR,
+ (errmsg("cannot deactivate logical decoding while having valid
logical replication slots")));

/while having valid logical replication slots/while valid logical
replication slots are present/

~~~

46.
+ if (XLogStandbyInfoActive() && !recoveryInProgress)
+ {
+ bool enabled = false;
+
+ XLogBeginInsert();
+ XLogRegisterData((char *) (&enabled), sizeof(bool));
+
+ XLogInsert(RM_XLOG_ID, XLOG_LOGICAL_DECODING_STATUS);
+ }
+
+ SpinLockAcquire(&(XLogLogicalInfoCtl->mutex));
+ XLogLogicalInfoCtl->status = LOGICAL_DECODING_STATUS_DISABLED;
+ SpinLockRelease(&(XLogLogicalInfoCtl->mutex));

In the pg_activate_logical_decoding function, the assignment of the
status and the writing of the XLog record was done within a critical
section. Why isn't the code doing the same here?

~~~

pg_get_logical_decoding_status:

47.
Missing function comment.

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

ValidateSlotSyncParams:

48.
- if (wal_level < WAL_LEVEL_LOGICAL)
+ if (!XLogLogicalInfoActive())
ereport(ERROR,
errcode(ERRCODE_INVALID_PARAMETER_VALUE),
- errmsg("replication slot synchronization requires \"wal_level\" >=
\"logical\""));
+ errmsg("replication slot synchronization requires \"wal_level\" >=
\"logical\" or to activate logical decoding"));

This is another message like what was in CreatePublication, and that
might be better split into an errmsg and errhint.

e.g.
errmsg("replication slot synchronization requires logical decoding to
be enabled");
errhint("Set \"wal_level\" to \"logical\" or use
pg_activate_logical_decoding.");

======
src/backend/replication/slot.c

RestoreSlotFromDisk:

49.
+ if (cp.slotdata.database != InvalidOid && !XLogLogicalInfoActive())
ereport(FATAL,
(errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
errmsg("logical replication slot \"%s\" exists, but \"wal_level\" <
\"logical\"",
NameStr(cp.slotdata.name)),
- errhint("Change \"wal_level\" to be \"logical\" or higher.")));
+ errhint("Change \"wal_level\" to be \"logical\" or higher, or
activate logical decoding with \"replica\".")));

Another message that could be made more consistent with the similar
ones from CreatePublication, ValidateSlotSyncParams, etc...

e.g.
errmsg("logical replication slot \"%s\" exists, but logical decoding
is not enabled");
errhint("Set \"wal_level\" to \"logical\" or use
pg_activate_logical_decoding.");

======
src/backend/utils/activity/wait_event_names.txt

50.
+LOGICAL_DECODING_ACTIVATION "Waiting for logical decoding to be activated."

Should this be more like "Waiting for a requested logical decoding
activation to complete."

======
src/include/access/xlog.h

51.
-#define XLogLogicalInfoActive() (wal_level >= WAL_LEVEL_LOGICAL)
+#define XLogLogicalInfoActive() (XLogLogicalInfoEnabled())

Those extra parentheses seem redundant now.

======
src/include/replication/logicalxlog.h

52.
+/*-------------------------------------------------------------------------
+ * logicalxlog.h
+ * Exports from replication/logical/logicalxlog.c.
+ *
+ * Copyright (c) 2012-2024, PostgreSQL Global Development Group
+ *
+ *-------------------------------------------------------------------------

/2024/2025/

~~~

53.
+typedef enum LogicalDecodingStatus
+{
+ LOGICAL_DECODING_STATUS_DISABLED = 0,
+ LOGICAL_DECODING_STATUS_XLOG_LOGICALINFO,
+ LOGICAL_DECODING_STATUS_READY,
+} LogicalDecodingStatus;

Maybe some more comments about these status phases would be helpful.
Specifically, it's not immediately clear if the
LOGICAL_DECODING_STATUS_XLOG_LOGICALINFO is a real state we could find
ourselves permanently, or if it is more like just a temporary
"pending" state while transitioning from DISABLED to READY.

AFAICT it is just transitory, so in that case, maybe
LOGICAL_DECODING_PENDING might be a more appropriate name.

~~~

54.
+typedef struct XLogLogicalInfoCtlData XLogLogicalInfoCtlData;
+extern XLogLogicalInfoCtlData *XLogLogicalInfoCtl;
+extern bool XLogLogicalInfo;

Add a blank line between the typedef and the externs.

======
[1] Bertrand's hide everything idea -
https://www.postgresql.org/message-id/Z3fimYj0fbkLmWJb%40ip-10-97-1-34.eu-west-3.compute.internal

Kind Regards,
Peter Smith.
Fujitsu Australia