Re: [REVIEW] Re: Compression of full-page-writes - Mailing list pgsql-hackers

From Michael Paquier
Subject Re: [REVIEW] Re: Compression of full-page-writes
Date
Msg-id CAB7nPqTLXva1J_N_u=kX-JAKRyOaU=T38uhFnbM4aMtMxRRdAQ@mail.gmail.com
Whole thread Raw
In response to Re: [REVIEW] Re: Compression of full-page-writes  (Rahila Syed <rahilasyed90@gmail.com>)
Responses Re: [REVIEW] Re: Compression of full-page-writes  (Fujii Masao <masao.fujii@gmail.com>)
List pgsql-hackers


On Wed, Dec 17, 2014 at 11:33 PM, Rahila Syed <rahilasyed90@gmail.com> wrote:
I had a look at code. I have few minor points,
Thanks!

+           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.
OK, why not...
 
+               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.
Yeah that seems clearer, let's define it as ~XLR_BLCK_COMPRESSED_MASK though.

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.
Fixed. As well as an additional comment block down.

A couple of things noticed on the fly:
- Fixed pg_xlogdump being not completely correct to report the FPW information
- A couple of typos and malformed sentences fixed
- Added an assertion to check that the hole offset value does not the bit used for compression status
- Reworked docs, mentioning as well that wal_compression is off by default.
- Removed stuff in pg_controldata and XLOG_PARAMETER_CHANGE (mentioned by Fujii-san)

Regards,
--
Michael
Attachment

pgsql-hackers by date:

Previous
From: Petr Jelinek
Date:
Subject: Re: TABLESAMPLE patch
Next
From: Ashutosh Bapat
Date:
Subject: Re: Async execution of postgres_fdw.