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

Hello,

>I have some minor comments

The comments have been implemented in the attached patch.

>I think that extra parenthesis should be used for the first expression
>with BKPIMAGE_HAS_HOLE.
Parenthesis have been added to improve code readability.

Thank you,
Rahila Syed

On Mon, Mar 9, 2015 at 5:38 PM, Michael Paquier <michael(dot)paquier(at)gmail(dot)com>
wrote:

> On Mon, Mar 9, 2015 at 4:29 PM, Fujii Masao <masao(dot)fujii(at)gmail(dot)com> wrote:
> > On Thu, Mar 5, 2015 at 10:08 PM, Michael Paquier
> > <michael(dot)paquier(at)gmail(dot)com> wrote:
> >> On Thu, Mar 5, 2015 at 9:14 PM, Syed, Rahila <Rahila(dot)Syed(at)nttdata(dot)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?
>
> Regards,
> --
> Michael
>
>
> --
> Sent via pgsql-hackers mailing list (pgsql-hackers(at)postgresql(dot)org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers
>