Re: [REVIEW] Re: Compression of full-page-writes - Mailing list pgsql-hackers
| From | Rahila Syed |
|---|---|
| Subject | Re: [REVIEW] Re: Compression of full-page-writes |
| Date | |
| Msg-id | CAH2L28uppHy3JusNEj7P-6n7DYgZkYQKxpb976G0QtWhH=iWPw@mail.gmail.com Whole thread Raw |
| In response to | Re: [REVIEW] Re: Compression of full-page-writes (Michael Paquier <michael.paquier@gmail.com>) |
| Responses |
Re: [REVIEW] Re: Compression of full-page-writes
(Michael Paquier <michael.paquier@gmail.com>)
|
| List | pgsql-hackers |
Hello,
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.paquier@gmail.com> wrote:
On Wed, Dec 17, 2014 at 12:00 AM, Michael Paquier <michael.paquier@gmail.com> wrote: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.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.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
pgsql-hackers by date: