On Wed, Mar 4, 2015 at 12:41 AM, Syed, Rahila <rahila.s...@nttdata.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:

+#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

-                       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.

Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:

Reply via email to