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.