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

>In any case, those things have been introduced by what I did in previous versions... And attached is a new patch.
Thank you for feedback.

> /* allocate scratch buffer used for compression of block images */
>+ if (compression_scratch == NULL)
>+ compression_scratch = MemoryContextAllocZero(xloginsert_cxt,
>+ BLCKSZ);
>}
The compression patch can use the latest interface MemoryContextAllocExtended to proceed without compression when sufficient memory is not available for
scratch buffer.
The attached patch introduces OutOfMem flag which is set on when MemoryContextAllocExtended returns NULL .

Thank you,
Rahila Syed

-----Original Message-----
From: Michael Paquier [mailto:michael(dot)paquier(at)gmail(dot)com]
Sent: Friday, February 06, 2015 12:46 AM
To: Syed, Rahila
Cc: PostgreSQL mailing lists
Subject: Re: [HACKERS] [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

______________________________________________________________________
Disclaimer: This email and any attachments are sent in strictest confidence
for the sole use of the addressee and may contain legally privileged,
confidential, and proprietary data. If you are not the intended recipient,
please advise the sender by replying promptly to this email and then delete
and destroy this email and any attachments without any further use, copying
or forwarding.