Date: Fri, 27 Sep 2019 16:17:36 -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: <20190927191736.GA13447@alvherre.pgsql>
MIME-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <5990.1569606865@antos>
User-Agent: Mutt/1.9.4 (2018-02-28)
Precedence: bulk

On 2019-Sep-27, 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;

I see.

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

Yeah, the signature does seem a bit unwieldy.  But I wonder if that's
too terrible a problem, considering that this code is incurring a bunch
of syscalls in the best case anyway.

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.

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

Hmm.  Thanks.

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

I think that would be slightly clearer.  But if we can force this code
into actually making sense, that would be much better.

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