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: <20190927132210.GA18194@alvherre.pgsql>
References: <20190927132210.GA18194@alvherre.pgsql>
Comments: In-reply-to Alvaro Herrera <alvherre@2ndquadrant.com>
   message dated "Fri, 27 Sep 2019 10:22:10 -0300."
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-ID: <5989.1569606865.1@antos>
Date: Fri, 27 Sep 2019 19:54:25 +0200
Message-ID: <5990.1569606865@antos>
Precedence: bulk

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

> On 2019-Sep-26, Antonin Houska wrote:

> 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.

I was aware of this problem, therefore I defined the field as static:

+XLogReadError *
+XLogRead(char *buf, XLogRecPtr startptr, Size count, TimeLineID *tli_p,
+                WALOpenSegment *seg, WALSegmentContext *segcxt,
+                WALSegmentOpen openSegment)
+{
+       char       *p;
+       XLogRecPtr      recptr;
+       Size            nbytes;
+       static XLogReadError errinfo;

> 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 didn't choose this approach because that would add one more argument to the
function.

> 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.

Good point. Since walsender.c already has the "sendSeg" global variable, maybe
we can let WalSndSegmentOpen() use this one, and remove the "seg" argument
from the callback.

> > +			/*
> > +			 * 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?

I think there's no rule that ReadPageInternal() must use XLogRead(). If we do
what you suggest, we need make this responsibility documented. I'll consider
that.

> (BTW "is called by the XLOG reader" is a bit strange in code that appears in
> xlogreader.c).

ok, "called by XLogPageReadCB callback" would be more accurate. Not sure if
we'll eventually need this phrase in the comment at all.

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