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

On Wed, Dec 17, 2014 at 11:33 PM, Rahila Syed <rahilasyed90(at)gmail(dot)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