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

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;
> return true;
> The attached patch contains this. It also has a cosmetic change- renaming compressBuf to uncompressBuf as it is used to store uncompressed page.

Agreed on both things.

Just looking at your latest patch after some time to let it cool down,
I noticed a couple of things.

#define MaxSizeOfXLogRecordBlockHeader \
(SizeOfXLogRecordBlockHeader + \
- SizeOfXLogRecordBlockImageHeader + \
+ SizeOfXLogRecordBlockImageHeader, \
+ SizeOfXLogRecordBlockImageCompressionInfo + \
There is a comma here instead of a sum sign. We should really sum up
all those sizes to evaluate the maximum size of a block header.

+ * Permanently allocate readBuf uncompressBuf. We do it this way,
+ * rather than just making a static array, for two reasons:
This comment is just but weird, "readBuf AND uncompressBuf" is more appropriate.

+ * 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. We add two bytes to store raw_length with the
+ * compressed image. So for compression to be effective
compressed_len should
+ * be atleast < orig_len - 2
This comment block should be reworked, and misses a dot at its end. I
rewrote it like that, hopefully that's clearer:
+ /*
+ * We recheck the actual size even if pglz_compress() reports
success and see
+ * if at least 2 bytes of length have been saved, as this
corresponds to the
+ * additional amount of data stored in WAL record for a compressed block
+ * via raw_length.
+ */

In any case, those things have been introduced by what I did in
previous versions... And attached is a new patch.
--
Michael