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.paqu...@gmail.com
> wrote:
>
>
>
> On Wed, Dec 17, 2014 at 12:00 AM, Michael Paquier <
> michael.paqu...@gmail.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
>

Reply via email to