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
signature.asc
Description: PGP signature
