Re: Replication & recovery_min_apply_delay

On Wed, Oct 27, 2021 at 1:02 AM Mahendra Singh Thalor
<mahi6run@gmail.com> wrote:
> On Mon, 3 Aug 2020 at 15:02, Daniel Gustafsson <daniel@yesql.se> wrote:
>>
>> This thread has stalled and the patch no longer applies, so I'm marking this
>> Returned with Feedback.  Please feel free to open a new entry if this patch is
>> revived.
>>
>> cheers ./daniel
>>
>
> Hi all,
> I took v3[1] patch from this thread and re-based on commit head(5fedf7417b69295294b154a21).
>
> Please find the attached patch for review.

Thanks for reviving this patch. Here are some comments:

1) I don't think we need lseek() to the beginning of the file right
after XLogFileReadAnyTLI as the open() sys call will ensure that the
fd is set to the beginning of the file.
+ fd = XLogFileReadAnyTLI(segno, LOG, XLOG_FROM_PG_WAL);
+ if (fd == -1)
+ return -1;
+
+ /* Seek to the beginning, we want to check if the first page is valid */
+ if (lseek(fd, (off_t) 0, SEEK_SET) < 0)
+ {
+ XLogFileName(xlogfname, ThisTimeLineID, segno, wal_segment_size);

2) How about saying "starting WAL receiver while redo in progress,
startpoint %X/%X"," something like this? Because the following message
looks like we are starting the WAL receiver for the first time, just
to differentiate this with the redo case.
+ elog(LOG, "starting WAL receiver, startpoint %X/%X",

3) Although, WAL_RCV_START_AT_CATCHUP has been defined and used as
default for wal_receiver_start_condition GUC, are we starting wal
receiver when this is set? We are doing it only when
WAL_RCV_START_AT_CONSISTENCY. If we were to start the WAL receiver
even for WAL_RCV_START_AT_CATCHUP, let's have the LOG message (2)
clearly say this.

4) I think the default value for wal_receiver_start_condition GUC can
be WAL_RCV_START_AT_CONSISTENCY as it starts streaming once it reaches
a consistent state.

5) Should it be StandbyMode instead of StandbyModeRequested? I'm not
sure if it does make a difference.
+ if (StandbyModeRequested &&

6) Documentation missing for the new GUC.

7) Should we just collect the output of the read() and use it in the
if condition? It will be easier for debugging to know how many bytes
the read() returns.
+ if (read(fd, readBuf, XLOG_BLCKSZ) != XLOG_BLCKSZ)

8) I think we should be using new style ereport(LOG, than elog(LOG,

9) Can't we place this within an inline function for better
readability and reusability if needed?
+ if ((longhdr->std.xlp_info & XLP_LONG_HEADER) == 0 ||
+ (longhdr->std.xlp_magic != XLOG_PAGE_MAGIC) ||
+ ((longhdr->std.xlp_info & ~XLP_ALL_FLAGS) != 0) ||
+ (longhdr->xlp_sysid != ControlFile->system_identifier) ||
+ (longhdr->xlp_seg_size != wal_segment_size) ||
+ (longhdr->xlp_xlog_blcksz != XLOG_BLCKSZ) ||
+ (longhdr->std.xlp_pageaddr != lsn) ||
+ (longhdr->std.xlp_tli != ThisTimeLineID))
+ {

10) I don't think we need all the logs to be elog(LOG, which might
blow up the server logs. Try to have a one single message with LOG
that sort of gives information like whether the wal receiver is
started or not, the startpoint, the last valid segment number and so
on. The detailed message can be at DEBUG1 level.

11) I think you should use LSN_FORMAT_ARGS instead of
+ (uint32) (lsn >> 32), (uint32) lsn);
+ (uint32) (startpoint >> 32), (uint32) startpoint);

Regards,
Bharath Rupireddy.