Re: Remove page-read callback from XLogReaderState.

Hi,

On 2021-04-07 05:09:53 +1200, Thomas Munro wrote:
> From 560cdfa444a3b05a0e6b8054f3cfeadf56e059fc Mon Sep 17 00:00:00 2001
> From: Kyotaro Horiguchi <horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp>
> Date: Thu, 5 Sep 2019 20:21:55 +0900
> Subject: [PATCH v18 1/3] Move callback-call from ReadPageInternal to
> XLogReadRecord.
>
> The current WAL record reader reads page data using a call back
> function. Redesign the interface so that it asks the caller for more
> data when required. This model works better for proposed projects that
> encryption, prefetching and other new features that would require
> extending the callback interface for each case.
>
> As the first step of that change, this patch moves the page reader
> function out of ReadPageInternal(), then the remaining tasks of the
> function are taken over by the new function XLogNeedData().

> -static int
> +static bool
> XLogPageRead(XLogReaderState *xlogreader, XLogRecPtr targetPagePtr, int reqLen,
> XLogRecPtr targetRecPtr, char *readBuf)
> {
> @@ -12170,7 +12169,8 @@ retry:
> readLen = 0;
> readSource = XLOG_FROM_ANY;
>
> - return -1;
> + xlogreader->readLen = -1;
> + return false;
> }
> }

It seems a bit weird to assign to XlogReaderState->readLen inside the
callbacks. I first thought it was just a transient state, but it's
not. I think it'd be good to wrap the xlogreader->readLen assignment an
an inline function. That we can add more asserts etc over time.

> -/* pg_waldump's XLogReaderRoutine->page_read callback */
> +/*
> + * pg_waldump's WAL page rader, also used as page_read callback for
> + * XLogFindNextRecord
> + */
> static bool
> -WALDumpReadPage(XLogReaderState *state, XLogRecPtr targetPagePtr, int reqLen,
> - XLogRecPtr targetPtr, char *readBuff)
> +WALDumpReadPage(XLogReaderState *state, void *priv)
> {
> - XLogDumpPrivate *private = state->private_data;
> + XLogRecPtr targetPagePtr = state->readPagePtr;
> + int reqLen = state->readLen;
> + char *readBuff = state->readBuf;
> + XLogDumpPrivate *private = (XLogDumpPrivate *) priv;

It seems weird to pass a void *priv to a function that now doesn't at
all need the type punning anymore.

Greetings,

Andres Freund