Re: Replication & recovery_min_apply_delay
От | Mahendra Singh Thalor |
---|---|
Тема | Re: Replication & recovery_min_apply_delay |
Дата | |
Msg-id | CAKYtNAqjDChFH0aktG9HFQ30ng5fXe4wo=aHXjWUkn1FemA__Q@mail.gmail.com обсуждение исходный текст |
Ответ на | Re: Replication & recovery_min_apply_delay (Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>) |
Список | pgsql-hackers |
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.rupireddyforpostgres@gmail.com> wrote: > > 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. -- Thanks and Regards Mahendra Singh Thalor EnterpriseDB: http://www.enterprisedb.com
В списке pgsql-hackers по дате отправления:
Предыдущее
От: Bharath RupireddyДата:
Сообщение: Re: Trap errors from streaming child in pg_basebackup to exit early
Следующее
От: Bharath RupireddyДата:
Сообщение: Re: Inconsistency in startup process's MyBackendId and procsignal array registration with ProcSignalInit()