Re: [HACKERS] AdvanceXLInsertBuffer vs. WAL segment compressibility

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