Re: Attempt to consolidate reading of XLOG page

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?

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.

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.

+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().  There is also no point in using a pointer to
the TLI, no?

+ * 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.
--
Michael