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(dot)pg(at)gmail(dot)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