Re: Minimal logical decoding on standbys

Hi,

On 1/6/23 4:40 AM, Andres Freund wrote:
> Hi,
> On 2023-01-05 16:15:39 -0500, Robert Haas wrote:
>> On Tue, Jan 3, 2023 at 2:42 AM Drouvot, Bertrand
>> <bertranddrouvot.pg@gmail.com> wrote:

> 0003:
> 
>> Allow a logical slot to be created on standby. Restrict its usage
>> or its creation if wal_level on primary is less than logical.
>> During slot creation, it's restart_lsn is set to the last replayed
>> LSN. Effectively, a logical slot creation on standby waits for an
>> xl_running_xact record to arrive from primary. Conflicting slots
>> would be handled in next commits.
> 
> I think the commit message might be outdated, the next commit is a test.

Oops, thanks, fixed in V38 attached.

> 
>> +            /*
>> +             * Replay pointer may point one past the end of the record. If that
>> +             * is a XLOG page boundary, it will not be a valid LSN for the
>> +             * start of a record, so bump it up past the page header.
>> +             */
>> +            if (!XRecOffIsValid(restart_lsn))
>> +            {
>> +                if (restart_lsn % XLOG_BLCKSZ != 0)
>> +                    elog(ERROR, "invalid replay pointer");
>> +
>> +                /* For the first page of a segment file, it's a long header */
>> +                if (XLogSegmentOffset(restart_lsn, wal_segment_size) == 0)
>> +                    restart_lsn += SizeOfXLogLongPHD;
>> +                else
>> +                    restart_lsn += SizeOfXLogShortPHD;
>> +            }
> 
> Is this actually needed? Supposedly xlogreader can work just fixe with an
> address at the start of a page?
> 
>         /*
>          * Caller supplied a position to start at.
>          *
>          * In this case, NextRecPtr should already be pointing either to a
>          * valid record starting position or alternatively to the beginning of
>          * a page. See the header comments for XLogBeginRead.
>          */
>         Assert(RecPtr % XLOG_BLCKSZ == 0 || XRecOffIsValid(RecPtr));
> 


Oh you're right, thanks, indeed that's not needed anymore now that XLogDecodeNextRecord() exists.
Removed in V38.

> 
>>       /*
>> -     * 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).
>>        */
>> +    if (!RecoveryInProgress())
>> +        currTLI = GetWALInsertionTimeLine();
>> +    else
>> +        GetXLogReplayRecPtr(&currTLI);
>> +
> 
> This seems to remove some content from the !recovery case.
> 
> It's a bit odd that here RecoveryInProgress() is used, whereas further down
> am_cascading_walsender is used.
> 
> 

Agree, using am_cascading_walsender instead in V38.


>> @@ -3074,10 +3078,12 @@ XLogSendLogical(void)
>>        * If first time through in this session, initialize flushPtr.  Otherwise,
>>        * we only need to update flushPtr if EndRecPtr is past it.
>>        */
>> -    if (flushPtr == InvalidXLogRecPtr)
>> -        flushPtr = GetFlushRecPtr(NULL);
>> -    else if (logical_decoding_ctx->reader->EndRecPtr >= flushPtr)
>> -        flushPtr = GetFlushRecPtr(NULL);
>> +    if (flushPtr == InvalidXLogRecPtr ||
>> +        logical_decoding_ctx->reader->EndRecPtr >= flushPtr)
>> +    {
>> +        flushPtr = (am_cascading_walsender ?
>> +                    GetStandbyFlushRecPtr(NULL) : GetFlushRecPtr(NULL));
>> +    }
>>
>>       /* If EndRecPtr is still past our flushPtr, it means we caught up. */
>>       if (logical_decoding_ctx->reader->EndRecPtr >= flushPtr)
> 
> A short if inside a normal if seems ugly to me.
> 

Using 2 normal if in v38.

Please find V38 attached, I'll look at the other comments you've done in [1] on 0004 and 0006.

Regards,

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

[1]: https://www.postgresql.org/message-id/20230106034036.2m4qnn7ep7b5ipet%40awork3.anarazel.de