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, > > XLR_CHUNK_ID_DATA_SHORT > > XLR_CHUNK_ID_DATA_LONG > > XLR_CHUNK_BKP_COMPRESSED > > XLR_CHUNK_BKP_WITH_HOLE > > > > 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 that: typedef struct XLogRecordBlockImageHeader { uint32 length:15, hole_length:15, is_compressed:1, is_hole:1; } 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 byte. 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) -- Michael