Re: Attempt to consolidate reading of XLOG page

Michael Paquier <michael(at)paquier(dot)xyz> wrote:

> On Fri, Oct 04, 2019 at 12:11:11PM +0200, Antonin Houska wrote:
> > This is the next version.
>
> So... These are the two last bits to look at, reducing a bit the code
> size:
> 5 files changed, 396 insertions(+), 419 deletions(-)
>
> And what this patch set does is to refactor the routines we use now in
> xlogreader.c to read a page by having new callbacks to open a segment,
> as that's basically the only difference between the context of a WAL
> sender, pg_waldump and recovery.
>
> Here are some comments reading through the code.
>
> + * Note that XLogRead(), if used, should have updated the "seg" too for
> + * its own reasons, however we cannot rely on ->read_page() to call
> + * XLogRead().
> Why?

I've updated the comment:

+ /*
+ * Update read state information.
+ *
+ * If XLogRead() is was called by ->read_page, it should have updated the
+ * ->seg fields accordingly (since we never request more than a single
+ * page, neither ws_segno nor ws_off should have advanced beyond
+ * targetSegNo and targetPageOff respectively). However it's not mandatory
+ * for ->read_page to call XLogRead().
+ */

Besides what I say here, I'm not sure if we should impose additional
requirement on the existing callbacks (possibly those in extensions) to update
the XLogReaderState.seg structure.

> Your patch removes all the three optional lseek() calls which can
> happen in a segment. Am I missing something but isn't that plain
> wrong? You could reuse the error context for that as well if an error
> happens as what's needed is basically the segment name and the LSN
> offset.

Explicit call of lseek() is not used because XLogRead() uses pg_pread()
now. Nevertheless I found out that in the the last version of the patch I set
ws_off to 0 for a newly opened segment. This was wrong, fixed now.

> All the callers of XLogReadProcessError() are in src/backend/, so it
> seems to me that there is no point to keep that in xlogreader.c but it
> should be instead in xlogutils.c, no? It seems to me that this is
> more like XLogGenerateError, or just XLogError(). We have been using
> xlog as an acronym in many places of the code, so switching now to wal
> just for the past matter of the pg_xlog -> pg_wal switch does not seem
> worth bothering.

ok, moved to xlogutils.c and renamed to XLogReadRaiseError(). I think the
"Read" word should be there because many other error can happen during XLOG
processing.

> +read_local_xlog_page_segment_open(XLogSegNo nextSegNo,
> + WALSegmentContext *segcxt,
> Could you think about a more simple name here? It is a callback to
> open a new segment, so it seems to me that we could call it just
> open_segment_callback().

ok, the function is not exported to other modules, so there's no need to care
about uniqueness of the name. I chose wal_segment_open(), according to the
callback type WALSegmentOpen.

> There is also no point in using a pointer to the TLI, no?

This particular callback makes no decision about the TLI, so it only uses
tli_p as an input argument.

> + * Read 'count' bytes from WAL fetched from timeline 'tli' into 'buf',
> + * starting at location 'startptr'. 'seg' is the last segment used,
> + * 'openSegment' is a callback to opens the next segment if needed and
> + * 'segcxt' is additional segment info that does not fit into 'seg'.
> A typo and the last part of the last sentence could be better worded.

ok, adjusted a bit.

Thanks for review.

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