|From:||Antonin Houska <ah(at)cybertec(dot)at>|
|To:||Michael Paquier <michael(at)paquier(dot)xyz>|
|Cc:||Alvaro Herrera <alvherre(at)2ndquadrant(dot)com>, Thomas Munro <thomas(dot)munro(at)gmail(dot)com>, Robert Haas <robertmhaas(at)gmail(dot)com>, "pgsql-hackers(at)postgresql(dot)org" <pgsql-hackers(at)postgresql(dot)org>|
|Subject:||Re: Attempt to consolidate reading of XLOG page|
|Views:||Raw Message | Whole Thread | Download mbox | Resend email|
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
> 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().
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
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
> +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
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.
|Next Message||Gilles Darold||2019-11-11 15:43:18||Re: [PATCH][DOC] Fix for PREPARE TRANSACTION doc and postgres_fdw message.|
|Previous Message||Konstantin Knizhnik||2019-11-11 15:19:21||Re: [Proposal] Global temporary tables|