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: <7753.1569658465@antos>
References: <20190927191736.GA13447@alvherre.pgsql> <7753.1569658465@antos>
Comments: In-reply-to Antonin Houska <ah@cybertec.at>
   message dated "Sat, 28 Sep 2019 10:14:25 +0200."
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-ID: <9235.1569675635.1@antos>
Date: Sat, 28 Sep 2019 15:00:35 +0200
Message-ID: <9236.1569675635@antos>
Precedence: bulk

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.

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.

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.

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