Re: Attempt to consolidate reading of XLOG page

At Tue, 01 Oct 2019 08:28:03 +0200, Antonin Houska <ah@cybertec.at> wrote in <2188.1569911283@antos>
> Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote:
> > > > XLogRead() tests for NULL so it should not crash but I don't insist on doing
> > > > it this way. XLogRead() actually does not have to care whether the "open
> > > > segment callback" determines the TLI or not, so it (XLogRead) can always
> > > > receive a valid pointer to seg.ws_tli.
> > > 
> > > This is actually wrong - seg.ws_tli is not always the correct value to
> > > pass. If seg.ws_tli refers to the segment from which data was read last time,
> > > then XLogRead() still needs a separate argument to specify from which TLI the
> > > current call should read. If these two differ, new file needs to be opened.
> > 
> > openSegment represents the file *currently* opened.
> 
> I suppose you mean the "seg" argument.
> 
> > XLogRead needs the TLI *to be* opened. If they are different, as far as wal
> > logical wal sender and pg_waldump is concerned, XLogRead switches to the new
> > TLI and the new TLI is set to openSegment.ws_tli.
> 
> Yes, it works in these cases.
> 
> > So, it seems to me that the parameter doesn't need to be inout? It is enough
> > that it is an "in" parameter.
> 
> I did consider "TimeLineID *tli_p" to be "in" parameter in the last patch
> version. The reason I used pointer was the special meaning of the NULL value:
> if NULL is passed, then the timeline should be ignored (because of the other
> cases, see below).

Understood.

> > > The problem of walsender.c is that its implementation of XLogRead() does not
> > > care about the TLI of the previous read. If the behavior of the new, generic
> > > implementation should be exactly the same, we need to tell XLogRead() that in
> > > some cases it also should not compare the current TLI to the previous
> > > one. That's why I tried to use the NULL pointer, or the InvalidTimeLineID
> > > earlier.
> > 
> > Physical wal sender doesn't switch TLI. So I don't think the
> > behavior doesn't harm (or doesn't fire). openSegment holds the
> > TLI set at the first call. (Even if future wal sender switches
> > TLI, the behavior should be needed.)
> 
> Note that walsender.c:XLogRead() has no TLI argument, however the XLogRead()
> introduced by the patch does have one. What should be passed for TLI to the
> new implementation if it's called from walsender.c? If the check for a segment
> change looks like this (here "tli" is the argument representing the desired
> TLI)

TLI is mandatory to generate a wal file name so it must be passed
to the function anyways. In the current code it is sendTimeLine
for the walsender.c:XLogRead(). logical_read_xlog_page sets the
variable very time immediately before calling
XLogRead(). CreateReplicationSlot and StartReplication set the
variable to desired TLI immediately before calling and once it is
set by StartReplication, it is not changed by XLogSendPhysical
and wal sender ends at the end of the current timeline. In the
XLogRead, the value is copied to sendSeg->ws_tli when the file
for the new timeline is read.

>     if (seg->ws_file < 0 ||
>         !XLByteInSeg(recptr, seg->ws_segno, segcxt->ws_segsize) ||
>         tli != seg->ws_tli)
>     {
>         XLogSegNo    nextSegNo;
> 
>         /* Switch to another logfile segment */
>         if (seg->ws_file >= 0)
>             close(seg->ws_file);
> 
> then any valid TLI can result in accidental closing of the current segment
> file. Since this is only refactoring patch, we should not allow such a change
> of behavior even if it seems that the same segment will be reopened
> immediately.

Mmm. ws_file must be -1 in the case? tli != seg->ws_tli is true
but seg->ws_file < 0 is also always true at the time. In other
words, the "tli != seg->ws_tli" is not even evaluated.

If wal sender had an open file (ws_file >= 0) and the new TLI is
different from ws_tli, it would be the sign of a serious bug.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center