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: shveta malik <shveta.malik@gmail.com>
Cc: Peter Smith <smithpb2250@gmail.com>, Amit Kapila <amit.kapila16@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-11-12T02:05:41Z
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 Mon, Nov 10, 2025 at 8:05 PM shveta malik <shveta.malik@gmail.com> wrote:
>
> On Thu, Nov 6, 2025 at 4:32 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> >
> >
> > I've updated and rebased the patch.
> >
>
> Thanks for the patch. Please find a few comments:
>
>
> 1)
> ReplicationSlotsDropDBSlots:
>
> + SpinLockAcquire(&s->mutex);
> + invalidated = s->data.invalidated == RS_INVAL_NONE;
> + SpinLockRelease(&s->mutex);
> +
> + /*
> + * Count slots on other databases too so we can disable logical
> + * decoding only if no slots in the cluster.
> + */
> + if (invalidated)
> + n_valid_logicalslots++;
>
>
> This seems confusing to me. Can we instead do:
>
> SpinLockAcquire(&s->mutex);
> if (s->data.invalidated == RS_INVAL_NONE)
> n_valid_logicalslots++;
> SpinLockRelease(&s->mutex);
>
> 2)
> InvalidateObsoleteReplicationSlots:
>
> + bool islogical = SlotIsLogical(s);
>
>   /* Prevent invalidation of logical slots during binary upgrade */
>   if (SlotIsLogical(s) && IsBinaryUpgrade)
> + {
> + SpinLockAcquire(&s->mutex);
> + if (s->data.invalidated == RS_INVAL_NONE)
> + n_valid_logicalslots++;
> + SpinLockRelease(&s->mutex);
> +
>   continue;
> + }
>
> We should use 'islogical' instead of SlotIsLogical here.
>
> 3)
> InvalidateObsoleteReplicationSlots() is more robust now as we are
> using both 'invalidated' and 'released_lock' flags but still nowhere
> we guarantee that invalidated=true implies released_lock=true. Since
> we jump to 'restart' label only if released_lock is true, it becomes
> important to have an ASSERT which says invalidated=true implicitly
> means released_lock=true or vice versa. Because at the end we go by
> 'invalidated_logical' rather than 'released_lock' to decide about
> logical-decoding disabling.
>
> In this logic:
>
> + if (InvalidatePossiblyObsoleteSlot(possible_causes, s, oldestLSN,
> +    dboid, snapshotConflictHorizon,
> +    &released_lock))
>   {
> - /* if the lock was released, start from scratch */
> - goto restart;
> + /* Remember we have invalidated a physical or logical slot */
> + invalidated = true;
> +
> + /*
> + * Additionally, remember we have invalidated a logical slot too
> + * as we can request disabling logical decoding later.
> + */
> + if (islogical)
> + invalidated_logical = true;
>   }
>
> Shall we have an Assert(released_lock) if
> InvalidatePossiblyObsoleteSlot returns true. Or any better way?
>
> 4)
> + SpinLockAcquire(&s->mutex);
> + if (s->data.invalidated == RS_INVAL_NONE)
> + n_valid_logicalslots++;
>
> In the same function, isn't the above code problematic: Don't we need
> 'islogical' check before incrementing 'n_valid_logicalslots',
> otherwise it may wrongly count valid physical slots as well.

Agreed with all the above points. Will fix and update the updated version.

Regards,

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