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.174822.208675030.horikyota.ntt@gmail.com>
References: <9236.1569675635@antos>
 <20191001.122227.209046280.horikyota.ntt@gmail.com> <2188.1569911283@antos>
 <20191001.174822.208675030.horikyota.ntt@gmail.com>
Comments: In-reply-to Kyotaro Horiguchi <horikyota.ntt@gmail.com>
   message dated "Tue, 01 Oct 2019 17:48:22 +0900."
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-ID: <27139.1570000570.1@antos>
Content-Transfer-Encoding: quoted-printable
Date: Wed, 02 Oct 2019 09:16:10 +0200
Message-ID: <27140.1570000570@antos>
Precedence: bulk

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

> At Tue, 01 Oct 2019 08:28:03 +0200, Antonin Houska <ah@cybertec.at> wrot=
e in <2188.1569911283@antos>
> > Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote:

> > > > 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 ne=
w, generic
> > > > implementation should be exactly the same, we need to tell XLogRea=
d() that in
> > > > some cases it also should not compare the current TLI to the previ=
ous
> > > > one. That's why I tried to use the NULL pointer, or the InvalidTim=
eLineID
> > > > 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 XLog=
Read()
> > introduced by the patch does have one. What should be passed for TLI t=
o the
> > new implementation if it's called from walsender.c? I f the check for =
a segment
> > change looks like this (here "tli" is the argument representing the de=
sired
> > 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.

Are you saying that we should pass sendTimeLine to XLogRead()? I think it'=
s
not always correct because sendSeg->ws_tli is sometimes assigned
sendTimeLineNextTLI, so the test "tli !=3D seg->ws_tli" in

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

could pass occasionally.

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

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

So we can probably pass ws_tli as the "new TLI" when calling the new
XLogRead() from walsender.c. Is that what you try to say? I need to think
about it more but it sounds like a good idea.

-- =

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