Re: Attempt to consolidate reading of XLOG page

Kyotaro Horiguchi <horikyota(dot)ntt(at)gmail(dot)com> wrote:

> At Sat, 28 Sep 2019 15:00:35 +0200, Antonin Houska <ah(at)cybertec(dot)at> wrote in <9236(dot)1569675635(at)antos>
> > Antonin Houska <ah(at)cybertec(dot)at> wrote:
> >
> > > Alvaro Herrera <alvherre(at)2ndquadrant(dot)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.

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

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

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.

--
Antonin Houska
Web: https://www.cybertec-postgresql.com