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

On Fri, Feb 6, 2015 at 4:15 AM, Michael Paquier
<michael(dot)paquier(at)gmail(dot)com> wrote:
> On Thu, Feb 5, 2015 at 11:06 PM, Syed, Rahila <Rahila(dot)Syed(at)nttdata(dot)com> wrote:
>>>/*
>>>+ * We recheck the actual size even if pglz_compress() report success,
>>>+ * because it might be satisfied with having saved as little as one byte
>>>+ * in the compressed data.
>>>+ */
>>>+ *len = (uint16) compressed_len;
>>>+ if (*len >= orig_len - 1)
>>>+ return false;
>>>+ return true;
>>>+}
>>
>> As per latest code ,when compression is 'on' we introduce additional 2 bytes in the header of each block image for storing raw_length of the compressed block.
>> In order to achieve compression while accounting for these two additional bytes, we must ensure that compressed length is less than original length - 2.
>> So , IIUC the above condition should rather be
>>
>> If (*len >= orig_len -2 )
>> return false;

"2" should be replaced with the macro variable indicating the size of
extra header for compressed backup block.

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.

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?

+ 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.

Regards,

--
Fujii Masao