Re: [REVIEW] Re: Compression of full-page-writes

Hello,

>Patches as well as results are attached.

I had a look at code. I have few minor points,

+ bkpb.fork_flags |= BKPBLOCK_HAS_IMAGE;
+
+ if (is_compressed)
{
- rdt_datas_last->data = page;
- rdt_datas_last->len = BLCKSZ;
+ /* compressed block information */
+ bimg.length = compress_len;
+ bimg.extra_data = hole_offset;
+ bimg.extra_data |= XLR_BLCK_COMPRESSED_MASK;

For consistency with the existing code , how about renaming the macro
XLR_BLCK_COMPRESSED_MASK as BKPBLOCK_HAS_COMPRESSED_IMAGE on the lines of
BKPBLOCK_HAS_IMAGE.

+ blk->hole_offset = extra_data & ~XLR_BLCK_COMPRESSED_MASK;
Here , I think that having the mask as BKPBLOCK_HOLE_OFFSET_MASK will be
more indicative of the fact that lower 15 bits of extra_data field
comprises of hole_offset value. This suggestion is also just to achieve
consistency with the existing BKPBLOCK_FORK_MASK for fork_flags field.

And comment typo
+ * First try to compress block, filling in the page hole with
zeros
+ * to improve the compression of the whole. If the block is
considered
+ * as incompressible, complete the block header information as
if
+ * nothing happened.

As hole is no longer being compressed, this needs to be changed.

Thank you,
Rahila Syed

On Tue, Dec 16, 2014 at 10:04 PM, Michael Paquier <michael(dot)paquier(at)gmail(dot)com
> wrote:
>
>
>
> On Wed, Dec 17, 2014 at 12:00 AM, Michael Paquier <
> michael(dot)paquier(at)gmail(dot)com> wrote:
>>
>> Actually, the original length of the compressed block in saved in
>> PGLZ_Header, so if we are fine to not check the size of the block
>> decompressed when decoding WAL we can do without the hole filled with
>> zeros, and use only 1 bit to see if the block is compressed or not.
>>
> And.. After some more hacking, I have been able to come up with a patch
> that is able to compress blocks without the page hole, and that keeps the
> WAL record length the same as HEAD when compression switch is off. The
> numbers are pretty good, CPU is saved in the same proportions as previous
> patches when compression is enabled, and there is zero delta with HEAD when
> compression switch is off.
>
> Here are the actual numbers:
> test_name | pg_size_pretty | user_diff | system_diff
> -------------------------------+----------------+-----------+-------------
> FPW on + 2 bytes, ffactor 50 | 582 MB | 42.391894 | 0.807444
> FPW on + 2 bytes, ffactor 20 | 229 MB | 14.330304 | 0.729626
> FPW on + 2 bytes, ffactor 10 | 117 MB | 7.335442 | 0.570996
> FPW off + 2 bytes, ffactor 50 | 746 MB | 25.330391 | 1.248503
> FPW off + 2 bytes, ffactor 20 | 293 MB | 10.537475 | 0.755448
> FPW off + 2 bytes, ffactor 10 | 148 MB | 5.762775 | 0.763761
> FPW on + 0 bytes, ffactor 50 | 582 MB | 42.174297 | 0.790596
> FPW on + 0 bytes, ffactor 20 | 229 MB | 14.424233 | 0.770459
> FPW on + 0 bytes, ffactor 10 | 117 MB | 7.057195 | 0.584806
> FPW off + 0 bytes, ffactor 50 | 746 MB | 25.261998 | 1.054516
> FPW off + 0 bytes, ffactor 20 | 293 MB | 10.589888 | 0.860207
> FPW off + 0 bytes, ffactor 10 | 148 MB | 5.827191 | 0.874285
> HEAD, ffactor 50 | 746 MB | 25.181729 | 1.133433
> HEAD, ffactor 20 | 293 MB | 9.962242 | 0.765970
> HEAD, ffactor 10 | 148 MB | 5.693426 | 0.775371
> Record, ffactor 50 | 582 MB | 54.904374 | 0.678204
> Record, ffactor 20 | 229 MB | 19.798268 | 0.807220
> Record, ffactor 10 | 116 MB | 9.401877 | 0.668454
> (18 rows)
>
> The new tests of this patch are "FPW off + 0 bytes". Patches as well as
> results are attached.
> Regards,
> --
> Michael
>