Horton

Thread

Commits

  1. Avoid some unnecessary block reads in WAL reader

  2. Split out XLog reading as an independent facility

  3. Make standby server continuously retry restoring the next WAL segment with

  1. [PATCH] xlogreader: do not read a file block twice

    Artur Zakirov <a.zakirov@postgrespro.ru> — 2019-02-11T16:25:39Z

    Hello hackers,

Grigory noticed that one of our utilities has very slow performance when 
xlogreader reads zlib archives. We found out that xlogreader sometimes 
reads a WAL file block twice.

zlib has slow performance when you read an archive not in sequential 
order. I think reading a block twice in same position isn't sequential, 
because gzread() moves current position forward and next call gzseek() 
to the same position moves it back.

It seems that the attached patch solves the issue. I think when reqLen 
== state->readLen the requested block already is in the xlogreader's buffer.

What do you think?

-- 
Arthur Zakirov
Postgres Professional: http://www.postgrespro.com
Russian Postgres Company

  2. Re: [PATCH] xlogreader: do not read a file block twice

    Grigory Smolkin <g.smolkin@postgrespro.ru> — 2019-02-11T16:32:59Z

    Hm, looks like it could speed up PostgreSQL recovery, but is it safe?


On 02/11/2019 07:25 PM, Arthur Zakirov wrote:
> Hello hackers,
>
> Grigory noticed that one of our utilities has very slow performance 
> when xlogreader reads zlib archives. We found out that xlogreader 
> sometimes reads a WAL file block twice.
>
> zlib has slow performance when you read an archive not in sequential 
> order. I think reading a block twice in same position isn't 
> sequential, because gzread() moves current position forward and next 
> call gzseek() to the same position moves it back.
>
> It seems that the attached patch solves the issue. I think when reqLen 
> == state->readLen the requested block already is in the xlogreader's 
> buffer.
>
> What do you think?
>

-- 
Grigory Smolkin
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

  3. Re: [PATCH] xlogreader: do not read a file block twice

    Michael Paquier <michael@paquier.xyz> — 2019-02-12T04:23:00Z

    On Mon, Feb 11, 2019 at 07:32:59PM +0300, Grigory Smolkin wrote:
> Hm, looks like it could speed up PostgreSQL recovery, but is it
> safe?

(Please avoid top-posting.)

> On 02/11/2019 07:25 PM, Arthur Zakirov wrote:
>> Grigory noticed that one of our utilities has very slow performance when
>> xlogreader reads zlib archives. We found out that xlogreader sometimes
>> reads a WAL file block twice.
>> 
>> What do you think?

I think that such things, even if they look simple, need a careful
lookup, and I have not looked at the proposal yet.  Could you add it
to the next commit fest so as we don't lose track of it?
https://commitfest.postgresql.org/22/
--
Michael

  4. Re: [PATCH] xlogreader: do not read a file block twice

    Artur Zakirov <a.zakirov@postgrespro.ru> — 2019-02-12T08:44:14Z

    On 12.02.2019 07:23, Michael Paquier wrote:
>> On 02/11/2019 07:25 PM, Arthur Zakirov wrote:
>>> Grigory noticed that one of our utilities has very slow performance when
>>> xlogreader reads zlib archives. We found out that xlogreader sometimes
>>> reads a WAL file block twice.
>>>
>>> What do you think?
> 
> I think that such things, even if they look simple, need a careful
> lookup, and I have not looked at the proposal yet.  Could you add it
> to the next commit fest so as we don't lose track of it?
> https://commitfest.postgresql.org/22/

Of course. Agree, it may be a non trivial case. Added as a bug fix:
https://commitfest.postgresql.org/22/1994/

-- 
Arthur Zakirov
Postgres Professional: http://www.postgrespro.com
Russian Postgres Company

  5. Re: [PATCH] xlogreader: do not read a file block twice

    Andrei Lepikhov <a.lepikhov@postgrespro.ru> — 2019-02-12T17:47:32Z

    
On 11.02.2019 21:25, Arthur Zakirov wrote:
> Hello hackers,
> 
> Grigory noticed that one of our utilities has very slow performance when 
> xlogreader reads zlib archives. We found out that xlogreader sometimes 
> reads a WAL file block twice.
> 
> zlib has slow performance when you read an archive not in sequential 
> order. I think reading a block twice in same position isn't sequential, 
> because gzread() moves current position forward and next call gzseek() 
> to the same position moves it back.
> 
> It seems that the attached patch solves the issue. I think when reqLen 
> == state->readLen the requested block already is in the xlogreader's 
> buffer.
> 
> What do you think?
I looked at the history of the code changes:

---------------------------------------------------------------
7fcbf6a405f (Alvaro Herrera 2013-01-16 16:12:53 -0300 539) 
reqLen < state->readLen)

1bb2558046c (Heikki Linnakangas 2010-01-27 15:27:51 +0000 9349) 
        targetPageOff == readOff && targetRecOff < readLen)

eaef111396e (Tom Lane 2006-04-03 23:35:05 +0000 3842)
len = XLOG_BLCKSZ - RecPtr->xrecoff % XLOG_BLCKSZ;
4d14fe0048c (Tom Lane 2001-03-13 01:17:06 +0000 3843)
if (total_len > len) 
---------------------------------------------------------------

In the original code of Tom Lane, condition (total_len > len) caused a 
page reread from disk. As I understand it, this is equivalent to your 
proposal.
Th code line in commit 1bb2558046c seems tantamount to the corresponding 
line in commit 7fcbf6a405f but have another semantics: the targetPageOff 
value can't be more or equal XLOG_BLCKSZ, but the reqLen value can be. 
It may be a reason of appearance of possible mistake, introduced by 
commit 7fcbf6a405f.

-- 
Andrey Lepikhov
Postgres Professional
https://postgrespro.com
The Russian Postgres Company

  6. Re: [PATCH] xlogreader: do not read a file block twice

    Artur Zakirov <a.zakirov@postgrespro.ru> — 2019-02-13T11:17:41Z

    On 12.02.2019 20:47, Andrey Lepikhov wrote:
> I looked at the history of the code changes:
> 
> ---------------------------------------------------------------
> 7fcbf6a405f (Alvaro Herrera 2013-01-16 16:12:53 -0300 539) reqLen < 
> state->readLen)
> 
> 1bb2558046c (Heikki Linnakangas 2010-01-27 15:27:51 +0000 9349)        
> targetPageOff == readOff && targetRecOff < readLen)
> 
> eaef111396e (Tom Lane 2006-04-03 23:35:05 +0000 3842)
> len = XLOG_BLCKSZ - RecPtr->xrecoff % XLOG_BLCKSZ;
> 4d14fe0048c (Tom Lane 2001-03-13 01:17:06 +0000 3843)
> if (total_len > len) 
> ---------------------------------------------------------------
> 
> In the original code of Tom Lane, condition (total_len > len) caused a 
> page reread from disk. As I understand it, this is equivalent to your 
> proposal.
> Th code line in commit 1bb2558046c seems tantamount to the corresponding 
> line in commit 7fcbf6a405f but have another semantics: the targetPageOff 
> value can't be more or equal XLOG_BLCKSZ, but the reqLen value can be. 
> It may be a reason of appearance of possible mistake, introduced by 
> commit 7fcbf6a405f.

Thank you for your research. Indeed, it makes sense now.

In my case after reading a page both `reqLen` and `state->readLen` equal 
to XLOG_BLCKSZ. This leads to a page reread, since `pageptr` is the same 
as the previous read. But `targetRecOff` is different in the second case 
because we want to read next record, which probably doesn't fit into the 
page wholly (that's why `reqLen` is equal to XLOG_BLCKSZ).

-- 
Arthur Zakirov
Postgres Professional: http://www.postgrespro.com
Russian Postgres Company

  7. Re: [PATCH] xlogreader: do not read a file block twice

    Michael Paquier <michael@paquier.xyz> — 2019-02-14T06:51:56Z

    On Tue, Feb 12, 2019 at 11:44:14AM +0300, Arthur Zakirov wrote:
> Of course. Agree, it may be a non trivial case. Added as a bug fix:
> https://commitfest.postgresql.org/22/1994/

I have been looking at the patch, and I agree that the current coding
is a bit crazy.  If the wanted data has already been read, it makes
little sense to require reading it again if the size requested by the
caller of ReadPageInternal() exactly equals what has been read
already, and that's what the code is doing.

Now I don't actually agree that this qualifies as a bug fix.  As
things stand, a page may finish by being more than once if what has
been read previously equals what is requested, however this does not
prevent the code to work correctly.  The performance gain is also
heavily dependent on the callback reading a page and the way the WAL
reader is used.  How do you actually read WAL pages in your own
plugin with compressed data?  It begins by reading a full page once,
then it moves on to a per-record read after making sure that the page
has been read?
--
Michael

  8. Re: [PATCH] xlogreader: do not read a file block twice

    Artur Zakirov <a.zakirov@postgrespro.ru> — 2019-02-14T08:20:56Z

    On 14.02.2019 09:51, Michael Paquier wrote:
> Now I don't actually agree that this qualifies as a bug fix.  As
> things stand, a page may finish by being more than once if what has
> been read previously equals what is requested, however this does not
> prevent the code to work correctly.  The performance gain is also
> heavily dependent on the callback reading a page and the way the WAL
> reader is used.  How do you actually read WAL pages in your own
> plugin with compressed data?  It begins by reading a full page once,
> then it moves on to a per-record read after making sure that the page
> has been read?

Yes, an application reads WAL pages wholly at a time. It is done within 
SimpleXLogPageRead() (it is a read_page callback passed to 
XLogReaderAllocate()). It returns XLOG_BLCKSZ.

Here is the part of the code, not sure that it will be useful though:

SimpleXLogPageRead(...)
{
     ...
     targetPageOff = targetPagePtr % private_data->xlog_seg_size;
     ...
     if (gzseek(private_data->gz_xlogfile, (z_off_t) targetPageOff,
                SEEK_SET) == -1)
         ...
     if (gzread(private_data->gz_xlogfile, readBuf, XLOG_BLCKSZ) !=
                XLOG_BLCKSZ)
     ...
     return XLOG_BLCKSZ;
}

So we read whole page with size XLOG_BLCKSZ. The full code:
https://github.com/postgrespro/pg_probackup/blob/c052651b8c8864733bcabbc2660c387b792229d8/src/parsexlog.c#L1074

Here is the little optimization I made. Mainly I just add a buffer to 
store previous read page:
https://github.com/postgrespro/pg_probackup/blob/c052651b8c8864733bcabbc2660c387b792229d8/src/parsexlog.c#L1046

-- 
Arthur Zakirov
Postgres Professional: http://www.postgrespro.com
Russian Postgres Company

  9. Re: [PATCH] xlogreader: do not read a file block twice

    Michael Paquier <michael@paquier.xyz> — 2019-02-14T23:06:16Z

    On Thu, Feb 14, 2019 at 11:20:56AM +0300, Arthur Zakirov wrote:
> So we read whole page with size XLOG_BLCKSZ. The full code:
> https://github.com/postgrespro/pg_probackup/blob/c052651b8c8864733bcabbc2660c387b792229d8/src/parsexlog.c#L1074
> 
> Here is the little optimization I made. Mainly I just add a buffer to store
> previous read page:
> https://github.com/postgrespro/pg_probackup/blob/c052651b8c8864733bcabbc2660c387b792229d8/src/parsexlog.c#L1046

Thanks, I see what you have done.  I cannot comment if your shortcut
is actually fully correct based on my knowledge of this code, but
things cannot be in the best conditions without having the WAL reader
handle properly the limits.  So I am planning to commit what you
propose after an extra pass on it in the next couple of days or so.
--
Michael

  10. Re: [PATCH] xlogreader: do not read a file block twice

    Michael Paquier <michael@paquier.xyz> — 2019-02-18T01:09:02Z

    On Fri, Feb 15, 2019 at 08:06:16AM +0900, Michael Paquier wrote:
> Thanks, I see what you have done.  I cannot comment if your shortcut
> is actually fully correct based on my knowledge of this code, but
> things cannot be in the best conditions without having the WAL reader
> handle properly the limits.  So I am planning to commit what you
> propose after an extra pass on it in the next couple of days or so.

And done, after doing and extra pass, doing more testing using by own
plugins, pg_waldump and more fancy stuff with a primary/standby and
pgbench.
--
Michael

  11. Re: [PATCH] xlogreader: do not read a file block twice

    Artur Zakirov <a.zakirov@postgrespro.ru> — 2019-02-18T08:23:46Z

    On 18.02.2019 04:09, Michael Paquier wrote:
> And done, after doing and extra pass, doing more testing using by own
> plugins, pg_waldump and more fancy stuff with a primary/standby and
> pgbench.

Thank you Michael.

-- 
Arthur Zakirov
Postgres Professional: http://www.postgrespro.com
Russian Postgres Company