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

From: Michael Paquier <michael(dot)paquier(at)gmail(dot)com>
To: "Syed, Rahila" <Rahila(dot)Syed(at)nttdata(dot)com>
Cc: PostgreSQL mailing lists <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: [REVIEW] Re: Compression of full-page-writes
Date: 2015-02-05 19:15:33
Message-ID: CAB7nPqQhu=3611fF0nDXNN=Gv2f8fZTa6G-FQBbo6TS9k=5xTg@mail.gmail.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

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

Attachment Content-Type Size
Support-compression-for-full-page-writes-in-WAL_v16.patch text/x-diff 23.8 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Jan Wieck 2015-02-05 19:15:52 Re: Possible problem with pgcrypto
Previous Message Josh Berkus 2015-02-05 19:11:10 Re: Redesigning checkpoint_segments