Re: [REVIEW] Re: Compression of full-page-writes
| От | Michael Paquier |
|---|---|
| Тема | Re: [REVIEW] Re: Compression of full-page-writes |
| Дата | |
| Msg-id | CAB7nPqRCcEHK4cKpTLjFKSqFaiLos6ZWkr5h_vamHw-bRjQTGw@mail.gmail.com обсуждение исходный текст |
| Ответ на | Re: [REVIEW] Re: Compression of full-page-writes ("Syed, Rahila" <Rahila.Syed@nttdata.com>) |
| Ответы |
Re: [REVIEW] Re: Compression of full-page-writes
("Syed, Rahila" <Rahila.Syed@nttdata.com>)
|
| Список | pgsql-hackers |
On Wed, Feb 11, 2015 at 11:03 PM, Syed, Rahila <Rahila.Syed@nttdata.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))
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).
+ 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
В списке pgsql-hackers по дате отправления: