On Tue, Nov 27, 2018 at 3:46 AM Petr Jelinek <petr.jeli...@2ndquadrant.com> wrote: > > Hi, > > On 26/11/2018 01:29, Masahiko Sawada wrote: > > On Sun, Nov 25, 2018 at 12:27 AM Petr Jelinek > > <petr.jeli...@2ndquadrant.com> wrote: > >> > >> The more serious thing is: > >> > >>> + if (MyReplicationSlot) > >>> + ReplicationSlotRelease(); > >>> + > >>> + /* Release the saved slot if exist while preventing double > >>> releasing */ > >>> + if (savedslot && savedslot != MyReplicationSlot) > >> > >> This won't work as intended as the ReplicationSlotRelease() will set > >> MyReplicationSlot to NULL, you might need to set aside MyReplicationSlot > >> to yet another temp variable inside this function prior to releasing it. > >> > > > > You're right. I've fixed it by checking if we need to release the > > saved slot before releasing the origin slot. Is that right? > > Attached an updated patch. > > > > Sounds good. > > I do have one more minor gripe after reading though again: >
Thank you for the review comment and sorry for the late response. > > + > > + /* > > + * The requested wal lsn is no longer available. We don't > > want to retry > > + * it, so raise an error. > > + */ > > + if (!XLogRecPtrIsInvalid(requested_lsn)) > > + { > > + char filename[MAXFNAMELEN]; > > + > > + XLogFileName(filename, ThisTimeLineID, segno, > > wal_segment_size); > > + ereport(ERROR, > > + (errmsg("could not reserve WAL > > segment %s", filename))); > > + } > > I would reword the comment to something like "The caller has requested a > specific wal lsn which we failed to reserve. We can't retry here as the > requested wal is no longer available." (It took me a while to understand > this part). > > Also the ereport should have errcode as it's going to be thrown to user > sessions and it might be better if the error itself used same wording as > CheckXLogRemoved() and XLogRead() for consistency. What do you think? > I agreed your both comments. I've changed the above comment and ereport. Attached the updated version patch. Regards, -- Masahiko Sawada NIPPON TELEGRAPH AND TELEPHONE CORPORATION NTT Open Source Software Center
v8-0001-Copy-function-for-logical-and-physical-replicatio.patch
Description: Binary data