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

Hi,

On Mon, Dec 30, 2024 at 10:44:38PM -0600, Masahiko Sawada wrote:

Hi all,

Logical decoding (and logical replication) are available only when
wal_level = logical. As the documentation says[1], Using the 'logical'
level increases the WAL volume which could negatively affect the
performance. For that reason, users might want to start with using
'replica', but when they want to use logical decoding they need a
server restart to increase wal_level to 'logical'. My goal is to allow
users who are using 'replica' level to use logical decoding without a
server restart.

Thanks for starting that thread and +1 for the idea!

The first idea I came up with is to make the wal_level a PGC_SIGHUP
parameter. However, it affects not only setting 'replica' to 'logical'
but also setting 'minimal' to 'replica' or higher. I'm not sure the
latter case is common and it might require a checkpoint. I don't want
to make the patch complex for uncommon cases.

The second idea is to somehow allow both WAL-logging logical info and
logical decoding even when wal_level is 'replica'. I've attached a PoC
patch for that. The patch introduces new SQL functions such as
pg_activate_logical_decoding() and pg_deactivate_logical_decoding().
These functions are available only when wal_level is 'repilca'(or
higher). In pg_activate_logical_decoding(), we set the status of
logical decoding stored on the shared memory from 'disabled' to
'xlog-logical-info', allowing all processes to write logical
information to WAL records for logical decoding. But the logical
decoding is still not allowed. Once we confirm all in-progress
transactions completed, we switch the status to
'logical-decoding-ready', meaning that users can create logical
replication slots and use logical decoding.

Overall, with the patch, there are two ways to enable logical
decoding: setting wal_level to 'logical' and calling
pg_activate_logical_decoding() when wal_level is 'replica'. I left the
'logical' level for backward compatibility and for users who want to
enable the logical decoding without calling that SQL function. If we
can automatically enable the logical decoding when creating the first
logical replication slot, probably we no longer need the 'logical'
level. There is room to discuss the user interface. Feedback is very
welcome.

If we don't want to force wal_level = logical to enable logical decoding (your
second idea) then I think that it would be better to "hide" everything behind
logical replication slot creation / deletion. That would mean that having at
least one logical replication slot created would be synonym to "activate logical
decoding" and zero logical replication slot created would be synonym to "deactivate
logical decoding".

That way:

1. an end user don't need to manipulate new functions and would just rely on
replication slots existence
2. we ensure that no extra WAL data is generated if not absolutely "needed"

Thoughts?

Regards,

--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com

On Fri, Jan 3, 2025, at 10:14 AM, Bertrand Drouvot wrote:

If we don't want to force wal_level = logical to enable logical decoding (your
second idea) then I think that it would be better to "hide" everything behind
logical replication slot creation / deletion. That would mean that having at
least one logical replication slot created would be synonym to "activate logical
decoding" and zero logical replication slot created would be synonym to "deactivate
logical decoding".

I like this idea. The logical replication slot existence already has the
required protections and guarantees (no running transactions from the past while
creating) for logical decoding.

ERROR: logical decoding requires "wal_level" >= "logical"
STATEMENT: select pg_create_logical_replication_slot('test', 'pgoutput');

FATAL: logical replication slot "test" exists, but "wal_level" < "logical"
HINT: Change "wal_level" to be "logical" or higher.

Having said that, you are basically folding 'logical' machinery into 'replica'.
The 'logical' option can still exists but it will be less attractive because it
increases the WAL volume even if you are not using logical replication. I don't
know if the current 'logical' behavior (WAL records for logical decoding even
if there is no active logical replication) has any use case (maybe someone
inspects these extra records for analysis) but one suggestion (separate patch)
is to make 'logical' synonymous with the new 'replica' behavior (logical
decoding capability). This opens the door to remove 'logical' in future
releases (accepted as synonym but not documented).

--
Euler Taveira
EDB https://www.enterprisedb.com/

On Fri, Jan 3, 2025 at 6:31 AM Euler Taveira <euler@eulerto.com> wrote:

On Fri, Jan 3, 2025, at 10:14 AM, Bertrand Drouvot wrote:

If we don't want to force wal_level = logical to enable logical decoding (your
second idea) then I think that it would be better to "hide" everything behind
logical replication slot creation / deletion. That would mean that having at
least one logical replication slot created would be synonym to "activate logical
decoding" and zero logical replication slot created would be synonym to "deactivate
logical decoding".

I like this idea. The logical replication slot existence already has the
required protections and guarantees (no running transactions from the past while
creating) for logical decoding.

I agree that it's better behavior.

Having said that, you are basically folding 'logical' machinery into 'replica'.
The 'logical' option can still exists but it will be less attractive because it
increases the WAL volume even if you are not using logical replication. I don't
know if the current 'logical' behavior (WAL records for logical decoding even
if there is no active logical replication) has any use case (maybe someone
inspects these extra records for analysis) but one suggestion (separate patch)
is to make 'logical' synonymous with the new 'replica' behavior (logical
decoding capability). This opens the door to remove 'logical' in future
releases (accepted as synonym but not documented).

To enable the logical decoding automatically when the first slot is
created, probably we need to do something like:

1. enable WAL-logging logical info.
2. wait for all running transactions to finish.
3. enable logical decoding, and write a XLOG_LOGICAL_DECODING_STATUS record,
4. create a logical slot
4-1. reserve the WAL and write an XLOG_RUNNING_XACTS record.
4-2. wait for all transactions in the XLOG_RUNNING_XACTS record to
finish during creating the initial snapshot.

That is, we could need to wait for different sets of transactions to
finish (at 2 and 4-2). Which could surprise users since the first slot
creation could have users wait for a longer time. Using the 'logical'
level would avoid such waits.

Also, if we make the 'logical' level synonymous with the new 'replica'
behavior, we need to adjust regression tests as well. In some
regression tests (e.g., ondisk_startup.spec), we create a logical slot
while a transaction is concurrently running. We expect we wait for the
concurrent transaction during the slot creation but with the above
algorithm, we would wait for them to activate logical decoding. So I
agree that making the 'logical' level synonymous with the new
'replica' behavior is done in a separate patch. It would be a good
start to implement the new 'replica' level behavior.

Regards,

--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com

On Sat, Jan 4, 2025 at 6:03 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:

On Fri, Jan 3, 2025 at 6:31 AM Euler Taveira <euler@eulerto.com> wrote:

On Fri, Jan 3, 2025, at 10:14 AM, Bertrand Drouvot wrote:

If we don't want to force wal_level = logical to enable logical decoding (your
second idea) then I think that it would be better to "hide" everything behind
logical replication slot creation / deletion. That would mean that having at
least one logical replication slot created would be synonym to "activate logical
decoding" and zero logical replication slot created would be synonym to "deactivate
logical decoding".

I like this idea. The logical replication slot existence already has the
required protections and guarantees (no running transactions from the past while
creating) for logical decoding.

I agree that it's better behavior.

Having said that, you are basically folding 'logical' machinery into 'replica'.
The 'logical' option can still exists but it will be less attractive because it
increases the WAL volume even if you are not using logical replication. I don't
know if the current 'logical' behavior (WAL records for logical decoding even
if there is no active logical replication) has any use case (maybe someone
inspects these extra records for analysis) but one suggestion (separate patch)
is to make 'logical' synonymous with the new 'replica' behavior (logical
decoding capability). This opens the door to remove 'logical' in future
releases (accepted as synonym but not documented).

To enable the logical decoding automatically when the first slot is
created, probably we need to do something like:

1. enable WAL-logging logical info.
2. wait for all running transactions to finish.
3. enable logical decoding, and write a XLOG_LOGICAL_DECODING_STATUS record,
4. create a logical slot
4-1. reserve the WAL and write an XLOG_RUNNING_XACTS record.
4-2. wait for all transactions in the XLOG_RUNNING_XACTS record to
finish during creating the initial snapshot.

A naive question: Can we combine step 2 and 4-2 - may be we need to
combine 4-1 and 3 too.

--
Best Wishes,
Ashutosh Bapat

On Mon, Jan 6, 2025 at 3:20 AM Ashutosh Bapat
<ashutosh.bapat.oss@gmail.com> wrote:

On Sat, Jan 4, 2025 at 6:03 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:

On Fri, Jan 3, 2025 at 6:31 AM Euler Taveira <euler@eulerto.com> wrote:

On Fri, Jan 3, 2025, at 10:14 AM, Bertrand Drouvot wrote:

If we don't want to force wal_level = logical to enable logical decoding (your
second idea) then I think that it would be better to "hide" everything behind
logical replication slot creation / deletion. That would mean that having at
least one logical replication slot created would be synonym to "activate logical
decoding" and zero logical replication slot created would be synonym to "deactivate
logical decoding".

I like this idea. The logical replication slot existence already has the
required protections and guarantees (no running transactions from the past while
creating) for logical decoding.

I agree that it's better behavior.

Having said that, you are basically folding 'logical' machinery into 'replica'.
The 'logical' option can still exists but it will be less attractive because it
increases the WAL volume even if you are not using logical replication. I don't
know if the current 'logical' behavior (WAL records for logical decoding even
if there is no active logical replication) has any use case (maybe someone
inspects these extra records for analysis) but one suggestion (separate patch)
is to make 'logical' synonymous with the new 'replica' behavior (logical
decoding capability). This opens the door to remove 'logical' in future
releases (accepted as synonym but not documented).

FYI one possible use case of the 'logical' level would be that if we
support retrieving WAL records for logical decoding from somewhere
like restore_command in the future, users would like to keep the
wal_level 'logical' or keep the logical decoding active.

To enable the logical decoding automatically when the first slot is
created, probably we need to do something like:

1. enable WAL-logging logical info.
2. wait for all running transactions to finish.
3. enable logical decoding, and write a XLOG_LOGICAL_DECODING_STATUS record,
4. create a logical slot
4-1. reserve the WAL and write an XLOG_RUNNING_XACTS record.
4-2. wait for all transactions in the XLOG_RUNNING_XACTS record to
finish during creating the initial snapshot.

A naive question: Can we combine step 2 and 4-2 - may be we need to
combine 4-1 and 3 too.

Yeah, maybe we can somewhat optimize this process. Probably we can do like:

1. create a logical slot.
2. enable WAL-logging logical info.
3. reserve the WAL and write an XLOG_RUNNING_XACTS record.
4. wait for all transactions in the XLOG_RUNNING_XACTS record to
finish during creating the initial snapshot.
4-1. write a XLOG_LOGICAL_DECODING_STATUS record if a process
detects all transactions in the XLOG_RUNNING_XACTS record to finish.

However, we need to note that 4-1 should be done by only one process.
Also, it's a bit weird and concerns to me that the snapbuild
initialization contains the process of activating logical decoding.

Regards,

--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com

Hi Sawada-San.

FWIW, I also thought it was a good idea suggested by Bertrand [1]Bertrand's hide everything idea - /messages/by-id/Z3fimYj0fbkLmWJb@ip-10-97-1-34.eu-west-3.compute.internal 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@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 - /messages/by-id/Z3fimYj0fbkLmWJb@ip-10-97-1-34.eu-west-3.compute.internal
/messages/by-id/Z3fimYj0fbkLmWJb@ip-10-97-1-34.eu-west-3.compute.internal

Kind Regards,
Peter Smith.
Fujitsu Australia

On Tue, Dec 31, 2024 at 10:15 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:

Hi all,

Logical decoding (and logical replication) are available only when
wal_level = logical. As the documentation says[1], Using the 'logical'
level increases the WAL volume which could negatively affect the
performance. For that reason, users might want to start with using
'replica', but when they want to use logical decoding they need a
server restart to increase wal_level to 'logical'. My goal is to allow
users who are using 'replica' level to use logical decoding without a
server restart. There are other GUC parameters related to logical
decoding and logical replication such as max_wal_senders,
max_logical_replication_workers, and max_replication_slots, but even
if users set these parameters >0, there would not be a noticeable
performance impact. And their default values are already >0. So I'd
like to focus on making only the wal_level dynamic GUC parameter.
There are several earlier discussions[2][3] but no one has submitted
patches unless I'm missing something.

The first idea I came up with is to make the wal_level a PGC_SIGHUP
parameter. However, it affects not only setting 'replica' to 'logical'
but also setting 'minimal' to 'replica' or higher. I'm not sure the
latter case is common and it might require a checkpoint. I don't want
to make the patch complex for uncommon cases.

The second idea is to somehow allow both WAL-logging logical info and
logical decoding even when wal_level is 'replica'. I've attached a PoC
patch for that. The patch introduces new SQL functions such as
pg_activate_logical_decoding() and pg_deactivate_logical_decoding().
These functions are available only when wal_level is 'repilca'(or
higher). In pg_activate_logical_decoding(), we set the status of
logical decoding stored on the shared memory from 'disabled' to
'xlog-logical-info', allowing all processes to write logical
information to WAL records for logical decoding. But the logical
decoding is still not allowed. Once we confirm all in-progress
transactions completed, we switch the status to
'logical-decoding-ready', meaning that users can create logical
replication slots and use logical decoding.

Overall, with the patch, there are two ways to enable logical
decoding: setting wal_level to 'logical' and calling
pg_activate_logical_decoding() when wal_level is 'replica'. I left the
'logical' level for backward compatibility and for users who want to
enable the logical decoding without calling that SQL function. If we
can automatically enable the logical decoding when creating the first
logical replication slot, probably we no longer need the 'logical'
level. There is room to discuss the user interface. Feedback is very
welcome.

If a server is running at minimal wal_level and they want to enable
logical replication, they would still need a server restart. That
would be rare but not completely absent.

Our documentation says "wal_level determines how much information is
written to the WAL.". Users would may not expect that the WAL amount
changes while wal_level = replica depending upon whether logical
decoding is possible. It may be possible to set the expectations right
by changing the documentation. It's not in the patch, so I am not sure
whether this is considered.

Cloud providers do not like multiple ways of changing configuration
esp. when they can not control it. See [1]/messages/by-id/CA+VUV5rEKt2+CdC_KUaPoihMu+i5ChT4WVNTr4CD5-xXZUfuQw@mail.gmail.com. Changing wal_level through
a SQL function may fit the same category.

I agree that it would be a lot of work to make all combinations of
wal_level changes work, but changing wal_level through SIGHUP looks
like a cleaner solution. Is there way that we make the GUC SIGHUP but
disallow certain combinations of old and new values?

[1]: /messages/by-id/CA+VUV5rEKt2+CdC_KUaPoihMu+i5ChT4WVNTr4CD5-xXZUfuQw@mail.gmail.com

--
Best Wishes,
Ashutosh Bapat

On Thu, Jan 9, 2025 at 3:29 AM Ashutosh Bapat
<ashutosh.bapat.oss@gmail.com> wrote:

On Tue, Dec 31, 2024 at 10:15 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:

Hi all,

Logical decoding (and logical replication) are available only when
wal_level = logical. As the documentation says[1], Using the 'logical'
level increases the WAL volume which could negatively affect the
performance. For that reason, users might want to start with using
'replica', but when they want to use logical decoding they need a
server restart to increase wal_level to 'logical'. My goal is to allow
users who are using 'replica' level to use logical decoding without a
server restart. There are other GUC parameters related to logical
decoding and logical replication such as max_wal_senders,
max_logical_replication_workers, and max_replication_slots, but even
if users set these parameters >0, there would not be a noticeable
performance impact. And their default values are already >0. So I'd
like to focus on making only the wal_level dynamic GUC parameter.
There are several earlier discussions[2][3] but no one has submitted
patches unless I'm missing something.

The first idea I came up with is to make the wal_level a PGC_SIGHUP
parameter. However, it affects not only setting 'replica' to 'logical'
but also setting 'minimal' to 'replica' or higher. I'm not sure the
latter case is common and it might require a checkpoint. I don't want
to make the patch complex for uncommon cases.

The second idea is to somehow allow both WAL-logging logical info and
logical decoding even when wal_level is 'replica'. I've attached a PoC
patch for that. The patch introduces new SQL functions such as
pg_activate_logical_decoding() and pg_deactivate_logical_decoding().
These functions are available only when wal_level is 'repilca'(or
higher). In pg_activate_logical_decoding(), we set the status of
logical decoding stored on the shared memory from 'disabled' to
'xlog-logical-info', allowing all processes to write logical
information to WAL records for logical decoding. But the logical
decoding is still not allowed. Once we confirm all in-progress
transactions completed, we switch the status to
'logical-decoding-ready', meaning that users can create logical
replication slots and use logical decoding.

Overall, with the patch, there are two ways to enable logical
decoding: setting wal_level to 'logical' and calling
pg_activate_logical_decoding() when wal_level is 'replica'. I left the
'logical' level for backward compatibility and for users who want to
enable the logical decoding without calling that SQL function. If we
can automatically enable the logical decoding when creating the first
logical replication slot, probably we no longer need the 'logical'
level. There is room to discuss the user interface. Feedback is very
welcome.

If a server is running at minimal wal_level and they want to enable
logical replication, they would still need a server restart. That
would be rare but not completely absent.

Currently we don't allow the server to start with the 'minimal' level
and max_wal_senders > 0. Even if we support changing 'minimal' to
'logical' without a server restart, we still need a server restart to
increase max_wal_senders for users who want to use logical
replication. Or we need to eliminate this restriction too. I guess it
would be too complex for such uncommon use cases.

Our documentation says "wal_level determines how much information is
written to the WAL.". Users would may not expect that the WAL amount
changes while wal_level = replica depending upon whether logical
decoding is possible. It may be possible to set the expectations right
by changing the documentation. It's not in the patch, so I am not sure
whether this is considered.

We should mention that in the doc. The WAL amount changes depending on
not only wal_level but also other parameters such as wal_log_hints and
full_page_writes.

Cloud providers do not like multiple ways of changing configuration
esp. when they can not control it. See [1]. Changing wal_level through
a SQL function may fit the same category.

Thank you for pointing it out. This would support the idea of
automatically enabling logical decoding.

I agree that it would be a lot of work to make all combinations of
wal_level changes work, but changing wal_level through SIGHUP looks
like a cleaner solution. Is there way that we make the GUC SIGHUP but
disallow certain combinations of old and new values?

While I agree that it's cleaner I think there is no way today. I think
we need to invent something for that.

Another idea would be to have another SIGHUP GUC parameter to control
logical info as another way to enable logical info WAL-logging while
trying to deprecate the 'logical' level over some releases. While it
doesn't need a SQL function, it could confuse users since we will
require two GUC parameters for doing things that we used to use one
GUC parameter.

Regards,

--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com

Hi,

On Tue, Jan 7, 2025 at 11:30 PM Peter Smith <smithpb2250@gmail.com> wrote:

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.

I think that as long as there is at least one logical replication slot
users still have the will to execute logical decoding, so they require
logical info WAL-logging. I think it would rather be good for users to
be able to forget about 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?

Yes.

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?

I think that if we automatically enable logical decoding even when
wal_level=replica, we would not need pg_get_logical_decoding_status().
These changes would break compatibility tools checking if wal_level is
'logical', but I guess that tools will not need to check that anymore.
Users can simply understand that if there is at least one logical
replication slot the system writes necessary information to use it.
Having said that we anyway need to mention it in the doc and raising
NOTICE/WARNING would also be a good idea as you mentioned.

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.

Thank you for reviewing the patch! It seems that most comments are
about wording, typos, suggestions for variable names and function
names etc. Since the patch is in the PoC/design phase where we might
need to change the whole design, I'll refer to these comments when
updating the patch next time.

Regards,

--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com

On Fri, Jan 10, 2025 at 8:28 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:

Hi,

On Tue, Jan 7, 2025 at 11:30 PM Peter Smith <smithpb2250@gmail.com> wrote:

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.

I think that as long as there is at least one logical replication slot
users still have the will to execute logical decoding, so they require
logical info WAL-logging. I think it would rather be good for users to
be able to forget about 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?

Yes.

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?

I think that if we automatically enable logical decoding even when
wal_level=replica, we would not need pg_get_logical_decoding_status().
These changes would break compatibility tools checking if wal_level is
'logical', but I guess that tools will not need to check that anymore.
Users can simply understand that if there is at least one logical
replication slot the system writes necessary information to use it.
Having said that we anyway need to mention it in the doc and raising
NOTICE/WARNING would also be a good idea as you mentioned.

Hi Sawada-San

IMO it seems misleading to be having logical replication behaviour
while the wal_level still displays as 'replica'.

So, I was wondering if it would be worthwhile to introduce a
'show_hook' for this GUC.

I hacked a quick implementation (attached) which now displays a middle
value for this scenario as "replica-logical". See below:

test_pub=# show wal_level;
wal_level
-----------
replica
(1 row)

test_pub=# select pg_get_logical_decoding_status();
pg_get_logical_decoding_status
--------------------------------
disabled
(1 row)

test_pub=# select pg_activate_logical_decoding();
pg_activate_logical_decoding
------------------------------

(1 row)

test_pub=# show wal_level;
wal_level
-----------------
replica-logical
(1 row)

test_pub=# select pg_get_logical_decoding_status();
pg_get_logical_decoding_status
--------------------------------
ready

(1 row)
test_pub=# \x
Expanded display is on.
test_pub=# select * from pg_settings where name in ('wal_level');
-[ RECORD 1 ]---+--------------------------------------------------
name | wal_level
setting | replica-logical
unit |
category | Write-Ahead Log / Settings
short_desc | Sets the level of information written to the WAL.
extra_desc |
context | postmaster
vartype | enum
source | configuration file
min_val |
max_val |
enumvals | {minimal,replica,logical}
boot_val | replica
reset_val | replica
sourcefile | /home/postgres/MYDATAOSS_2PC_PUB/postgresql.conf
sourceline | 199
pending_restart | f

~~

OTOH, you might prefer to display the "replica-logical" case as
"logical". (doing so might make things easier for existing tools that
are already checking if wal_level is "logical").

Of course, it doesn't really change the 'wal_level' variable value, it
just changes how it is *displayed*. Anyway, maybe it's an idea to
consider.

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

On Tue, 31 Dec 2024 at 10:15, Masahiko Sawada <sawada.mshk@gmail.com> wrote:

Hi all,

Logical decoding (and logical replication) are available only when
wal_level = logical. As the documentation says[1], Using the 'logical'
level increases the WAL volume which could negatively affect the
performance. For that reason, users might want to start with using
'replica', but when they want to use logical decoding they need a
server restart to increase wal_level to 'logical'. My goal is to allow
users who are using 'replica' level to use logical decoding without a
server restart. There are other GUC parameters related to logical
decoding and logical replication such as max_wal_senders,
max_logical_replication_workers, and max_replication_slots, but even
if users set these parameters >0, there would not be a noticeable
performance impact. And their default values are already >0. So I'd
like to focus on making only the wal_level dynamic GUC parameter.
There are several earlier discussions[2][3] but no one has submitted
patches unless I'm missing something.

The first idea I came up with is to make the wal_level a PGC_SIGHUP
parameter. However, it affects not only setting 'replica' to 'logical'
but also setting 'minimal' to 'replica' or higher. I'm not sure the
latter case is common and it might require a checkpoint. I don't want
to make the patch complex for uncommon cases.

The second idea is to somehow allow both WAL-logging logical info and
logical decoding even when wal_level is 'replica'. I've attached a PoC
patch for that. The patch introduces new SQL functions such as
pg_activate_logical_decoding() and pg_deactivate_logical_decoding().
These functions are available only when wal_level is 'repilca'(or
higher). In pg_activate_logical_decoding(), we set the status of
logical decoding stored on the shared memory from 'disabled' to
'xlog-logical-info', allowing all processes to write logical
information to WAL records for logical decoding. But the logical
decoding is still not allowed. Once we confirm all in-progress
transactions completed, we switch the status to
'logical-decoding-ready', meaning that users can create logical
replication slots and use logical decoding.

I felt that the only disadvantage with this approach is that we
currently wait for all in-progress transactions to complete before
enabling logical decoding. If a long-running transaction exists and
the session enabling logical decoding fails—due to factors like
statement_timeout, transaction_timeout,
idle_in_transaction_session_timeout, idle_session_timeout, or any
other failure. This would require restarting the wait. During this
time, there's a risk that a new long-running transaction could start,
further delaying the process. Probably this could be solved if the
waiting is done from any of the background processes through
PGC_SIGHUP. While this type of failure is likely rare, I’m unsure
whether we should consider this scenario.

Thoughts?

Regards,
Vignesh

On Mon, Jan 13, 2025 at 2:52 PM vignesh C <vignesh21@gmail.com> wrote:

I felt that the only disadvantage with this approach is that we
currently wait for all in-progress transactions to complete before
enabling logical decoding. If a long-running transaction exists and
the session enabling logical decoding fails—due to factors like
statement_timeout, transaction_timeout,
idle_in_transaction_session_timeout, idle_session_timeout, or any
other failure. This would require restarting the wait. During this
time, there's a risk that a new long-running transaction could start,
further delaying the process. Probably this could be solved if the
waiting is done from any of the background processes through
PGC_SIGHUP. While this type of failure is likely rare, I’m unsure
whether we should consider this scenario.

A related question: While the operation is waiting for already running
transactions to end, the backends whose transactions have finished may
start new transactions. When we switch WAL from 'replica' to
'logical', there may be some transactions running. Will this lead to
WAL stream with mixes WAL records - some with logical information and
some without? Do we need to adjust logical decoding to tackle this
situation? Is there a chance that some WAL from same transaction have
logical information and some do not?

--
Best Wishes,
Ashutosh Bapat

On Mon, Jan 13, 2025 at 1:22 AM vignesh C <vignesh21@gmail.com> wrote:

On Tue, 31 Dec 2024 at 10:15, Masahiko Sawada <sawada.mshk@gmail.com> wrote:

Hi all,

Logical decoding (and logical replication) are available only when
wal_level = logical. As the documentation says[1], Using the 'logical'
level increases the WAL volume which could negatively affect the
performance. For that reason, users might want to start with using
'replica', but when they want to use logical decoding they need a
server restart to increase wal_level to 'logical'. My goal is to allow
users who are using 'replica' level to use logical decoding without a
server restart. There are other GUC parameters related to logical
decoding and logical replication such as max_wal_senders,
max_logical_replication_workers, and max_replication_slots, but even
if users set these parameters >0, there would not be a noticeable
performance impact. And their default values are already >0. So I'd
like to focus on making only the wal_level dynamic GUC parameter.
There are several earlier discussions[2][3] but no one has submitted
patches unless I'm missing something.

The first idea I came up with is to make the wal_level a PGC_SIGHUP
parameter. However, it affects not only setting 'replica' to 'logical'
but also setting 'minimal' to 'replica' or higher. I'm not sure the
latter case is common and it might require a checkpoint. I don't want
to make the patch complex for uncommon cases.

The second idea is to somehow allow both WAL-logging logical info and
logical decoding even when wal_level is 'replica'. I've attached a PoC
patch for that. The patch introduces new SQL functions such as
pg_activate_logical_decoding() and pg_deactivate_logical_decoding().
These functions are available only when wal_level is 'repilca'(or
higher). In pg_activate_logical_decoding(), we set the status of
logical decoding stored on the shared memory from 'disabled' to
'xlog-logical-info', allowing all processes to write logical
information to WAL records for logical decoding. But the logical
decoding is still not allowed. Once we confirm all in-progress
transactions completed, we switch the status to
'logical-decoding-ready', meaning that users can create logical
replication slots and use logical decoding.

I felt that the only disadvantage with this approach is that we
currently wait for all in-progress transactions to complete before
enabling logical decoding. If a long-running transaction exists and
the session enabling logical decoding fails—due to factors like
statement_timeout, transaction_timeout,
idle_in_transaction_session_timeout, idle_session_timeout, or any
other failure. This would require restarting the wait. During this
time, there's a risk that a new long-running transaction could start,
further delaying the process. Probably this could be solved if the
waiting is done from any of the background processes through
PGC_SIGHUP. While this type of failure is likely rare, I’m unsure
whether we should consider this scenario.

Thoughts?

Yeah, delegating the activation to the background process such as the
checkpointer would also be one solution. This would work with the
approach that we enable the logical decoding via
pg_activate_logical_decoding(). On the other hand, if we support
automatically enabling the logical decoding (and logical info logging)
when the first logical slot is created, we might want to have the
process who is creating the logical slot activate the logical decoding
too.

Regards,

--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com

On Mon, Jan 13, 2025 at 1:31 AM Ashutosh Bapat
<ashutosh.bapat.oss@gmail.com> wrote:

On Mon, Jan 13, 2025 at 2:52 PM vignesh C <vignesh21@gmail.com> wrote:

I felt that the only disadvantage with this approach is that we
currently wait for all in-progress transactions to complete before
enabling logical decoding. If a long-running transaction exists and
the session enabling logical decoding fails—due to factors like
statement_timeout, transaction_timeout,
idle_in_transaction_session_timeout, idle_session_timeout, or any
other failure. This would require restarting the wait. During this
time, there's a risk that a new long-running transaction could start,
further delaying the process. Probably this could be solved if the
waiting is done from any of the background processes through
PGC_SIGHUP. While this type of failure is likely rare, I’m unsure
whether we should consider this scenario.

A related question: While the operation is waiting for already running
transactions to end, the backends whose transactions have finished may
start new transactions. When we switch WAL from 'replica' to
'logical', there may be some transactions running. Will this lead to
WAL stream with mixes WAL records - some with logical information and
some without?

Yes. There could be mixed WAL records until all running transactions complete.

Do we need to adjust logical decoding to tackle this
situation? Is there a chance that some WAL from same transaction have
logical information and some do not?

In the current approach, we first enable logical info WAL-logging to
let newly started transactions do logical info WAL-logging, then allow
the logical decoding only after all running transactions complete.
Therefore, at the time when we allow the logical decoding, all WAL
records are written with logical information.

Regards,

--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com

On Tue, 14 Jan 2025 at 03:03, Masahiko Sawada <sawada.mshk@gmail.com> wrote:

Yeah, delegating the activation to the background process such as the
checkpointer would also be one solution. This would work with the
approach that we enable the logical decoding via
pg_activate_logical_decoding(). On the other hand, if we support
automatically enabling the logical decoding (and logical info logging)
when the first logical slot is created, we might want to have the
process who is creating the logical slot activate the logical decoding
too.

One possible approach is to delegate the task of updating the wal
level to include logical decoding information during the creation of
the first logical replication slot also to a background process (e.g.,
the checkpoint process) and wait until the wal level has been updated
and reaches ready state. Additionally, we could introduce a new
optional parameter during slot creation to specify whether we should
roll back changes related to updating the WAL level in case of
failure. If a rollback is needed, we could invoke the
logical_decoding_activation_abort_callback. By default, the rollback
could be set to false, since in most failure scenarios, the user will
likely attempt to create the slot again immediately. By then, the WAL
level may have already been updated, eliminating the need to wait for
any new long-running transactions.
An alternative approach could be to skip the rollback of logical info
logging during logical replication slot creation in case of failure,
without introducing a rollback parameter. This is because there may be
concurrent transactions (e.g., parallel slot creation or
pg_activate_logical_decoding from different sessions) that would also
fail. In such cases, the user could be allowed to call
pg_deactivate_logical_decoding if needed.

Regards,
Vignesh

On Tue, Jan 14, 2025 at 3:15 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:

On Mon, Jan 13, 2025 at 1:31 AM Ashutosh Bapat
<ashutosh.bapat.oss@gmail.com> wrote:

On Mon, Jan 13, 2025 at 2:52 PM vignesh C <vignesh21@gmail.com> wrote:

I felt that the only disadvantage with this approach is that we
currently wait for all in-progress transactions to complete before
enabling logical decoding. If a long-running transaction exists and
the session enabling logical decoding fails—due to factors like
statement_timeout, transaction_timeout,
idle_in_transaction_session_timeout, idle_session_timeout, or any
other failure. This would require restarting the wait. During this
time, there's a risk that a new long-running transaction could start,
further delaying the process. Probably this could be solved if the
waiting is done from any of the background processes through
PGC_SIGHUP. While this type of failure is likely rare, I’m unsure
whether we should consider this scenario.

A related question: While the operation is waiting for already running
transactions to end, the backends whose transactions have finished may
start new transactions. When we switch WAL from 'replica' to
'logical', there may be some transactions running. Will this lead to
WAL stream with mixes WAL records - some with logical information and
some without?

Yes. There could be mixed WAL records until all running transactions complete.

Do we need to adjust logical decoding to tackle this
situation? Is there a chance that some WAL from same transaction have
logical information and some do not?

In the current approach, we first enable logical info WAL-logging to
let newly started transactions do logical info WAL-logging, then allow
the logical decoding only after all running transactions complete.
Therefore, at the time when we allow the logical decoding, all WAL
records are written with logical information.

Thanks for the clarification. Let's consider that T1, T2, T3 were
running when the request to enable logical decoding came in. Let's say
T1 finished first, then T2 and then T3. But in-the mean time T4, T5
and T6 were started, which logged WAL with logical information. While
T2, T3, T4, T5, T6 all are running simultaneously, there will be mixed
WAL logs (some with logical information and some without). When T3
ends, logical decoding will be allowed. If a logical decoding process
is started at this point, it will start reading WAL from the earliest
of T4, T5 or T6 and hence it will encounter WAL written by T2 or T3
which do not have logical information. That would create a problem.
Thus in order for the logical process to start The transactions T4, T5
and T6 need to be finished as well. Am I correct? If that is so, my
earlier proposal to combine the step to wait for running transactions
to complete for both logical decoding as well as replication slot
creation would not be possible.

Will the mixed logs create a problem for an ongoing physical stream replication?

--
Best Wishes,
Ashutosh Bapat

On Sun, Jan 12, 2025 at 10:52 PM Peter Smith <smithpb2250@gmail.com> wrote:

On Fri, Jan 10, 2025 at 8:28 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:

Hi,

On Tue, Jan 7, 2025 at 11:30 PM Peter Smith <smithpb2250@gmail.com> wrote:

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.

I think that as long as there is at least one logical replication slot
users still have the will to execute logical decoding, so they require
logical info WAL-logging. I think it would rather be good for users to
be able to forget about 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?

Yes.

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?

I think that if we automatically enable logical decoding even when
wal_level=replica, we would not need pg_get_logical_decoding_status().
These changes would break compatibility tools checking if wal_level is
'logical', but I guess that tools will not need to check that anymore.
Users can simply understand that if there is at least one logical
replication slot the system writes necessary information to use it.
Having said that we anyway need to mention it in the doc and raising
NOTICE/WARNING would also be a good idea as you mentioned.

Hi Sawada-San

IMO it seems misleading to be having logical replication behaviour
while the wal_level still displays as 'replica'.

So, I was wondering if it would be worthwhile to introduce a
'show_hook' for this GUC.

I hacked a quick implementation (attached) which now displays a middle
value for this scenario as "replica-logical". See below:

test_pub=# show wal_level;
wal_level
-----------
replica
(1 row)

test_pub=# select pg_get_logical_decoding_status();
pg_get_logical_decoding_status
--------------------------------
disabled
(1 row)

test_pub=# select pg_activate_logical_decoding();
pg_activate_logical_decoding
------------------------------

(1 row)

test_pub=# show wal_level;
wal_level
-----------------
replica-logical
(1 row)

I think this would break the compatibility for monitoring tools
anyway. Given that the logical decoding is enabled if there is at
least one logical slot, they just need to check the number of existing
logical slots to know whether logical info WAL-logging is enabled or
not.

Regards,

--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com

On Tue, Jan 14, 2025 at 9:46 PM Ashutosh Bapat
<ashutosh.bapat.oss@gmail.com> wrote:

On Tue, Jan 14, 2025 at 3:15 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:

On Mon, Jan 13, 2025 at 1:31 AM Ashutosh Bapat
<ashutosh.bapat.oss@gmail.com> wrote:

On Mon, Jan 13, 2025 at 2:52 PM vignesh C <vignesh21@gmail.com> wrote:

I felt that the only disadvantage with this approach is that we
currently wait for all in-progress transactions to complete before
enabling logical decoding. If a long-running transaction exists and
the session enabling logical decoding fails—due to factors like
statement_timeout, transaction_timeout,
idle_in_transaction_session_timeout, idle_session_timeout, or any
other failure. This would require restarting the wait. During this
time, there's a risk that a new long-running transaction could start,
further delaying the process. Probably this could be solved if the
waiting is done from any of the background processes through
PGC_SIGHUP. While this type of failure is likely rare, I’m unsure
whether we should consider this scenario.

A related question: While the operation is waiting for already running
transactions to end, the backends whose transactions have finished may
start new transactions. When we switch WAL from 'replica' to
'logical', there may be some transactions running. Will this lead to
WAL stream with mixes WAL records - some with logical information and
some without?

Yes. There could be mixed WAL records until all running transactions complete.

Do we need to adjust logical decoding to tackle this
situation? Is there a chance that some WAL from same transaction have
logical information and some do not?

In the current approach, we first enable logical info WAL-logging to
let newly started transactions do logical info WAL-logging, then allow
the logical decoding only after all running transactions complete.
Therefore, at the time when we allow the logical decoding, all WAL
records are written with logical information.

Thanks for the clarification. Let's consider that T1, T2, T3 were
running when the request to enable logical decoding came in. Let's say
T1 finished first, then T2 and then T3. But in-the mean time T4, T5
and T6 were started, which logged WAL with logical information. While
T2, T3, T4, T5, T6 all are running simultaneously, there will be mixed
WAL logs (some with logical information and some without). When T3
ends, logical decoding will be allowed. If a logical decoding process
is started at this point, it will start reading WAL from the earliest
of T4, T5 or T6 and hence it will encounter WAL written by T2 or T3
which do not have logical information.

IIUC we should prevent a logical slot from reserving the WAL until T1,
T2, and T3 finishes. That is, we wait for T1, T2, and T3 to finish
before calling ReplicationSlotReserveWal() where we decide the start
point for reading WAL. That way, the logical decoding will start
reading WAL from the point after the latest commit record of T1, T2,
and T3 (i.e. T3 in this case), meaning that there is no mixed WAL
after this point and T4, T5, and T6 would not be included in the
logical decoding.

Will the mixed logs create a problem for an ongoing physical stream replication?

I don't see any problem for physical stream replication in terms of
mixed logs. We write a WAL record to enable the logical decoding after
the point where we can ensure there are no mixed WAL records. So the
logical decoding on standbys also can start reading WAL records from
that point.

Regards,

--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com

On Thu, Jan 16, 2025 at 4:28 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:

On Sun, Jan 12, 2025 at 10:52 PM Peter Smith <smithpb2250@gmail.com> wrote:

On Fri, Jan 10, 2025 at 8:28 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:

Hi,

On Tue, Jan 7, 2025 at 11:30 PM Peter Smith <smithpb2250@gmail.com> wrote:

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.

I think that as long as there is at least one logical replication slot
users still have the will to execute logical decoding, so they require
logical info WAL-logging. I think it would rather be good for users to
be able to forget about 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?

Yes.

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?

I think that if we automatically enable logical decoding even when
wal_level=replica, we would not need pg_get_logical_decoding_status().
These changes would break compatibility tools checking if wal_level is
'logical', but I guess that tools will not need to check that anymore.
Users can simply understand that if there is at least one logical
replication slot the system writes necessary information to use it.
Having said that we anyway need to mention it in the doc and raising
NOTICE/WARNING would also be a good idea as you mentioned.

Hi Sawada-San

IMO it seems misleading to be having logical replication behaviour
while the wal_level still displays as 'replica'.

So, I was wondering if it would be worthwhile to introduce a
'show_hook' for this GUC.

I hacked a quick implementation (attached) which now displays a middle
value for this scenario as "replica-logical". See below:

test_pub=# show wal_level;
wal_level
-----------
replica
(1 row)

test_pub=# select pg_get_logical_decoding_status();
pg_get_logical_decoding_status
--------------------------------
disabled
(1 row)

test_pub=# select pg_activate_logical_decoding();
pg_activate_logical_decoding
------------------------------

(1 row)

test_pub=# show wal_level;
wal_level
-----------------
replica-logical
(1 row)

I think this would break the compatibility for monitoring tools
anyway. Given that the logical decoding is enabled if there is at
least one logical slot, they just need to check the number of existing
logical slots to know whether logical info WAL-logging is enabled or
not.

But, that "replica-logical" string value was just for my demonstration.

From your reply, I cannot tell if you accidentally overlooked the next
part where I said:
OTOH, you might prefer to display the "replica-logical" case as "logical".

~~~

e.g. then the above example would look like this:

test_pub=# show wal_level;
wal_level
-----------
replica
(1 row)

test_pub=# select pg_get_logical_decoding_status();
pg_get_logical_decoding_status
--------------------------------
disabled
(1 row)

test_pub=# select pg_activate_logical_decoding();
pg_activate_logical_decoding
------------------------------
(1 row)

test_pub=# show wal_level;
wal_level
-----------
logical
(1 row)

Eventually, if the number of logical replication slots falls to 0, the
wal_level value displayed reverts to "replica".

Since there are no *new* wal_level strings introduced how would that
break compatibility for monitoring tools?

And, since the (fudged) wal_level value "logical" already indicates
whether logical info WAL-logging is enabled or not, no additional
kinds of checking are needed.

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