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