Re: Minimal logical decoding on standbys

Hi,

On 4/5/23 1:59 PM, Amit Kapila wrote:
> On Wed, Apr 5, 2023 at 3:58 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>>
>> On Wed, Apr 5, 2023 at 2:41 PM Drouvot, Bertrand
>> <bertranddrouvot.pg@gmail.com> wrote:
>>
>> minor nitpick:
>> +
>> + /* Intentional fall through to session cancel */
>> + /* FALLTHROUGH */
>>
>> Do we need to repeat fall through twice in different ways?
>>
> 
> Few minor comments on 0003:
> ========================
> 1.
> + case XLOG_PARAMETER_CHANGE:
> + {
> + xl_parameter_change *xlrec =
> + (xl_parameter_change *) XLogRecGetData(buf->record);
> +
> + /*
> + * If wal_level on primary is reduced to less than logical,
> + * then we want to prevent existing logical slots from being
> + * used. Existing logical slots on standby get invalidated
> + * when this WAL record is replayed; and further, slot
> + * creation fails when the wal level is not sufficient; but
> + * all these operations are not synchronized, so a logical
> + * slot may creep in while the wal_level is being reduced.
> + * Hence this extra check.
> + */
> + if (xlrec->wal_level < WAL_LEVEL_LOGICAL)
> + ereport(ERROR,
> + (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
> + errmsg("logical decoding on standby requires wal_level to be at
> least logical on the primary server")));
> 
> By looking at this change, it is not very clear that this can occur
> only on standby. I understand that on primary, we will not allow
> restarting the server after changing wal_level if there is a
> pre-existing slot but still this looks a bit odd. Shall we have an
> Assert to indicate that this will occur only on standby?

I think that's a fair point. Adding an Assert and a comment before the
Assert in V61 attached.

> 
> 2.
> /*
> - * Since logical decoding is only permitted on a primary server, we know
> - * that the current timeline ID can't be changing any more. If we did this
> - * on a standby, we'd have to worry about the values we compute here
> - * becoming invalid due to a promotion or timeline change.
> + * Since logical decoding is also permitted on a standby server, we need
> + * to check if the server is in recovery to decide how to get the current
> + * timeline ID (so that it also cover the promotion or timeline change
> + * cases).
>    */
> +
> + /* make sure we have enough WAL available */
> + flushptr = WalSndWaitForWal(targetPagePtr + reqLen);
> +
> + /* the standby could have been promoted, so check if still in recovery */
> + am_cascading_walsender = RecoveryInProgress();
> 
> The first part of the comment explains why it is important to check
> RecoveryInProgress() and then immediately after that, the patch
> invokes WalSndWaitForWal(). It may be better to move the comment after
> WalSndWaitForWal() invocation.

Good catch, thanks! done in V61.

> Also, it will be better to write a
> comment as to why you need to do WalSndWaitForWal() before retrieving
> the current timeline as previously that was done afterward.
> 

Agree, done in V61.

Regards,

-- 
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com