Re: Attempt to consolidate reading of XLOG page

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
Date: 2019-11-12 11:06:33
Message-ID: 44237.1573556793@antos
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

Michael Paquier <michael(at)paquier(dot)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...":

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

Antonin Houska

Attachment Content-Type Size
v10-0005-Use-only-xlogreader.c-XLogRead.patch text/x-diff 20.3 KB
v10-0006-Remove-the-old-implemenations-of-XLogRead.patch text/x-diff 13.8 KB

In response to


Browse pgsql-hackers by date

  From Date Subject
Next Message Amit Kapila 2019-11-12 11:11:07 Re: [HACKERS] Block level parallel vacuum
Previous Message 2019-11-12 10:49:49 RE: [Patch] Optimize dropping of relation buffers using dlist