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.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:
* xlogreader.c
** Comments mention "opcode", "op" and "expression step" - probably leftover
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 (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.
*** 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 read
from the page even if it's already in the buffer.
* 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.
** 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.
* 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 the
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
