Re: Attempt to consolidate reading of XLOG page - Mailing list pgsql-hackers

Michael Paquier <michael@paquier.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