From: Antonin Houska <ah@cybertec.at>
To: Kyotaro Horiguchi <horikyota.ntt@gmail.com>
cc: alvherre@2ndquadrant.com, thomas.munro@gmail.com,
    robertmhaas@gmail.com, pgsql-hackers@postgresql.org
Subject: Re: Attempt to consolidate reading of XLOG page
In-reply-to: <20191001.122227.209046280.horikyota.ntt@gmail.com>
References: <20190927191736.GA13447@alvherre.pgsql> <7753.1569658465@antos>
 <9236.1569675635@antos> <20191001.122227.209046280.horikyota.ntt@gmail.com>
Comments: In-reply-to Kyotaro Horiguchi <horikyota.ntt@gmail.com>
   message dated "Tue, 01 Oct 2019 12:22:27 +0900."
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-ID: <2187.1569911283.1@antos>
Content-Transfer-Encoding: quoted-printable
Date: Tue, 01 Oct 2019 08:28:03 +0200
Message-ID: <2188.1569911283@antos>
Precedence: bulk

Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote:

> At Sat, 28 Sep 2019 15:00:35 +0200, Antonin Houska <ah@cybertec.at> wrot=
e 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 outri=
ght
> > > > crash, even though the API docs say that the callback must determi=
ne 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 c=
ode is
> > > > doing nasty stuff with globals (endSegNo).  As a very minor stylis=
tic
> > > > 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 al=
ways
> > > 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 las=
t 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 op=
ened.
> =

> 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 en=
ough
> 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 val=
ue:
if NULL is passed, then the timeline should be ignored (because of the oth=
er
cases, see below).

> > The problem of walsender.c is that its implementation of XLogRead() do=
es not
> > care about the TLI of the previous read. If the behavior of the new, g=
eneric
> > 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 InvalidTimeLin=
eID
> > 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 th=
e
new implementation if it's called from walsender.c? If the check for a seg=
ment
change looks like this (here "tli" is the argument representing the desire=
d
TLI)

	if (seg->ws_file < 0 ||
		!XLByteInSeg(recptr, seg->ws_segno, segcxt->ws_segsize) ||
		tli !=3D seg->ws_tli)
	{
		XLogSegNo	nextSegNo;

		/* Switch to another logfile segment */
		if (seg->ws_file >=3D 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 cha=
nge
of behavior even if it seems that the same segment will be reopened
immediately.

-- =

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