agora inbox for pgsql-hackers@postgresql.org
help / color / mirror / Atom feedFrom: Antonin Houska <ah@cybertec.at>
To: Alvaro Herrera <alvherre@2ndquadrant.com>
Cc: Thomas Munro <thomas.munro@gmail.com>
Cc: Robert Haas <robertmhaas@gmail.com>
Cc: pgsql-hackers@postgresql.org <pgsql-hackers@postgresql.org>
Subject: Re: Attempt to consolidate reading of XLOG page
Date: Fri, 27 Sep 2019 19:54:25 +0200
Message-ID: <5990.1569606865@antos> (raw)
In-Reply-To: <20190927132210.GA18194@alvherre.pgsql>
References: <20190927132210.GA18194@alvherre.pgsql>
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
view thread (62+ messages) latest in thread
Message-ID: <5990.1569606865@antos>
Permalink: ../5990.1569606865@antos/
Also on: postgresql.org/message-id/5990.1569606865@antos
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: ah@cybertec.at, alvherre@2ndquadrant.com, thomas.munro@gmail.com, robertmhaas@gmail.com
Subject: Re: Attempt to consolidate reading of XLOG page
In-Reply-To: <5990.1569606865@antos>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox