On Fri, Oct 24, 2025 at 4:48 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Fri, Oct 24, 2025 at 3:23 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> >
> > On Thu, Oct 23, 2025 at 11:52 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> >
> > I've addressed the above comments and made some cosmetic changes.
> >
>
> Below are few comments/questions:
Thank you for reviewing the patch!
>
> 1.
> + <para>
> + When <varname>wal_level</varname> is set to <literal>replica</literal>,
> + dropping the last logical slot may disable logical decoding on
> the primary,
>
> In this sentence, using 'may' sounds wrong because we will disable
> logical decoding on primary. Is there any case where the same won't
> happen?
>
> 2.
> * don't include the new tuple in the WAL record in that case. Also
> - * disable if wal_level='logical', as logical decoding needs to be able to
> + * disable if logical decoding is enabled and the relation requires WAL to
> + * be logged for logical decoding, as logical decoding needs to be able to
> * read the new tuple in whole from the WAL record alone.
>
> Using logical decoding in the above sentence thrice sounds redundant.
> Can we avoid its second usage?
>
> 3.
> + /*
> + * During the recovery, effective_wal_level inherits on the primary's
>
> 'on' sounds redundant in the above sentence.
>
> 4.
>
> +/*
> + * Initialize the logical decoding status on shmem at server startup.
> …
> …
> +/*
> + * Enable or disable both status of logical info WAL logging and
> logical decoding
> + * on shared memory.
> +
>
> In both the above comments, instead of on shmem, it seems in shmem
> should be used.
> /both status/both the status
Fixed the above points.
>
> 5.
> +bool
> +CheckLogicalSlotExists(void)
> {
> …
> + /* NB: counting invalidated slots */
> +
> + if (SlotIsLogical(s))
>
> Why can't we avoid counting invalid slots? I think this needs more
> comments. BTW, shouldn't this patch consider changing
> effective_wal_level when the last logical slot is invalidated?
> Ideally, when logical decoding is not possible in the system, then we
> can reduce the wal_level back to replica, no?
Hmm, good point. I've considered this idea before but I didn't
implement it probably because it makes the code more complex. But
thinking of this idare carefully, it doesn't seem too complex. I've
implemented this part as a separate patch to make reviews easy. I'll
merge them if it looks good.
>
> 6.
> Therefore, there
> + * should not be any operations that could wait for the checkpointer until
> + * then.
> + */
> + UpdateLogicalDecodingStatusEndOfRecovery();
> +
>
> This part of the comment is not clear to me. Can you please try to
> explain with some context?
Fixed.
Regards,
--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com
