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

On Fri, Aug 29, 2025 at 9:38 AM Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote:
>
> I've attached the updated patch.
>

Few comments:
=============
1.
+ * When XLogLogicalInfoActive() is true, guarantee that a subtransaction's
+ * xid can only be seen in the WAL stream if its toplevel xid has been
+ * logged before. If necessary we log an xact_assignment record with fewer
+ * than PGPROC_MAX_CACHED_SUBXIDS. Note that it is fine if didLogXid isn't
+ * set for a transaction even though it appears in a WAL record, we just
+ * might superfluously log something. That can happen when an xid is
+ * included somewhere inside a wal record, but not in XLogRecord->xl_xid,
+ * like in xl_standby_locks.
*/
if (isSubXact && XLogLogicalInfoActive() &&
!TopTransactionStateData.didLogXid)

Instead of writing XLogLogicalInfoActive() is true in comments, can we
say When effective wal_level is logical and then also point to some
place if required where the patch has explained about effective
wal_level? Otherwise, it sounds like we are writing what is apparent
from code and may not be very clear.

2.
- /*
- * Invalidate logical slots if we are in hot standby and the primary
- * does not have a WAL level sufficient for logical decoding. No need
- * to search for potentially conflicting logically slots if standby is
- * running with wal_level lower than logical, because in that case, we
- * would have either disallowed creation of logical slots or
- * invalidated existing ones.
- */
- if (InRecovery && InHotStandby &&
- xlrec.wal_level < WAL_LEVEL_LOGICAL &&
- wal_level >= WAL_LEVEL_LOGICAL)
- InvalidateObsoleteReplicationSlots(RS_INVAL_WAL_LEVEL,
- 0, InvalidOid,
- InvalidTransactionId);
-
LWLockAcquire(ControlFileLock, LW_EXCLUSIVE);
ControlFile->MaxConnections = xlrec.MaxConnections;
ControlFile->max_worker_processes = xlrec.max_worker_processes;
@@ -8605,6 +8643,50 @@ xlog_redo(XLogReaderState *record)
{
/* nothing to do here, just for informational purposes */
}
+ else if (info == XLOG_LOGICAL_DECODING_STATUS_CHANGE)
+ {
+ bool logical_decoding;
+
+ /* Update the status on shared memory */
+ memcpy(&logical_decoding, XLogRecGetData(record), sizeof(bool));
+ UpdateLogicalDecodingStatus(logical_decoding, true);
+
+ if (InRecovery && InHotStandby)
+ {
+ if (!logical_decoding)

Like previously, shouldn't we have a check for standby's wal_level as
well due to the reasons mentioned in the removed comments?

3.
+ errmsg("logical decoding needs to be enabled to publish logical changes"),

This message doesn't sound intuitive. How about "logical decoding
should be allowed to publish logical changes"?

4.
+ else if (info == XLOG_LOGICAL_DECODING_STATUS_CHANGE)
...
+ /*
+ * Request to launch or shutdown the slotsync worker depending on
+ * the new logical decoding status.
+ */

If we see a similar part in existing code as a handling of
XLOG_PARAMETER_CHANGE, we don't shutdown or restart slotsync worker,
so why do it as part of this patch? This new behaviour may be better
but shouldn't we try to handle it as a separate HEAD patch? Also, a
few additional comments explaining the rationale behind this would be
good.

5.
Assert(RecoveryInProgress());
ereport(ERROR,
(errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
- errmsg("logical decoding on standby requires \"wal_level\" >=
\"logical\" on the primary")));
+ errmsg("logical decoding must be enabled on the primary")));

Can't we keep the tone of the existing message as it is? How about
"logical decoding on standby requires \"effective_wal_level\" >=
\"logical\" on the primary"? Also, if we agree with this, we could
have a similar change for other messages in the patch.

6. Can we write some comments as to why we didn't support wal_level to
be changed from minimal to logical? It will be helpful for future
readers/authors to understand what it would require to further extend
this functionality.

--
With Regards,
Amit Kapila.