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