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

Hello,

>Yes, that's caused by ccb161b. Attached are rebased versions. 

Following are some comments, 

>uint16  hole_offset:15, /* number of bytes in "hole" */
Typo in description of hole_offset
           
>        for (block_id = 0; block_id <= record->max_block_id; block_id++)
>-       {
>-               if (XLogRecHasBlockImage(record, block_id))
>-                       fpi_len += BLCKSZ -
record->blocks[block_id].hole_length;
>-       }
>+               fpi_len += record->blocks[block_id].bkp_len;

IIUC, if condition, /if(XLogRecHasBlockImage(record, block_id))/ is
incorrectly removed from the above for loop.

>typedef struct XLogRecordCompressedBlockImageHeader
I am trying to understand the purpose behind declaration of the above
struct. IIUC, it is defined in order to introduce new field uint16 
raw_length and it has been declared as a separate struct from
XLogRecordBlockImageHeader to not affect the size of WAL record when
compression is off.
I wonder if it is ok to simply memcpy the uint16 raw_length in the
hdr_scratch when compression is on
and not have a separate header struct for it neither declare it in existing
header. raw_length can be a locally defined variable is XLogRecordAssemble
or it can be a field in registered_buffer struct like compressed_page.
I think this can simplify the code. 
Am I missing something obvious?   

> /*
>  * Fill in the remaining fields in the XLogRecordBlockImageHeader
>  * struct and add new entries in the record chain.
> */  
>   bkpb.fork_flags |= BKPBLOCK_HAS_IMAGE;

This code line seems to be misplaced with respect to the above comment. 
Comment indicates filling of XLogRecordBlockImageHeader fields while
fork_flags is a field of XLogRecordBlockHeader.
Is it better to place the code close to following condition? if (needs_backup) {

>+  *the original length of the
>+ * block without its page hole being deducible from the compressed data
>+ * itself.
IIUC, this comment before XLogRecordBlockImageHeader seems to be no longer
valid as original length is not deducible from compressed data and rather
stored in header.


Thank you,
Rahila Syed



--
View this message in context: http://postgresql.nabble.com/Compression-of-full-page-writes-tp5769039p5833025.html
Sent from the PostgreSQL - hackers mailing list archive at Nabble.com.