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

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.
>
> Following are WAL numbers based on attached test script posted by Michael earlier in the thread.
>
> WAL generated
> FPW compression on 122.032 MB
>
> FPW compression off 155.223 MB
>
> HEAD 155.236 MB
>
> Compression : 21 %
> Number of block images generated in WAL : 63637

ISTM that we are getting a nice thing here. I tested the patch and WAL
replay is working correctly.

Some nitpicky comments...

+ * bkp_info stores flags for information about the backup block image
+ * BKPIMAGE_IS_COMPRESSED is used to identify if a given block image
is compressed.
+ * BKPIMAGE_WITH_HOLE is used to identify the presence of a hole in a
block image.
+ * 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.
Take care of the limit of 80 characters per line. (Perhaps you could
run pgindent on your code before sending a patch?). The first line of
this paragraph is a sentence in itself, no?

In xlogreader.c, blk->with_hole is a boolean, you could remove the ==0
and ==1 it is compared with.

+ /*
+ * Length of a block image must be less than BLCKSZ
+ * if the block has hole
+ */
"if the block has a hole." (End of the sentence needs a dot.)

+ /*
+ * Length of a block image must be equal to BLCKSZ
+ * if the block does not have hole
+ */
"if the block does not have a hole."

Regards,
--
Michael