Re: Remove page-read callback from XLogReaderState.

From: Kyotaro HORIGUCHI <horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp>
To: ah(at)cybertec(dot)at
Cc: pgsql-hackers(at)postgresql(dot)org
Subject: Re: Remove page-read callback from XLogReaderState.
Date: 2019-04-26 08:40:34
Message-ID: 20190426.174034.203489629.horiguchi.kyotaro@lab.ntt.co.jp
Views: Raw Message | Whole Thread | Download mbox
Thread:
Lists: pgsql-hackers

Hello. Thank you for looking this.

At Thu, 25 Apr 2019 13:58:20 +0200, Antonin Houska <ah(at)cybertec(dot)at> wrote in <18581(dot)1556193500(at)localhost>
> Kyotaro HORIGUCHI <horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp> wrote:
>
> > Hello. As mentioned before [1], read_page callback in
> > XLogReaderState is a cause of headaches. Adding another
> > remote-controlling stuff to xlog readers makes things messier [2].
>
> The patch I posted in thread [2] tries to solve another problem: it tries to
> merge xlogutils.c:XLogRead(), walsender.c:XLogRead() and
> pg_waldump.c:XLogDumpXLogRead() into a single function,
> xlogutils.c:XLogRead().
>
> > [2]
> > https://www.postgresql.org/message-id/20190412.122711.158276916.horiguchi.kyotaro@lab.ntt.co.jp
>
> > I refactored XLOG reading functions so that we don't need the
> > callback.
>
> I was curious about the patch, so I reviewed it:

Thnak for the comment. (It's a shame that I might made it more complex..)

> * xlogreader.c
>
> ** Comments mention "opcode", "op" and "expression step" - probably leftover
> from the executor, which seems to have inspired you.

Uggh. Yes, exactly. I believed change them all. Fixed.

> ** XLR_DISPATCH() seems to be unused

Right. XLR_ macros are used to dispatch internally in a function
differently from EEO_ macros so I thought it uesless but I
hesitated to remove it. I remove it.

> ** Comment: "duplicatedly" -> "repeatedly" ?

It aimed reentrance. But I notieced that it doesn't work when
ERROR-exiting. So I remove the assertion and related code..

> ** XLogReadRecord(): comment "All internal state need ..." -> "needs"

Fixed.

> ** XLogNeedData()
>
> *** shouldn't only the minimum amount of data needed (SizeOfXLogLongPHD)
> be requested here?
>
> state->loadLen = XLOG_BLCKSZ;
> XLR_LEAVE(XLND_STATE_SEGHEAD, true);
>
> Note that ->loadLen is also set only to the minimum amount of data needed
> elsewhere.

Maybe right, but it is existing behavior so I preserved it as
focusing on refactoring.

> *** you still mention "read_page callback" in a comment.

Thanks. "the read_page callback" were translated to "the caller"
and it seems the last one.

> *** state->readLen is checked before one of the calls of XLR_LEAVE(), but
> I think it should happen before *each* call. Otherwise data can be read
> from the page even if it's already in the buffer.

That doesn't happen since XLogReadRecord doesn't LEAVE unless
XLogNeedData returns true (that is, needs more data) and
XLogNeedData returns true only when requested data is not on the
buffer yet. (If I refactored it correctly and it seems to me so.)

> * xlogreader.h
>
> ** XLND_STATE_PAGEFULLHEAD - maybe LONG rather than FULL? And perhaps HEAD
> -> HDR, so it's clear that it's about (page) header, not e.g. list head.

Perhaps that's better. Thanks.

>
> ** XLogReaderState.loadLen - why not reqLen? loadLen sounds to me like "loaded"
> as opposed to "requested". And assignemnt like this
>
> int reqLen = xlogreader->loadLen;
>
> will also be less confusing with ->reqLen.
>
> Maybe also ->loadPagePtr should be renamed to ->targetPagePtr.

Yeah, that's annoyance. reqLen *was* actually the "requested"
length to XLogNeedData FKA ReadPageInternal, but in the current
shape XLogNeedData makes different request to the callers (when
fetching the first page in the newly visited segment), so the two
(req(uest)Len and (to be)loadLen) are different things. At the
same time, targetPagePoint is different from loadPagePtr.

Of course the naming as arguable.

> * trailing whitespace: xlogreader.h:130, xlogreader.c:1058

Thanks, it have been fixed on my repo.

> * The 2nd argument of SimpleXLogPageRead() is "private", which seems too
> generic given that the function is no longer used as a callback. Since the
> XLogPageReadPrivate structure only has two fields, I think it'd be o.k. to
> pass them to the function directly.

Sound reasonable. Fixed.

> * I haven't found CF entry for this patch.

Yeah, I'll register this, maybe the week after next week.

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center

Attachment Content-Type Size
v2-0001-Define-macros-to-make-XLogReadRecord-a-state-machine.patch text/x-patch 3.9 KB
v2-0002-Make-ReadPageInternal-a-state-machine.patch text/x-patch 24.2 KB
v2-0003-Change-interface-of-XLogReadRecord.patch text/x-patch 12.0 KB
v2-0004-Make-XLogReadRecord-a-state-machine.patch text/x-patch 17.1 KB
v2-0005-Make-XLogPageRead-not-use-callback-but-call-the-func.patch text/x-patch 4.7 KB
v2-0006-Make-XlogReadTwoPhaseData-not-use-callback-but-call-.patch text/x-patch 4.0 KB
v2-0007-Make-logical-rep-stuff-not-use-callback-but-call-the.patch text/x-patch 10.1 KB
v2-0008-Make-pg_waldump-not-use-callback-but-call-the-functi.patch text/x-patch 5.9 KB
v2-0009-Make-pg_rewind-not-use-callback-but-call-the-functio.patch text/x-patch 6.2 KB
v2-0010-Remove-callback-entry-from-XLogReaderState.patch text/x-patch 7.5 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Etsuro Fujita 2019-04-26 09:25:38 Re: pgsql: Allow insert and update tuple routing and COPY for foreign table
Previous Message Kyotaro HORIGUCHI 2019-04-26 08:29:56 Re: Regression test PANICs with master-standby setup on same machine