On Mon, Feb 23, 2015 at 9:22 PM, Fujii Masao <masao.fu...@gmail.com> wrote:

> On Mon, Feb 23, 2015 at 5:28 PM, Rahila Syed <rahilasye...@gmail.com>
> wrote:
> > Hello,
> >
> > Attached is a patch which has following changes,
> >
> > As suggested above block ID in xlog structs has been replaced by chunk
> ID.
> > Chunk ID is used to distinguish between different types of xlog record
> > fragments.
> > Like,
> >
> > In block references, block ID follows the chunk ID. Here block ID retains
> > its functionality.
> > This approach increases data by 1 byte for each block reference in an
> xlog
> > record. This approach separates ID referring different fragments of xlog
> > record from the actual block ID which is used to refer  block references
> in
> > xlog record.
> I've not read this logic yet, but ISTM there is a bug in that new WAL
> format
> because I got the following error and the startup process could not replay
> any WAL records when I set up replication and enabled wal_compression.
> LOG:  record with invalid length at 0/30000B0
> LOG:  record with invalid length at 0/3000518
> LOG:  Invalid block length in record 0/30005A0
> LOG:  Invalid block length in record 0/3000D60

Looking at this code, I think that it is really confusing to move the data
related to the status of the backup block out of XLogRecordBlockImageHeader
to the chunk ID itself that may *not* include a backup block at all as it
is conditioned by the presence of BKPBLOCK_HAS_IMAGE. I would still prefer
the idea of having the backup block data in its dedicated header with bits
stolen from the existing fields, perhaps by rewriting it to something like
typedef struct XLogRecordBlockImageHeader {
uint32 length:15,
} XLogRecordBlockImageHeader;
Now perhaps I am missing something and this is really "ugly" ;)

+#define XLR_CHUNK_ID_DATA_SHORT                255
+#define XLR_CHUNK_ID_DATA_LONG         254
+#define XLR_CHUNK_BKP_COMPRESSED       0x01
+#define XLR_CHUNK_BKP_WITH_HOLE                0x02
Wouldn't we need a XLR_CHUNK_ID_BKP_HEADER or equivalent? The idea between
this chunk_id stuff it to be able to make the difference between a short
header, a long header and a backup block header by looking at the first

The comments on top of XLogRecordBlockImageHeader are still mentioning the
old parameters like with_hole or is_compressed that you have removed.

It seems as well that there is some noise:
-   lp_off and lp_len fields in ItemIdData (see include/storage/itemid.h).
+   lp_off and lp_len fields in ItemIdData (see include/storage/itemid.h)

Reply via email to