> On 25 Feb 2018, at 18:22, Chapman Flack <c...@anastigmatix.net> 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