Re: [HACKERS] AdvanceXLInsertBuffer vs. WAL segment compressibility

From: Stephen Frost <sfrost(at)snowman(dot)net>
To: Chapman Flack <chap(at)anastigmatix(dot)net>
Cc: Daniel Gustafsson <daniel(at)yesql(dot)se>, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>, Michael Paquier <michael(dot)paquier(at)gmail(dot)com>, Heikki Linnakangas <hlinnaka(at)iki(dot)fi>, Robert Haas <robertmhaas(at)gmail(dot)com>, Bruce Momjian <bruce(at)momjian(dot)us>, PostgreSQL Hackers <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: [HACKERS] AdvanceXLInsertBuffer vs. WAL segment compressibility
Date: 2018-03-26 03:27:31
Message-ID: 20180326032731.GM24540@tamriel.snowman.net
Views: Raw Message | Whole Thread | Download mbox
Thread:
Lists: pgsql-hackers

Greetings,

* Chapman Flack (chap(at)anastigmatix(dot)net) wrote:
> On 03/18/18 19:28, Daniel Gustafsson wrote:
> > It seems expensive to regex over BLCKSZ, but it’s probably the safest option
> > and it’s not a performance critical codepath. Feel free to whack the test
> > patch over the head with the above diff.
>
> Both patches in a single email for cfbot's enjoyment, coming right up.

Thanks for this. I'm also of the opinion, having read through the
thread, that it was an unintentional side-effect to have the headers in
the otherwise-zero'd end of the WAL segment and that re-zero'ing the end
makes sense and while it's a bit of extra time, it's not on a
performance-critical path given this is only happening on a less-busy
system to begin with.

I'm mildly disappointed to see the gzip has only a 2x improvement with
the change, but that's certainly not this patch's fault.

Reviewing the patch itself..

> .travis.yml | 47 ++++++++++++++++++++++++++++++++++++

... not something that I think we're going to add into the main tree.

> diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
> index 47a6c4d..a91ec7b 100644
> --- a/src/backend/access/transam/xlog.c
> +++ b/src/backend/access/transam/xlog.c
> @@ -1556,7 +1556,16 @@ CopyXLogRecordToWAL(int write_len, bool isLogSwitch, XLogRecData *rdata,
> {
> /* initialize the next page (if not initialized already) */
> WALInsertLockUpdateInsertingAt(CurrPos);
> - AdvanceXLInsertBuffer(CurrPos, false);
> + /*
> + * Fields in the page header preinitialized by AdvanceXLInsertBuffer
> + * to nonconstant values reduce the compressibility of WAL segments,
> + * and aren't needed in the freespace following a switch record.
> + * Re-zero that header area. This is not performance-critical, as
> + * the more empty pages there are for this loop to touch, the less
> + * busy the system is.
> + */
> + currpos = GetXLogBuffer(CurrPos);
> + MemSet(currpos, 0, SizeOfXLogShortPHD);
> CurrPos += XLOG_BLCKSZ;
> }
> }

AdvanceXLInsertBuffer() does quite a bit, so I'm a bit surprised to see
this simply removing that call, you're confident there's nothing done
which still needs doing..?

Thanks!

Stephen

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Jacob Champion 2018-03-26 04:47:45 Re: Proposal: http2 wire format
Previous Message Craig Ringer 2018-03-26 03:16:33 Re: Backend memory dump analysis