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

On Wed, Mar 4, 2015 at 12:41 AM, Syed, Rahila <Rahila(dot)Syed(at)nttdata(dot)com> wrote:
> Please find attached updated patch with WAL replay error fixed. The patch follows chunk ID approach of xlog format.

(Review done independently of the chunk_id stuff being good or not,
already gave my opinion on the matter).

* readRecordBufSize is set to the new buffer size.
- *
+
The patch has some noise diffs.

You may want to change the values of BKPBLOCK_WILL_INIT and
BKPBLOCK_SAME_REL to respectively 0x01 and 0x02.

+ uint8 chunk_id = 0;
+ chunk_id |= XLR_CHUNK_BLOCK_REFERENCE;

Why not simply that:
chunk_id = XLR_CHUNK_BLOCK_REFERENCE;

+#define XLR_CHUNK_ID_DATA_SHORT 255
+#define XLR_CHUNK_ID_DATA_LONG 254
Why aren't those just using one bit as well? This seems inconsistent
with the rest.

+ if ((blk->with_hole == 0 && blk->hole_offset != 0) ||
+ (blk->with_hole == 1 && blk->hole_offset <= 0))
In xlogreader.c blk->with_hole is defined as a boolean but compared
with an integer, could you remove the ==0 and ==1 portions for
clarity?

- goto err;
+ goto err;
}
}
-
if (remaining != datatotal)
This gathers incorrect code alignment and unnecessary diffs.

typedef struct XLogRecordBlockHeader
{
+ /* Chunk ID precedes */
+
uint8 id;
What prevents the declaration of chunk_id as an int8 here instead of
this comment? This is confusing.

> Following are brief measurement numbers.
> WAL
> FPW compression on 122.032 MB
> FPW compression off 155.239 MB
> HEAD 155.236 MB

What is the test run in this case? How many block images have been
generated in WAL for each case? You could gather some of those numbers
with pg_xlogdump --stat for example.
--
Michael