Re: Attempt to consolidate reading of XLOG page - Mailing list pgsql-hackers

From Antonin Houska
Subject Re: Attempt to consolidate reading of XLOG page
Date
Msg-id 5990.1569606865@antos
Whole thread Raw
In response to Re: Attempt to consolidate reading of XLOG page  (Alvaro Herrera <alvherre@2ndquadrant.com>)
Responses Re: Attempt to consolidate reading of XLOG page
List pgsql-hackers
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



pgsql-hackers by date:

Previous
From: Bruce Momjian
Date:
Subject: Re: pg_upgrade issues
Next
From: David Steele
Date:
Subject: Re: recovery starting when backup_label exists, but notrecovery.signal