Re: Attempt to consolidate reading of XLOG page

Hello,

At Sat, 28 Sep 2019 15:00:35 +0200, Antonin Houska <ah@cybertec.at> wrote in <9236.1569675635@antos>
> Antonin Houska <ah@cybertec.at> wrote:
> 
> > Alvaro Herrera <alvherre@2ndquadrant.com> wrote:
> > 
> > > BTW that tli_p business to the openSegment callback is horribly
> > > inconsistent.  Some callers accept a NULL tli_p, others will outright
> > > crash, even though the API docs say that the callback must determine the
> > > timeline.  This is made more complicated by us having the TLI in "seg"
> > > also.  Unless I misread, the problem is again that the walsender code is
> > > doing nasty stuff with globals (endSegNo).  As a very minor stylistic
> > > point, we prefer to have out params at the end of the signature.
> > 
> > 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. 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.  So, it seems to me that the parameter
doesn't need to be inout? It is enough that it is an "in"
parameter.

> 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.)

> Another approach is to add a boolean argument "check_tli", but that still
> forces caller to pass some (random) value of the tli. The concept of
> InvalidTimeLineID seems to me less disturbing than this.

So I think InvalidTimeLineID is not needed.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center