From: Antonin Houska <ah@cybertec.at>
To: Alvaro Herrera <alvherre@2ndquadrant.com>
cc: Thomas Munro <thomas.munro@gmail.com>,
    Robert Haas <robertmhaas@gmail.com>,
    "pgsql-hackers@postgresql.org" <pgsql-hackers@postgresql.org>
Subject: Re: Attempt to consolidate reading of XLOG page
In-reply-to: <20190923220010.GA23527@alvherre.pgsql>
References: <20190923220010.GA23527@alvherre.pgsql>
Comments: In-reply-to Alvaro Herrera <alvherre@2ndquadrant.com>
   message dated "Mon, 23 Sep 2019 19:00:10 -0300."
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-ID: <83391.1569339204.1@antos>
Content-Transfer-Encoding: quoted-printable
Date: Tue, 24 Sep 2019 17:33:24 +0200
Message-ID: <83392.1569339204@antos>
Precedence: bulk

Alvaro Herrera <alvherre@2ndquadrant.com> wrote:

> I spent a couple of hours on this patchset today.  I merged 0001 and
> 0002, and decided the result was still messier than I would have liked,
> so I played with it a bit more -- see attached.  I think this is
> committable, but I'm afraid it'll cause quite a few conflicts with the
> rest of your series.
> =

> I had two gripes, which I feel solved with my changes:
> =

> 1. I didn't like that "dir" and "wal segment size" were part of the
> "currently open segment" supporting struct.  It seemed that those two
> were slightly higher-level, since they apply to every segment that's
> going to be opened, not just the current one.

ok

> My first thought was to put those as members of XLogReaderState, but
> that doesn't work because the physical walsender.c code does not use
> xlogreader at all, even though it is reading WAL.

`I don't remember clearly but I think that this was the reason I tried to =
move
"wal_segment_size" away from XLogReaderState.

 =

> Separately from those two API-wise points, there was one bug which meant
> that with your 0002+0003 the recovery tests did not pass -- code
> placement bug.  I suppose the bug disappears with later patches in your
> series, which probably is why you didn't notice.  This is the fix for th=
at:
> =

> -   XLogRead(cur_page, state->seg.size, state->seg.tli, targetPagePtr,
> -   state->seg.tli =3D pageTLI;
> +   state->seg.ws_tli =3D pageTLI;
> +   XLogRead(cur_page, state->segcxt.ws_segsize, state->seg.ws_tli, targ=
etPagePtr,
>              XLOG_BLCKSZ);
> =


Yes, it seems so - the following parts ensure that XLogRead() adjusts the
timeline itself. I only checked that the each part of the series keeps the
source tree compilable. Thanks for fixing.

> ... Also, yes, I renamed all the struct members.
>
> =

> If you don't have any strong dislikes for these changes, I'll push this
> part and let you rebase the remains on top.

No objections here.

> 2. Not a fan of the InvalidTimeLineID stuff offhand.  Maybe it's okay ..=
.
> not convinced yet either way.

Well, it seems that the individual callbacks only use this constant in
Assert() statements. I'll consider if we really need it. The argument valu=
e
should not determine whether the callback derives the TLI or not.

-- =

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