Re: Attempt to consolidate reading of XLOG page

Alvaro Herrera <alvherre(at)2ndquadrant(dot)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