Re: [HACKERS] AdvanceXLInsertBuffer vs. WAL segment compressibility

[Chapman Flack]

On 03/30/18 19:57, Michael Paquier wrote:

> look for tools using XLP_BKP_REMOVABLE.  One is pglesslog that we
> already know about.  However I have to be honest, I have not been able
> to find its source code,

I'm glad it wasn't just me.

-Chap

Re: [HACKERS] AdvanceXLInsertBuffer vs. WAL segment compressibility

[Michael Paquier]

On Fri, Mar 30, 2018 at 07:11:02PM -0400, Tom Lane wrote:
> Chapman Flack  writes:
> > On 03/30/18 16:21, Tom Lane wrote:
> >> I did not like the proposed test case too much, particularly not its
> >> undocumented API change for check_pg_config,
> 
> > Other than that API change, was there something the test case could have
> > done differently to make you like it more?
> 
> Well, if that'd been properly documented I'd probably have pushed it
> without complaint.  But I did wonder whether it could've been folded
> into one of the existing tests of pg_switch_wal().  This doesn't seem
> like a property worth spending a lot of cycles on testing.

Sorry for coming in late.  I have been busy doing some net-archeology to
look for tools using XLP_BKP_REMOVABLE.  One is pglesslog that we
already know about.  However I have to be honest, I have not been able
to find its source code, nor have I seen another tool making use of
XLP_BKP_REMOVABLE.  Could we just remove the flag then? 
--
Michael


signature.asc
Description: PGP signature

Re: [HACKERS] AdvanceXLInsertBuffer vs. WAL segment compressibility

[Tom Lane]

Chapman Flack  writes:
> On 03/30/18 16:21, Tom Lane wrote:
>> I did not like the proposed test case too much, particularly not its
>> undocumented API change for check_pg_config,

> Other than that API change, was there something the test case could have
> done differently to make you like it more?

Well, if that'd been properly documented I'd probably have pushed it
without complaint.  But I did wonder whether it could've been folded
into one of the existing tests of pg_switch_wal().  This doesn't seem
like a property worth spending a lot of cycles on testing.

regards, tom lane

Re: [HACKERS] AdvanceXLInsertBuffer vs. WAL segment compressibility

[Chapman Flack]

On 03/30/18 16:21, Tom Lane wrote:

> Yup.  Pushed with some rewriting of the comments.

Thanks!

> I did not like the proposed test case too much, particularly not its
> undocumented API change for check_pg_config,

Other than that API change, was there something the test case could have
done differently to make you like it more?

-Chap

Re: [HACKERS] AdvanceXLInsertBuffer vs. WAL segment compressibility

[Tom Lane]

Stephen Frost  writes:
> * Tom Lane (t...@sss.pgh.pa.us) wrote:
>> A potentially stronger complaint is that WAL-reading tools might fail
>> outright on a page with an invalid header, but I'd say that's a robustness
>> issue that they'd need to address anyway.  There's never been any
>> guarantee that the trailing pages of a WAL segment are valid.

> Agreed, I don't buy off that tools which fall apart when reading a page
> with an invalid header should block this from moving forward- those
> tools need to be fixed to not rely on trailing/unused WAL pages to be
> valid.

Yup.  Pushed with some rewriting of the comments.

I did not like the proposed test case too much, particularly not its
undocumented API change for check_pg_config, so I did not push that.
We already have test coverage for pg_switch_wal() so it doesn't seem
very critical to have more.

regards, tom lane

Re: [HACKERS] AdvanceXLInsertBuffer vs. WAL segment compressibility

[Stephen Frost]

Greetings,

* Tom Lane (t...@sss.pgh.pa.us) wrote:
> Chapman Flack  writes:
> > On 03/27/18 22:10, Michael Paquier wrote:
> >> Here you go for one example:
> >> https://sourceforge.net/projects/pglesslog/
> 
> > In any case, from my study of the commit, it is hard for me to see an issue.
> > The code comment says: "mark the header to indicate that WAL records
> > beginning in this page have removable backup blocks."
> 
> Yeah, that commit just moved a flag from individual WAL records to page
> headers, arguing that it was okay to assume that the same flag value
> applies to all records on a page.  If there are no records in the page,
> it doesn't matter what you think the flag value is.
> 
> A potentially stronger complaint is that WAL-reading tools might fail
> outright on a page with an invalid header, but I'd say that's a robustness
> issue that they'd need to address anyway.  There's never been any
> guarantee that the trailing pages of a WAL segment are valid.

Agreed, I don't buy off that tools which fall apart when reading a page
with an invalid header should block this from moving forward- those
tools need to be fixed to not rely on trailing/unused WAL pages to be
valid.

Thanks!

Stephen


signature.asc
Description: PGP signature

Re: [HACKERS] AdvanceXLInsertBuffer vs. WAL segment compressibility

[Tom Lane]

Chapman Flack  writes:
> On 03/27/18 22:10, Michael Paquier wrote:
>> Here you go for one example:
>> https://sourceforge.net/projects/pglesslog/

> In any case, from my study of the commit, it is hard for me to see an issue.
> The code comment says: "mark the header to indicate that WAL records
> beginning in this page have removable backup blocks."

Yeah, that commit just moved a flag from individual WAL records to page
headers, arguing that it was okay to assume that the same flag value
applies to all records on a page.  If there are no records in the page,
it doesn't matter what you think the flag value is.

A potentially stronger complaint is that WAL-reading tools might fail
outright on a page with an invalid header, but I'd say that's a robustness
issue that they'd need to address anyway.  There's never been any
guarantee that the trailing pages of a WAL segment are valid.

regards, tom lane

Re: [HACKERS] AdvanceXLInsertBuffer vs. WAL segment compressibility

[Chapman Flack]

On 03/27/18 20:09, Tomas Vondra wrote:
> Not sure what's up with gitweb, but git finds it without any issue:
> 
> https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=2dd9322ba6eea76800b38bfea0599fbc459458f2

Thanks, that worked.

On 03/27/18 22:10, Michael Paquier wrote:
> Here you go for one example:
> https://sourceforge.net/projects/pglesslog/

So far, I have been able to study the commit pertaining to
XLP_BKP_REMOVABLE. For again some odd reason, I am striking out on
finding pglesslog code to study. Using the clone URL offered by sourceforge:

$ git clone https://git.code.sf.net/p/pglesslog/code pglesslog-code
Cloning into 'pglesslog-code'...
warning: You appear to have cloned an empty repository.
Checking connectivity... done.

and there's a Files tab, but it tells me This project has no files.

I can find 1.4.2 beta on pgFoundry, but that predates the BKP_REMOVABLE
commit.


In any case, from my study of the commit, it is hard for me to see an issue.
The code comment says: "mark the header to indicate that WAL records
beginning in this page have removable backup blocks."

In the only case where this patch will zero a header--in the unused space
following the switch record in a segment--there are no "WAL records
beginning in this page". There will not be another WAL record of any kind
until the next valid page (with valid xlp_magic xlp_tli xlp_pageaddr),
which will be at the start of the next segment, and that page will have
XLP_BKP_REMOVABLE if it ought to, and that will tell the reader what it
needs to know.

Am I overlooking something?

-Chap

Re: [HACKERS] AdvanceXLInsertBuffer vs. WAL segment compressibility

[Michael Paquier]

On Tue, Mar 27, 2018 at 06:32:08PM -0400, Chapman Flack wrote:
> Is 2dd9322 a commit? I'm having difficulty finding it:
> 
> https://git.postgresql.org/gitweb/?p=postgresql.git=search=HEAD=commit=2dd9322
> 
> Am I searching wrong?
> 
> I probably won't have more time to look at this tonight, but could
> I ask in advance for examples of tools that would need this bit when
> encountering unwritten pages at the end of a segment?

Here you go for one example:
https://sourceforge.net/projects/pglesslog/
At the end we may want to perhaps just drop the flag, but that's quite a
hard call as there could be people not around who use it..
--
Michael


signature.asc
Description: PGP signature

Re: [HACKERS] AdvanceXLInsertBuffer vs. WAL segment compressibility

[Tomas Vondra]

On 03/28/2018 12:32 AM, Chapman Flack wrote:
> On 03/26/18 01:43, Michael Paquier wrote:
> 
>> Have a look at BKP_REMOVABLE then.  This was moved to page headers in
>> 2dd9322, still it seems to me that the small benefits outlined on this
>> thread don't justify breaking tools relying on this flag set, especially
>> if there is no replacement for it.
> 
> Is 2dd9322 a commit? I'm having difficulty finding it:
> 
> https://git.postgresql.org/gitweb/?p=postgresql.git=search=HEAD=commit=2dd9322
> 
> Am I searching wrong?
> 

Not sure what's up with gitweb, but git finds it without any issue:

https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=2dd9322ba6eea76800b38bfea0599fbc459458f2

> I probably won't have more time to look at this tonight, but could
> I ask in advance for examples of tools that would need this bit when
> encountering unwritten pages at the end of a segment?
> 
> I don't mean that as snark; it's just nonobvious to me and I might need
> a little nudge where to look.
> 
> -Chap
> 

regards

-- 
Tomas Vondra  http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: [HACKERS] AdvanceXLInsertBuffer vs. WAL segment compressibility

[Chapman Flack]

On 03/26/18 01:43, Michael Paquier wrote:

> Have a look at BKP_REMOVABLE then.  This was moved to page headers in
> 2dd9322, still it seems to me that the small benefits outlined on this
> thread don't justify breaking tools relying on this flag set, especially
> if there is no replacement for it.

Is 2dd9322 a commit? I'm having difficulty finding it:

https://git.postgresql.org/gitweb/?p=postgresql.git=search=HEAD=commit=2dd9322

Am I searching wrong?

I probably won't have more time to look at this tonight, but could
I ask in advance for examples of tools that would need this bit when
encountering unwritten pages at the end of a segment?

I don't mean that as snark; it's just nonobvious to me and I might need
a little nudge where to look.

-Chap

Re: [HACKERS] AdvanceXLInsertBuffer vs. WAL segment compressibility

[Chapman Flack]

On 03/26/18 12:34, Tom Lane wrote:
> If that's the argument, why is the WALInsertLockUpdateInsertingAt(CurrPos)
> call still there?  GetXLogBuffer() would do that too.

"Because I hadn't noticed that," he said, sheepishly.

> In any case, the new comment ... fails to
> explain what's going on, and the reference to a function that is not
> actually called from the vicinity of the comment ...
> suggest something like "GetXLogBuffer() will fetch and initialize the
> next WAL page for us.  ... worth explaining how you know that the new
> page's header is short not long.

Here are patches responding to that (and also fixing the unintended
inclusion of .travis.yml).

What I have not done here is respond to Michael's objection, which
I haven't had time to think more about yet.

-Chap
>From 3606cccfd584654970f56e909798d0d163fa7e96 Mon Sep 17 00:00:00 2001
From: Chapman Flack 
Date: Mon, 26 Mar 2018 23:08:26 -0400
Subject: [PATCH 1/2] Zero headers of unused pages after WAL switch.

When writing zeroed pages to the remainder of a WAL segment
after a WAL switch, ensure that the headers of those pages are
also zeroed, as their initialized values otherwise reduce the
compressibility of the WAL segment file by general tools.
---
 src/backend/access/transam/xlog.c | 45 ---
 1 file changed, 42 insertions(+), 3 deletions(-)

diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index cb9c2a2..cf4eaa1 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -1552,11 +1552,50 @@ CopyXLogRecordToWAL(int write_len, bool isLogSwitch, XLogRecData *rdata,
 		/* Use up all the remaining space on the first page */
 		CurrPos += freespace;
 
+		/*
+		 * Cause all remaining pages in the segment to be flushed, leaving the
+		 * XLog position where it should be at the start of the next segment. In
+		 * the process, the pages will be initialized and physically written to
+		 * the file. That costs a bit of I/O (compared to simply leaving the
+		 * rest of the file unwritten, as was once done), but has an advantage
+		 * that the tail of the file will contain predictable (ideally constant)
+		 * data, so that general-purpose compression tools perform well on it.
+		 * (If left unwritten, the tail contains whatever is left over from the
+		 * file's last use as an earlier segment, and may compress poorly.) The
+		 * I/O cost is of little concern because any period when WAL segments
+		 * are being switched out (for, e.g., checkpoint timeout reasons) before
+		 * they are filled is clearly a low-workload period.
+		 */
 		while (CurrPos < EndPos)
 		{
-			/* initialize the next page (if not initialized already) */
-			WALInsertLockUpdateInsertingAt(CurrPos);
-			AdvanceXLInsertBuffer(CurrPos, false);
+			/*
+			 * This loop only touches full pages that follow the last actually-
+			 * used data in the segment. It will never touch the first page of a
+			 * segment; we would not be here to switch out a segment to which
+			 * nothing at all had been written.
+			 *
+			 * The minimal actions to flush the page would be to call
+			 * WALInsertLockUpdateInsertingAt(CurrPos) followed by
+			 * AdvanceXLInsertBuffer(...). The page would be left initialized
+			 * mostly to zeros, except for the page header (always the short
+			 * variant, as this is never a segment's first page).
+			 *
+			 * The large vistas of zeros are good for compressibility, but
+			 * the short headers interrupting them every XLOG_BLCKSZ (and,
+			 * worse, with values that differ from page to page) are not. The
+			 * effect varies with compression tool, but bzip2 (which compressed
+			 * best among several common tools on scantly-filled WAL segments)
+			 * compresses about an order of magnitude worse if those headers
+			 * are left in place.
+			 *
+			 * Rather than complicating AdvanceXLInsertBuffer itself (which is
+			 * called in heavily-loaded circumstances as well as this lightly-
+			 * loaded one) with variant behavior, we just use GetXLogBuffer
+			 * (which itself calls the two methods we need) to get the pointer
+			 * and re-zero the short header.
+			 */
+			currpos = GetXLogBuffer(CurrPos);
+			MemSet(currpos, 0, SizeOfXLogShortPHD);
 			CurrPos += XLOG_BLCKSZ;
 		}
 	}
-- 
2.7.3

>From f9549ec41d84e60964887d4784abe4562b0bda0f Mon Sep 17 00:00:00 2001
From: Chapman Flack 
Date: Mon, 26 Mar 2018 23:09:04 -0400
Subject: [PATCH 2/2] Add test for ensuring WAL segment is zeroed out

Switch to a new WAL file and ensure the last block in the now
switched-away-from file is completely zeroed out.

This adds a mode to check_pg_config() where the match from the
regexp can be returned, not just the literal count of matches.
Only one match is returned (the first), but given the structure
of pg_config.h that's likely to be enough.
---
 src/test/perl/TestLib.pm | 18 --
 

Re: [HACKERS] AdvanceXLInsertBuffer vs. WAL segment compressibility

[Tom Lane]

Chapman Flack  writes:
> On 03/25/18 23:27, Stephen Frost wrote:
>> 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..?

> My belief from looking at the code was that AdvanceXLInsertBuffer() is among
> the things GetXLogBuffer() does, so calling both would result in two calls
> to the former (which I don't believe would hurt, it would only
> do enough work the second time to determine it had already been done).

If that's the argument, why is the WALInsertLockUpdateInsertingAt(CurrPos)
call still there?  GetXLogBuffer() would do that too.

In any case, the new comment seems very poorly written, as it fails to
explain what's going on, and the reference to a function that is not
actually called from the vicinity of the comment doesn't help.  I'd
suggest something like "GetXLogBuffer() will fetch and initialize the
next WAL page for us.  But it initializes the page header to nonzero,
which is undesirable because X, so we then reset the header to zero".
It might also be worth explaining how you know that the new page's
header is short not long.

regards, tom lane

Re: [HACKERS] AdvanceXLInsertBuffer vs. WAL segment compressibility

[Chapman Flack]

On 03/25/18 23:27, Stephen Frost wrote:
>>  .travis.yml  | 47 
>> 
> 
> ... not something that I think we're going to add into the main tree.

Looks like that got in by mistake, sorry.

> - AdvanceXLInsertBuffer(CurrPos, false);
> ...
> + currpos = GetXLogBuffer(CurrPos);
>
> 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..?

My belief from looking at the code was that AdvanceXLInsertBuffer() is among
the things GetXLogBuffer() does, so calling both would result in two calls
to the former (which I don't believe would hurt, it would only
do enough work the second time to determine it had already been done).

However, it is done *conditionally* within GetXLogBuffer(), so it doesn't
hurt to have extra eyes reviewing my belief that the condition will be true
in this case (looping through tail blocks that haven't been touched yet).

-Chap

Re: [HACKERS] AdvanceXLInsertBuffer vs. WAL segment compressibility

[Michael Paquier]

On Sun, Mar 25, 2018 at 11:27:31PM -0400, Stephen Frost wrote:
> 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..?

Have a look at BKP_REMOVABLE then.  This was moved to page headers in
2dd9322, still it seems to me that the small benefits outlined on this
thread don't justify breaking tools relying on this flag set, especially
if there is no replacement for it.

So I would vote to not commit this patch.
--
Michael


signature.asc
Description: PGP signature

Re: [HACKERS] AdvanceXLInsertBuffer vs. WAL segment compressibility

[Stephen Frost]

Greetings,

* Chapman Flack (c...@anastigmatix.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


signature.asc
Description: PGP signature

Re: [HACKERS] AdvanceXLInsertBuffer vs. WAL segment compressibility

[Chapman Flack]

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.

-Chap
>From b2d36e3e4f4cd003cd7564a217bb15c19e1da5e2 Mon Sep 17 00:00:00 2001
From: Chapman Flack 
Date: Sun, 18 Mar 2018 20:03:04 -0400
Subject: [PATCH] Add test for ensuring WAL segment is zeroed out

Switch to a new WAL file and ensure the last block in the now
switched-away-from file is completely zeroed out.

This adds a mode to check_pg_config() where the match from the
regexp can be returned, not just the literal count of matches.
Only one match is returned (the first), but given the structure
of pg_config.h that's likely to be enough.
---
 .travis.yml  | 47 
 src/test/perl/TestLib.pm | 18 --
 src/test/recovery/t/002_archiving.pl | 19 ++-
 3 files changed, 81 insertions(+), 3 deletions(-)
 create mode 100644 .travis.yml

diff --git a/.travis.yml b/.travis.yml
new file mode 100644
index 000..386bb60
--- /dev/null
+++ b/.travis.yml
@@ -0,0 +1,47 @@
+
+sudo: required
+addons:
+  apt:
+packages:
+  - gdb
+  - lcov
+  - libipc-run-perl
+  - libperl-dev
+  - libpython-dev
+  - tcl-dev
+  - libldap2-dev
+  - libicu-dev
+  - docbook
+  - docbook-dsssl
+  - docbook-xsl
+  - libxml2-utils
+  - openjade1.3
+  - opensp
+  - xsltproc
+language: c
+cache: ccache
+before_install:
+  - echo '/tmp/%e-%s-%p.core' | sudo tee /proc/sys/kernel/core_pattern
+  - echo "deb http://archive.ubuntu.com/ubuntu xenial main" | sudo tee /etc/apt/sources.list.d/xenial.list > /dev/null
+  - |
+sudo tee -a /etc/apt/preferences.d/trusty > /dev/null /dev/null) ; do
+  binary=$(gdb -quiet -core $corefile -batch -ex 'info auxv' | grep AT_EXECFN | perl -pe "s/^.*\"(.*)\"\$/\$1/g")
+  echo dumping $corefile for $binary
+  gdb --batch --quiet -ex "thread apply all bt full" -ex "quit" $binary $corefile
+done
diff --git a/src/test/perl/TestLib.pm b/src/test/perl/TestLib.pm
index 7d017d4..8b9796c 100644
--- a/src/test/perl/TestLib.pm
+++ b/src/test/perl/TestLib.pm
@@ -227,15 +227,29 @@ sub append_to_file
 # that.
 sub check_pg_config
 {
-	my ($regexp) = @_;
+	my ($regexp, $value) = @_;
 	my ($stdout, $stderr);
+	my $match;
 	my $result = IPC::Run::run [ 'pg_config', '--includedir' ], '>',
 	  \$stdout, '2>', \$stderr
 	  or die "could not execute pg_config";
 	chomp($stdout);
 
 	open my $pg_config_h, '<', "$stdout/pg_config.h" or die "$!";
-	my $match = (grep {/^$regexp/} <$pg_config_h>);
+	if (defined $value)
+	{
+		while (<$pg_config_h>)
+		{
+			$_ =~ m/^$regexp/;
+			next unless defined $1;
+			$match = $1;
+			last;
+		}
+	}
+	else
+	{
+		$match = (grep {/^$regexp/} <$pg_config_h>);
+	}
 	close $pg_config_h;
 	return $match;
 }
diff --git a/src/test/recovery/t/002_archiving.pl b/src/test/recovery/t/002_archiving.pl
index e1bd3c9..9551db1 100644
--- a/src/test/recovery/t/002_archiving.pl
+++ b/src/test/recovery/t/002_archiving.pl
@@ -3,7 +3,7 @@ use strict;
 use warnings;
 use PostgresNode;
 use TestLib;
-use Test::More tests => 1;
+use Test::More tests => 2;
 use File::Copy;
 
 # Initialize master node, doing archives
@@ -49,3 +49,20 @@ $node_standby->poll_query_until('postgres', $caughtup_query)
 my $result =
   $node_standby->safe_psql('postgres', "SELECT count(*) FROM tab_int");
 is($result, qq(1000), 'check content from archives');
+
+# Add some content, switch WAL file and read the last segment of the WAL
+# file which should be zeroed out
+my $current_file = $node_master->safe_psql('postgres',
+	"SELECT pg_walfile_name(pg_current_wal_lsn())");
+$node_master->safe_psql('postgres', "INSERT INTO tab_int VALUES (1)");
+$node_master->safe_psql('postgres', "SELECT pg_switch_wal()");
+
+my $xlog_blcksz = check_pg_config('#define XLOG_BLCKSZ (\d+)', 1);
+
+open my $fh, '<:raw', $node_master->data_dir . '/pg_wal/' . $current_file;
+# SEEK_END is defined as 2, but is not allowed when using strict mode
+seek $fh, ($xlog_blcksz * -1), 2;
+my $len = read($fh, my $bytes, $xlog_blcksz);
+close $fh;
+
+ok($bytes =~ /\A\x00*+\z/, 'make sure wal segment is zeroed');
-- 
2.7.3

From 35684bdaa1d27c3f4b7198541f3c92bb2b4cb2f4 Mon Sep 17 00:00:00 2001
From: Chapman Flack 
Date: Sun, 25 Feb 2018 11:44:47 -0500
Subject: [PATCH] Zero headers of unused pages after WAL switch.

When writing zeroed pages to the remainder of a WAL segment
after a WAL switch, ensure that the headers of those pages are
also zeroed, as 

Re: [HACKERS] AdvanceXLInsertBuffer vs. WAL segment compressibility

[Daniel Gustafsson]

> On 18 Mar 2018, at 22:54, Chapman Flack  wrote:
> 
> On 03/18/18 16:56, Daniel Gustafsson wrote:
>> sorry about that.  Now we know that the proposed test fails without the patch
>> applied and clears with it, that was at least an interesting side effect =)
> 
> It was, and it got me looking at the test, and even though it does detect
> the difference between patch-applied and patch-not-applied, I sort of wonder
> if it does what it claims to. It seems to me that unpack('N8192', ...)
> would want to return 8192 32-bit ints (in array context), but really only
> be able to return 2048 of them (because it's only got 8192 bytes to unpack),
> and then being used in scalar context, it only returns the first one anyway,
> so the test only hinges on whether the first four bytes of the block are
> zero or not. Which turns out to be enough to catch a non-zeroed header. :)

Good point, thats what I get for hacking without enough coffee.

> What would you think about replacing the last two lines with just
> 
>  ok($bytes =~ /\A\x00*+\z/, 'make sure wal segment is zeroed’);

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.

cheers ./daniel

Re: [HACKERS] AdvanceXLInsertBuffer vs. WAL segment compressibility

[Chapman Flack]

On 03/18/18 16:56, Daniel Gustafsson wrote:
> sorry about that.  Now we know that the proposed test fails without the patch
> applied and clears with it, that was at least an interesting side effect =)

It was, and it got me looking at the test, and even though it does detect
the difference between patch-applied and patch-not-applied, I sort of wonder
if it does what it claims to. It seems to me that unpack('N8192', ...)
would want to return 8192 32-bit ints (in array context), but really only
be able to return 2048 of them (because it's only got 8192 bytes to unpack),
and then being used in scalar context, it only returns the first one anyway,
so the test only hinges on whether the first four bytes of the block are
zero or not. Which turns out to be enough to catch a non-zeroed header. :)

What would you think about replacing the last two lines with just

  ok($bytes =~ /\A\x00*+\z/, 'make sure wal segment is zeroed');

-Chap

Re: [HACKERS] AdvanceXLInsertBuffer vs. WAL segment compressibility

[Daniel Gustafsson]

> On 18 Mar 2018, at 03:09, Tom Lane  wrote:
> 
> Chapman Flack  writes:
>> Thanks for the review. I notice that cfbot has now flagged the patch as
>> failing, and when I look into it, it appears that cfbot is building with
>> your test patch, and without the xlog.c patch, and so the test naturally
>> fails. Does the cfbot require both patches to be attached to the same
>> email, in order to include them both?
> 
> I believe so --- AFAIK it does not know anything about dependencies
> between different patches, and will just try to build whatever patch(es)
> appear in the latest email on a given thread.  Munro might be able to
> provide more detail.

Right, I should’ve realized when I didn’t include your original patch as well,
sorry about that.  Now we know that the proposed test fails without the patch
applied and clears with it, that was at least an interesting side effect =)

cheers ./daniel

Re: [HACKERS] AdvanceXLInsertBuffer vs. WAL segment compressibility

[Tom Lane]

Chapman Flack  writes:
> Thanks for the review. I notice that cfbot has now flagged the patch as
> failing, and when I look into it, it appears that cfbot is building with
> your test patch, and without the xlog.c patch, and so the test naturally
> fails. Does the cfbot require both patches to be attached to the same
> email, in order to include them both?

I believe so --- AFAIK it does not know anything about dependencies
between different patches, and will just try to build whatever patch(es)
appear in the latest email on a given thread.  Munro might be able to
provide more detail.

regards, tom lane

Re: [HACKERS] AdvanceXLInsertBuffer vs. WAL segment compressibility

[Chapman Flack]

On 03/16/18 17:14, Daniel Gustafsson wrote:
> The attached patch adds the test, and a neccessary extension to 
> check_pg_config
> to allow for extracting values from pg_config.h as opposed to just returning
> the number of regex matches. (needed for XLOG_BLCKSZ.)

Thanks for the review. I notice that cfbot has now flagged the patch as
failing, and when I look into it, it appears that cfbot is building with
your test patch, and without the xlog.c patch, and so the test naturally
fails. Does the cfbot require both patches to be attached to the same
email, in order to include them both? I'll try attaching both to this one,
and see what it does.

This is good confirmation that the test is effective. :)

-Chap
From 35684bdaa1d27c3f4b7198541f3c92bb2b4cb2f4 Mon Sep 17 00:00:00 2001
From: Chapman Flack 
Date: Sun, 25 Feb 2018 11:44:47 -0500
Subject: [PATCH] Zero headers of unused pages after WAL switch.

When writing zeroed pages to the remainder of a WAL segment
after a WAL switch, ensure that the headers of those pages are
also zeroed, as their initialized values otherwise reduce the
compressibility of the WAL segment file by general tools.
---
 src/backend/access/transam/xlog.c | 11 ++-
 1 file changed, 10 insertions(+), 1 deletion(-)

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;
}
}
-- 
2.7.3

From a7cbcde7518e2f95673ec5ddba32b7ea6e0b84bb Mon Sep 17 00:00:00 2001
From: Daniel Gustafsson 
Date: Fri, 16 Mar 2018 21:32:32 +0100
Subject: [PATCH] Add test for ensuring WAL segment is zeroed out

Switch to a new WAL file and ensure the last segment in the now
switched-away-from file is completely zeroed out.

This adds a mode to check_pg_config() where the match from the
regexp can be returned, not just the literal count of matches.
Only one match is returned (the first), but given the structure
of pg_config.h that's likely to be enough.
---
 src/test/perl/TestLib.pm | 18 --
 src/test/recovery/t/002_archiving.pl | 20 +++-
 2 files changed, 35 insertions(+), 3 deletions(-)

diff --git a/src/test/perl/TestLib.pm b/src/test/perl/TestLib.pm
index 7d017d49bd..8b9796c4f6 100644
--- a/src/test/perl/TestLib.pm
+++ b/src/test/perl/TestLib.pm
@@ -227,15 +227,29 @@ sub append_to_file
 # that.
 sub check_pg_config
 {
-   my ($regexp) = @_;
+   my ($regexp, $value) = @_;
my ($stdout, $stderr);
+   my $match;
my $result = IPC::Run::run [ 'pg_config', '--includedir' ], '>',
  \$stdout, '2>', \$stderr
  or die "could not execute pg_config";
chomp($stdout);
 
open my $pg_config_h, '<', "$stdout/pg_config.h" or die "$!";
-   my $match = (grep {/^$regexp/} <$pg_config_h>);
+   if (defined $value)
+   {
+   while (<$pg_config_h>)
+   {
+   $_ =~ m/^$regexp/;
+   next unless defined $1;
+   $match = $1;
+   last;
+   }
+   }
+   else
+   {
+   $match = (grep {/^$regexp/} <$pg_config_h>);
+   }
close $pg_config_h;
return $match;
 }
diff --git a/src/test/recovery/t/002_archiving.pl 
b/src/test/recovery/t/002_archiving.pl
index e1bd3c95cc..c235e98904 100644
--- a/src/test/recovery/t/002_archiving.pl
+++ b/src/test/recovery/t/002_archiving.pl
@@ -3,7 +3,7 @@ use strict;
 use warnings;
 use PostgresNode;
 use TestLib;
-use Test::More tests => 1;
+use Test::More tests => 2;
 use File::Copy;
 
 # Initialize master node, doing archives
@@ -49,3 +49,21 @@ $node_standby->poll_query_until('postgres', $caughtup_query)
 my $result =
   $node_standby->safe_psql('postgres', "SELECT count(*) FROM tab_int");
 is($result, 

Re: [HACKERS] AdvanceXLInsertBuffer vs. WAL segment compressibility

[Daniel Gustafsson]

> On 25 Feb 2018, at 18:22, Chapman Flack  wrote:

> Here is a patch implementing the simpler approach Heikki suggested
> (though my original leaning had been to wrench on AdvanceXLInsertBuffer
> as Michael suggests). The sheer simplicity of the smaller change
> eventually won me over, unless there's a strong objection.

I had a look at this patch today.  The patch applies on current HEAD, and has
ample documentation in the included comment.  All existing tests pass, but
there are no new tests added (more on that below).  While somewhat outside my
wheelhouse, the implementation is the simple solution with no apparent traps
that I can think of.

Regarding the problem statement of the patch.  The headers on the zeroed
segments are likely an un-intended side effect, and serve no real purpose.
While they aren’t a problem to postgres, they do reduce the compressability of
the resulting files as evidenced by the patch author.

> As noted before, the cost of the extra small MemSet is proportional
> to the number of unused pages in the segment, and that is an indication
> of how busy the system *isn't*. I don't have a time benchmark of the
> patch's impact; if I should, what would be a good methodology?

This codepath doesn’t seem performance critical enough to warrant holding off
the patch awaiting a benchmark IMO.

> I considered adding a regression test for unfilled-segment compressibility,
> but wasn't sure where it would most appropriately go. I assume a TAP test
> would be the way to do it.

Adding a test that actually compress files seems hard to make stable, and adds
a dependency on external tools which is best to avoid when possible.  I took a
stab at this and added a test that ensures the last segment in the switched-
away file is completely zeroed out, which in a sense tests compressability.

The attached patch adds the test, and a neccessary extension to check_pg_config
to allow for extracting values from pg_config.h as opposed to just returning
the number of regex matches. (needed for XLOG_BLCKSZ.)

That being said, I am not entirely convinced that the test is adding much
value.  It’s however easier to reason about a written patch than an idea so I
figured I’d add it here either way.

Setting this to Ready for Committer and offering my +1 on getting this in.

cheers ./daniel



wal_zeroed_test.patch
Description: Binary data

Re: [HACKERS] AdvanceXLInsertBuffer vs. WAL segment compressibility

[Chapman Flack]

On 07/17/17 11:29, Michael Paquier wrote:
> On Thu, Jul 6, 2017 at 3:48 PM, Heikki Linnakangas  wrote:
>> On 07/03/2017 06:30 PM, Chapman Flack wrote:
>>> Although it's moot in the straightforward approach of re-zeroing in
>>> the loop, it would still help my understanding of the system to know
>>> if there is some subtlety that would have broken what I proposed
>>> earlier, which was an extra flag to AdvanceXLInsertBuffer() that
>>> ...
>>
>> Yeah, I suppose that would work, too.
> 
> FWIW, I would rather see any optimization done in
> AdvanceXLInsertBuffer() instead of seeing a second memset re-zeroing
> the WAL page header after its data has been initialized by
> AdvanceXLInsertBuffer() once.

Here is a patch implementing the simpler approach Heikki suggested
(though my original leaning had been to wrench on AdvanceXLInsertBuffer
as Michael suggests). The sheer simplicity of the smaller change
eventually won me over, unless there's a strong objection.

As noted before, the cost of the extra small MemSet is proportional
to the number of unused pages in the segment, and that is an indication
of how busy the system *isn't*. I don't have a time benchmark of the
patch's impact; if I should, what would be a good methodology?

Before the change, what some common compression tools can achieve on
a mostly empty (select pg_switch_wal()) segment on my hardware are:

gzip  27052 in 0.145s
xz 5852 in 0.678s
lzip   5747 in 1.254s
bzip2  1445 in 0.261s

bzip2 is already the clear leader (I don't have lz4 handy to include in
the comparison) at around 1/20th size gzip can achieve, and that's before
this patch. After:

gzip 16393 in 0.143s
xz2640 in 0.520s
lzip  2535 in 1.198s
bzip2  147 in 0.238s

The patch gives gzip almost an extra factor of two, and about the same
for xz and lzip, and bzip2 gains nearly another order of magnitude.

I considered adding a regression test for unfilled-segment compressibility,
but wasn't sure where it would most appropriately go. I assume a TAP test
would be the way to do it.

-Chap
>From 35684bdaa1d27c3f4b7198541f3c92bb2b4cb2f4 Mon Sep 17 00:00:00 2001
From: Chapman Flack 
Date: Sun, 25 Feb 2018 11:44:47 -0500
Subject: [PATCH] Zero headers of unused pages after WAL switch.

When writing zeroed pages to the remainder of a WAL segment
after a WAL switch, ensure that the headers of those pages are
also zeroed, as their initialized values otherwise reduce the
compressibility of the WAL segment file by general tools.
---
 src/backend/access/transam/xlog.c | 11 ++-
 1 file changed, 10 insertions(+), 1 deletion(-)

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;
 		}
 	}
-- 
2.7.3