From: Antonin Houska <ah@cybertec.at>
To: Kyotaro HORIGUCHI <horiguchi.kyotaro@lab.ntt.co.jp>
cc: pgsql-hackers@postgresql.org
Subject: Re: Remove page-read callback from XLogReaderState.
In-reply-to: <20190418.210257.43726183.horiguchi.kyotaro@lab.ntt.co.jp>
References: <20190418.210257.43726183.horiguchi.kyotaro@lab.ntt.co.jp>
Comments: In-reply-to Kyotaro HORIGUCHI <horiguchi.kyotaro@lab.ntt.co.jp>
   message dated "Thu, 18 Apr 2019 21:02:57 +0900."
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-ID: <18580.1556193500.1@localhost>
Content-Transfer-Encoding: quoted-printable
Date: Thu, 25 Apr 2019 13:58:20 +0200
Message-ID: <18581.1556193500@localhost>
Precedence: bulk

Kyotaro HORIGUCHI <horiguchi.kyotaro@lab.ntt.co.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.horiguch=
i.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:

* xlogreader.c

  ** Comments mention "opcode", "op" and "expression step" - probably left=
over
     from the executor, which seems to have inspired you.

  ** XLR_DISPATCH() seems to be unused

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

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

  ** XLogNeedData()

     *** shouldn't only the minimum amount of data needed (SizeOfXLogLongP=
HD)
     be requested here?

	state->loadLen =3D XLOG_BLCKSZ;
	XLR_LEAVE(XLND_STATE_SEGHEAD, true);

Note that ->loadLen is also set only to the minimum amount of data needed
elsewhere.

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

     *** 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 re=
ad
     from the page even if it's already in the buffer.

* xlogreader.h

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

  ** XLogReaderState.loadLen - why not reqLen? loadLen sounds to me like "=
loaded"
  as opposed to "requested". And assignemnt like this

	int reqLen	=3D xlogreader->loadLen;

  will also be less confusing with ->reqLen.

  Maybe also ->loadPagePtr should be renamed to ->targetPagePtr.


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


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

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

-- =

Antonin Houska
Web: https://www.cybertec-postgresql.com