On Fri, Oct 31, 2025 at 3:30 AM Masahiko Sawada <[email protected]> wrote: > > On Wed, Oct 29, 2025 at 8:12 PM Amit Kapila <[email protected]> wrote: > > > > On Thu, Oct 30, 2025 at 12:56 AM Masahiko Sawada <[email protected]> > > wrote: > > > > > > On Wed, Oct 29, 2025 at 5:10 AM Amit Kapila <[email protected]> > > > wrote: > > > > > > > > > > > > 8. > > > > @@ -136,6 +137,12 @@ create_logical_replication_slot(char *name, char > > > > *plugin, > > > > temporary ? RS_TEMPORARY : RS_EPHEMERAL, two_phase, > > > > failover, false); > > > > > > > > + /* > > > > + * Ensure the logical decoding is enabled before initializing the > > > > logical > > > > + * decoding context. > > > > + */ > > > > + EnsureLogicalDecodingEnabled(); > > > > … > > > > > > > > EnsureLogicalDecodingEnabled(void) > > > > { > > > > Assert(MyReplicationSlot); > > > > > > > > if (wal_level != WAL_LEVEL_REPLICA) > > > > return; > > > > > > > > /* Prepare and start the activation process if it's disabled */ > > > > if (!start_logical_decoding_status_change(true)) > > > > return; > > > > > > > > I see that EnsureLogicalDecodingEnabled() sometimes returns without > > > > enabling decoding (like when wal_level is not a replica). It is > > > > possible that the same is not possible in this code flow, but won't it > > > > be better to get the return value and assert if this function returns > > > > false? > > > > > > Do you mean to check the value returned by > > > start_logical_decoding_change()? I could not understand what assertion > > > we can put there. > > > > > > > I mean to check the return value of EnsureLogicalDecodingEnabled() > > because there are two code paths as quoted in my message from where if > > this function returned, the logical decoding won't be enabled. > > > > > Related to this point, I thought it might be better to raise an error > > > if this function is called when wal_level='minimal' instead of doing > > > nothing because it cannot ensure logical decoding enabled by calling > > > this function. > > > > > > > Yeah, that will also work for one of the code paths in > > EnsureLogicalDecodingEnabled() but I think we can still return without > > enabling the decoding from the other code path quoted in my message. > > There are four cases EnsureLogicalDecodingEnabled() returns without > changing the status (i.e., when start_logical_decoding_status_change() > returns false): > > 1. wal_level is 'minimal'. > 2. wal_level is 'logical'. > 3. wal_level is 'replica', but during recovery (the logical decoding > status depends on > 4. wal_level is 'replica', but logical decoding is already enabled. > > Among these four cases, case 1 is the case where logical decoding is > not available even though we call EnsureLogicalDecodingEnabled(). > Also, in the case 3, the function could just return and logical > decoding is not enabled if the primary doesn't enable logical > decoding. > > The code you quoted is the change in > create_logical_replication_slot(), and the both cases don't happen > there since we already passed CheckLogicalDecodingRequirements() > check. IIUC your comment is meant to check if logical decoding is > really enabled after calling EnsureLogicalDecodingEnabled(). Is that > right? >
Yes. > If so, how about putting an assertion like > 'Assert(IsLogicalDecodingEnabled())' after calling > EnsureLogicalDecodingEnabled() where needed? > WFM. -- With Regards, Amit Kapila.
