Re: Remove page-read callback from XLogReaderState.

From: Kyotaro Horiguchi <horikyota(dot)ntt(at)gmail(dot)com>
To: andres(at)anarazel(dot)de
Cc: thomas(dot)munro(at)gmail(dot)com, takashi(dot)menjo(at)gmail(dot)com, craig(at)2ndquadrant(dot)com, hlinnaka(at)iki(dot)fi, alvherre(at)2ndquadrant(dot)com, pgsql-hackers(at)postgresql(dot)org, takashi(dot)menjou(dot)vg(at)hco(dot)ntt(dot)co(dot)jp
Subject: Re: Remove page-read callback from XLogReaderState.
Date: 2021-04-07 08:50:25
Message-ID: 20210407.175025.2302402064687001100.horikyota.ntt@gmail.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

At Tue, 6 Apr 2021 16:09:55 -0700, Andres Freund <andres(at)anarazel(dot)de> wrote in
> Hi,
>
> > 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.

Sounds reasonable. The variable is split up into request/result
variables and setting the result variable is wrapped by a
function. (0005).

> > -/* 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.

Mmm. I omitted it since client code was somewhat out-of-scope. In the
attached 0004 WALDumpReadPage() is no longer used as the callback of
XLogFindNextRecord.

On the way fixing them, I found that XLogReaderState.readPageTLI has
been moved to XLogReaderState.seg.ws_tli so I removed it from 0001.

I haven't changed the name "XLog reader" to "XLog decoder". I'm doing
that but it affects somewhat wide range of code.

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center

Attachment Content-Type Size
v19-0001-Move-callback-call-from-ReadPageInternal-to-XLog.patch text/x-patch 27.9 KB
v19-0002-Move-page-reader-out-of-XLogReadRecord.patch text/x-patch 64.7 KB
v19-0003-Remove-globals-readOff-readLen-and-readSegNo.patch text/x-patch 7.9 KB
v19-0004-Make-XLogFindNextRecord-not-use-callback-functio.patch text/x-patch 15.1 KB
v19-0005-Split-readLen-and-reqLen-of-XLogReaderState.patch text/x-patch 12.2 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Amit Langote 2021-04-07 09:04:08 Re: Wired if-statement in gen_partprune_steps_internal
Previous Message tanghy.fnst@fujitsu.com 2021-04-07 08:34:50 RE: Truncate in synchronous logical replication failed