Thanks Dilip and Bharath for the review.
I am working on review comments and will post an updated patch.
On Wed, 10 Nov 2021 at 15:31, Bharath Rupireddy
<bharath.rupireddyforpostg...@gmail.com> wrote:
>
> On Wed, Oct 27, 2021 at 1:02 AM Mahendra Singh Thalor
> <mahi6...@gmail.com> wrote:
> > On Mon, 3 Aug 2020 at 15:02, Daniel Gustafsson <dan...@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.
--
Thanks and Regards
Mahendra Singh Thalor
EnterpriseDB: http://www.enterprisedb.com
