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

On Fri, Feb 6, 2015 at 3:03 PM, Fujii Masao wrote:
> Do we always need extra two bytes for compressed backup block?
> ISTM that extra bytes are not necessary when the hole length is zero.
> In this case the length of the original backup block (i.e., uncompressed)
> must be BLCKSZ, so we don't need to save the original size in
> the extra bytes.

Yes, we would need a additional bit to identify that. We could steal
it from length in XLogRecordBlockImageHeader.

> Furthermore, when fpw compression is disabled and the hole length
> is zero, we seem to be able to save one byte from the header of
> backup block. Currently we use 4 bytes for the header, 2 bytes for
> the length of backup block, 15 bits for the hole offset and 1 bit for
> the flag indicating whether block is compressed or not. But in that case,
> the length of backup block doesn't need to be stored because it must
> be BLCKSZ. Shouldn't we optimize the header in this way? Thought?

If we do it, that's something to tackle even before this patch on
HEAD, because you could use the 16th bit of the first 2 bytes of
XLogRecordBlockImageHeader to do necessary sanity checks, to actually
not reduce record by 1 byte, but 2 bytes as hole-related data is not
necessary. I imagine that a patch optimizing that wouldn't be that
hard to write as well.

> +                int page_len = BLCKSZ - hole_length;
> +                char *scratch_buf;
> +                if (hole_length != 0)
> +                {
> +                    scratch_buf = compression_scratch;
> +                    memcpy(scratch_buf, page, hole_offset);
> +                    memcpy(scratch_buf + hole_offset,
> +                           page + (hole_offset + hole_length),
> +                           BLCKSZ - (hole_length + hole_offset));
> +                }
> +                else
> +                    scratch_buf = page;
> +
> +                /* Perform compression of block */
> +                if (XLogCompressBackupBlock(scratch_buf,
> +                                            page_len,
> +                                            regbuf->compressed_page,
> +                                            &compress_len))
> +                {
> +                    /* compression is done, add record */
> +                    is_compressed = true;
> +                }
>
> You can refactor XLogCompressBackupBlock() and move all the
> above code to it for more simplicity.

Sure.
-- 
Michael