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