Re: Attempt to consolidate reading of XLOG page

Michael Paquier <michael@paquier.xyz>

From: Michael Paquier <michael@paquier.xyz>
To: Alvaro Herrera <alvherre@2ndquadrant.com>
Cc: Antonin Houska <ah@cybertec.at>, Thomas Munro <thomas.munro@gmail.com>, Robert Haas <robertmhaas@gmail.com>, "pgsql-hackers@postgresql.org" <pgsql-hackers@postgresql.org>
Date: 2019-11-25T03:30:48Z
Lists: pgsql-hackers
On Fri, Nov 22, 2019 at 07:56:32PM -0300, Alvaro Herrera wrote:
> I see no reason to leave ws_off.  We can move that to XLogReaderState; I
> did that here.  We also need the offset in WALReadError, though, so I
> added it there too.  Conceptually it seems clearer to me this way.

Yeah, that seems cleaner.

> What do you think of the attached?

Looks rather fine to me.

> BTW I'm not clear what errors can pread()/pg_pread() report that do not
> set errno.  I think lines 1083/1084 of WALRead are spurious now.

Because we have no guarantee that errno will be cleared if you do a
partial read where errno is not set, so you may finish by reporting
the state of a previous failed read instead of the partially-failed
one depending on how WALReadError is treated?  In short, I don't see
any actual reason why it would be good to remove the reset of errno
either before the calls to pread and pwrite().
--
Michael

Commits

  1. Remove logical_read_local_xlog_page

  2. Refactor WAL file-reading code into WALRead()

  3. Rework WAL-reading supporting structs

  4. Make XLogReaderInvalReadState static

  5. Use pg_pread() and pg_pwrite() for data files and WAL.