Horton

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

Masahiko Sawada <sawada.mshk@gmail.com>

View thread
From: Masahiko Sawada <sawada.mshk@gmail.com>
To: Peter Smith <smithpb2250@gmail.com>
Cc: Amit Kapila <amit.kapila16@gmail.com>, shveta malik <shveta.malik@gmail.com>, "Hayato Kuroda (Fujitsu)" <kuroda.hayato@fujitsu.com>, Ashutosh Bapat <ashutosh.bapat.oss@gmail.com>, Shlok Kyal <shlok.kyal.oss@gmail.com>, Bertrand Drouvot <bertranddrouvot.pg@gmail.com>, PostgreSQL-development <pgsql-hackers@postgresql.org>
Date: 2025-12-01T05:50:07Z
Lists: pgsql-hackers

Commits

Same data as JSON: GET /api/v1/messages/:b64id/commits the thread's linked commits as JSON, with link sources. API reference →

  1. Fix regression test failure when wal_level is set to minimal.

  2. Toggle logical decoding dynamically based on logical slot presence.

  3. Disallow server start with sync_replication_slots = on and wal_level < logical.

On Thu, Nov 27, 2025 at 12:33 AM Peter Smith <smithpb2250@gmail.com> wrote:
>
> Hi Sawada-San.
>
> Some review comments for v30-0001.

Thank you for the comments!

> ======
> src/backend/replication/logical/logicalctl.c
>
> 5.
> + * In the future, we could extend support to include automatic transitions
>
> Sawada-San reply to previous "XXX" comment:
> Hmm, I've seen we use an "XXX" marker like "TODO" or "FIXME", but what
> the comment explains is not actually a todo but possible future
> enhancements.
>
> ~
>
> You can search the source code for regex “XXX.*future” to find
> multiple examples just like this.

Yes, but I can also see some examples where we don't have "XXX"
marker. So I don't see why we should add it in this patch.

> ~
>
> 6b.
> Would it be better to have a self-documenting enum {UNKNOWN, ENABLED,
> DISABLED} instead of some magic int values that need lots of comments
> explaining how to use them?

No, we should not cast enum values directly to a boolean. We have a
precedent of such usage like LocalXLogInsertAllowed, and I don't think
we can reduce the comments even if we use a self-documenting enum
since the usage is complicated. We could consider #define values 0, 1,
and -1 but it seems to be overkill to this usage.

>
> ~~~
>
> LogicalDecodingCtlShmemInit:
>
> 7.
> +void
> +LogicalDecodingCtlShmemInit(void)
> +{
> + bool found;
> +
> + LogicalDecodingCtl = ShmemInitStruct("Logical decoding control",
> + LogicalDecodingCtlShmemSize(),
> + &found);
> +
> + if (!found)
> + MemSet(LogicalDecodingCtl, 0, LogicalDecodingCtlShmemSize());
> +}
>
> Maybe assign LogicalDecodingCtlShmemSize() to some variable; no need
> to invoke it 2x.

It's actually inlined so we don't need to have an additional variable here .

> InvalidatePossiblyObsoleteSlot:
>
> 11.
>   /* Let caller know */
> - *invalidated = true;
> + invalidated = true;
>
> This was a review v29 (#12) review comment that seems to have been
> missed. That comment is stale (should be removed) because
> 'invalidated' is no longer returned by reference like before.

Yes, but it doesn't change the fact that the function lets the caller
know using this variable if the slot has been invalidated, no?

Regards,

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