Re: Attempt to consolidate reading of XLOG page

On Mon, Nov 11, 2019 at 04:25:56PM +0100, Antonin Houska wrote:
>> On Fri, Oct 04, 2019 at 12:11:11PM +0200, Antonin Houska wrote:
> + /*
> + * 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.

"is was called" does not make sense in this sentence. Actually, I
would tend to just remove it completely.

>> 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.

Missed that part, thanks. This was actually not obvious after an
initial lookup of the patch. Wouldn't it make sense to split that
part in a separate patch that we could review and get committed first
then? It would have the advantage to make the rest easier to review
and follow. And using pread is actually better for performance
compared to read+lseek. Now there is also the argument that we don't
always seek into an opened WAL segment, and that a plain read() is
actually better than pread() in some cases.

> 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.

No issue with this name.

> 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.

Name is fine by me.

>> 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.

Missed that walsender.c can enforce the tli to a new value, objection
withdrawn.

+ * BasicOpenFile() is the preferred way to open the segment file in backend
+ * code, whereas open(2) should be used in frontend.
I would remove that sentence.

+#ifndef FRONTEND
+/*
+ * Backend-specific convenience code to handle read errors encountered by
+ * XLogRead().
+ */
+void
+XLogReadRaiseError(XLogReadError *errinfo)
No need for the FRONTEND ifndef's here as xlogutils.c is backend-only.

+#ifndef FRONTEND
+void XLogReadRaiseError(XLogReadError *errinfo);
+#endif
Same as above, and missing an extern declaration.

+ fatal_error("could not read from log file %s, offset %u, length %zu: %s",
+ fname, seg->ws_off, (Size) errinfo.reqbytes,
+ strerror(errinfo.read_errno));
Let's use this occasion to make those error messages more generic to
reduce the pain of translators as the file name lets us know that we
have to deal with a WAL segment. Here are some suggestions, taking
into account the offset:
- If errno is set: "could not read file \"%s\" at offset %u: %m"
- For partial read: "could not read file \"%s\" at offset %u: read %d of %zu"
--
Michael