Date: Fri, 27 Sep 2019 10:22:10 -0300
From: Alvaro Herrera <alvherre@2ndquadrant.com>
To: Antonin Houska <ah@cybertec.at>
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
Message-ID: <20190927132210.GA18194@alvherre.pgsql>
MIME-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <75115.1569499713@antos>
User-Agent: Mutt/1.9.4 (2018-02-28)
Precedence: bulk

On 2019-Sep-26, Antonin Houska wrote:

> One comment on the remaining part of the series:
> 
> Before this refactoring, the walsender.c:XLogRead() function contained these
> lines
> 
>        /*
>         * After reading into the buffer, check that what we read was valid. We do
>         * this after reading, because even though the segment was present when we
>         * opened it, it might get recycled or removed while we read it. The
>         * read() succeeds in that case, but the data we tried to read might
>         * already have been overwritten with new WAL records.
>         */
>        XLByteToSeg(startptr, segno, segcxt->ws_segsize);
>        CheckXLogRemoved(segno, ThisTimeLineID);
> 
> but they don't fit into the new, generic implementation, so I copied these
> lines to the two places right after the call of the new XLogRead(). However I
> was not sure if ThisTimeLineID was ever correct here. It seems the original
> walsender.c:XLogRead() implementation did not update ThisTimeLineID (and
> therefore neither the new callback WalSndSegmentOpen() does), so both
> logical_read_xlog_page() and XLogSendPhysical() could read the data from
> another (historic) timeline. I think we should check the segment we really
> read data from:
> 
> 	CheckXLogRemoved(segno, sendSeg->ws_tli);

Hmm, okay.  I hope we can get rid of ThisTimeLineID one day.

You placed the errinfo in XLogRead's stack rather than its callers' ...
I don't think that works, because as soon as XLogRead returns that
memory is no longer guaranteed to exist.  You need to allocate the
struct in the callers stacks and pass its address to XLogRead.  XLogRead
can return NULL if everything's okay or the pointer to the errinfo
struct.

I've been wondering if it's really necessary to pass 'seg' to the
openSegment() callback.  Only walsender wants that, and it seems ...
weird.  Maybe that's not something for this patch series to fix, but it
would be good to find a more decent way to do the TLI switch at some
point.

> +			/*
> +			 * If the function is called by the XLOG reader, the reader will
> +			 * eventually set both "ws_segno" and "ws_off", however the XLOG
> +			 * reader is not necessarily involved. Furthermore, we need to set
> +			 * the current values for this function to work.
> +			 */
> +			seg->ws_segno = nextSegNo;
> +			seg->ws_off = 0;

Why do we leave this responsibility to ReadPageInternal?  Wouldn't it
make more sense to leave XLogRead be always responsible for setting
these correctly, and remove those lines from ReadPageInternal?  (BTW "is
called by the XLOG reader" is a bit strange in code that appears in
xlogreader.c).

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services