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

On Thu, Feb 5, 2015 at 11:06 PM, Syed, Rahila <Rahila.Syed@nttdata.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
storingraw_length of the compressed block. 
> In order to achieve compression while accounting for these two additional bytes, we must ensure that compressed
lengthis 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
tostore 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