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 been added 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?