Re: Attempt to consolidate reading of XLOG page

Michael Paquier <michael@paquier.xyz> wrote:

> 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:
> >> 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, the next version uses explicit lseek(). Maybe the fact that XLOG is mostly
read sequentially (i.e. without frequent seeks) is the reason pread() has't
been adopted so far.

The new version reflects your other suggestions too, except the one about not
renaming "XLOG" -> "WAL" (actually you mentioned that earlier in the
thread). I recall that when working on the preliminary patch (709d003fbd),
Alvaro suggested "WAL" for some structures because these are new. The rule
seemed to be that "XLOG..." should be left for the existing symbols, while the
new ones should be "WAL...":

https://www.postgresql.org/message-id/20190917221521.GA15733%40alvherre.pgsql

So I decided to rename the new symbols and to remove the related comment.

-- 
Antonin Houska
Web: https://www.cybertec-postgresql.com