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

On Wed, Feb 11, 2015 at 11:03 PM, Syed, Rahila <Rahila(dot)Syed(at)nttdata(dot)com>
wrote:

> >IMO, we should add details about how this new field is used in the
> comments on top of XLogRecordBlockImageHeader, meaning that when a page
> hole is present we use the compression info structure and when there is no
> hole, we are sure that the FPW raw length is BLCKSZ meaning that the two
> bytes of the CompressionInfo stuff is unnecessary.
> This comment is included in the patch attached.
>
> > For correctness with_hole should be set even for uncompressed pages. I
> think that we should as well use it for sanity checks in xlogreader.c when
> decoding records.
> This change is made in the attached patch. Following sanity checks have
> been added in xlogreader.c
>
> if (!(blk->with_hole) && blk->hole_offset != 0 || blk->with_hole &&
> blk->hole_offset <= 0))
>
> if (blk->with_hole && blk->bkp_len >= BLCKSZ)
>
> if (!(blk->with_hole) && blk->bkp_len != BLCKSZ)
>

Cool, thanks!

This patch fails to compile:
xlogreader.c:1049:46: error: extraneous ')' after condition, expected a
statement
blk->with_hole && blk->hole_offset
<= 0))

Note as well that at least clang does not like much how the sanity check
with with_hole are done. You should place parentheses around the '&&'
expressions. Also, I would rather define with_hole == 0 or with_hole == 1
explicitly int those checks.

There is a typo:
s/true,see/true, see/

[nitpicky]Be as well aware of the 80-character limit per line that is
usually normally by comment blocks.[/]

+ * "with_hole" is used to identify the presence of hole in a block.
+ * As mentioned above, length of block cannnot be more than 15-bit long.
+ * So, the free bit in the length field is used by "with_hole" to identify
presence of
+ * XLogRecordBlockImageCompressionInfo. If hole is not present ,the raw
size of
+ * a compressed block is equal to BLCKSZ therefore
XLogRecordBlockImageCompressionInfo
+ * for the corresponding compressed block need not be stored in header.
+ * If hole is present raw size is stored.
I would rewrite this paragraph as follows, fixing the multiple typos:
"with_hole" is used to identify the presence of a hole in a block image. As
the length of a block cannot be more than 15-bit long, the extra bit in the
length field is used for this identification purpose. If the block image
has no hole, it is ensured that the raw size of a compressed block image is
equal to BLCKSZ, hence the contents of XLogRecordBlockImageCompressionInfo
are not necessary.

+ /* Followed by the data related to compression if block is
compressed */
This comment needs to be updated to "if block image is compressed and has a
hole".

+ lp_off and lp_len fields in ItemIdData (see include/storage/itemid.h)
and
+ XLogRecordBlockImageHeader where page hole offset and length is limited
to 15-bit
+ length (see src/include/access/xlogrecord.h).
80-character limit...

Regards
--
Michael