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

On Mon, Mar 9, 2015 at 4:29 PM, Fujii Masao <masao.fujii@gmail.com> wrote:
> On Thu, Mar 5, 2015 at 10:08 PM, Michael Paquier
> <michael.paquier@gmail.com> wrote:
>> On Thu, Mar 5, 2015 at 9:14 PM, Syed, Rahila <Rahila.Syed@nttdata.com> wrote:
>>> Please find attached  a patch. As discussed, flag to denote compression and presence of hole in block image has
beenadded in XLogRecordImageHeader rather than block header.
 
>
> Thanks for updating the patch! Attached is the refactored version of the patch.

Cool. Thanks!

I have some minor comments:

+        The default value is <literal>off</>
Dot at the end of this sentence.

+        Turning this parameter on can reduce the WAL volume without
"Turning <value>on</> this parameter

+        but at the cost of some extra CPU time by the compression during
+        WAL logging and the decompression during WAL replay."
Isn't a verb missing here, for something like that:
"but at the cost of some extra CPU spent on the compression during WAL
logging and on the decompression during WAL replay."

+ * This can reduce the WAL volume, but at some extra cost of CPU time
+ * by the compression during WAL logging.
Er, similarly "some extra cost of CPU spent on the compression...".

+       if (blk->bimg_info & BKPIMAGE_HAS_HOLE &&
+                       (blk->hole_offset == 0 ||
+                        blk->hole_length == 0 ||
I think that extra parenthesis should be used for the first expression
with BKPIMAGE_HAS_HOLE.

+                               if (blk->bimg_info & BKPIMAGE_IS_COMPRESSED &&
+                                       blk->bimg_len == BLCKSZ)
+                               {
Same here.

+                               /*
+                                * cross-check that hole_offset == 0
and hole_length == 0
+                                * if the HAS_HOLE flag is set.
+                                */
I think that you mean here that this happens when the flag is *not* set.

+       /*
+        * If BKPIMAGE_HAS_HOLE and BKPIMAGE_IS_COMPRESSED,
+        * an XLogRecordBlockCompressHeader follows
+        */
Maybe a "struct" should be added for "an XLogRecordBlockCompressHeader
struct". And a dot at the end of the sentence should be added?

Regards,
-- 
Michael