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

Hello,

>> Do we always need extra two bytes for compressed backup block?
>> ISTM that extra bytes are not necessary when the hole length is zero.
>> In this case the length of the original backup block (i.e.,
>> uncompressed) must be BLCKSZ, so we don't need to save the original
>> size in the extra bytes.

>Yes, we would need a additional bit to identify that. We could steal it from length in XLogRecordBlockImageHeader.

This is implemented in the attached patch by dividing length field as follows,
uint16 length:15,
with_hole:1;

>"2" should be replaced with the macro variable indicating the size of
>extra header for compressed backup block.
Macro SizeOfXLogRecordBlockImageCompressionInfo is used instead of 2

>You can refactor XLogCompressBackupBlock() and move all the
>above code to it for more simplicity
This is also implemented in the patch attached.

Thank you,
Rahila Syed

-----Original Message-----
From: Michael Paquier [mailto:michael(dot)paquier(at)gmail(dot)com]
Sent: Friday, February 06, 2015 6:00 PM
To: Fujii Masao
Cc: Syed, Rahila; PostgreSQL mailing lists
Subject: Re: [HACKERS] [REVIEW] Re: Compression of full-page-writes

On Fri, Feb 6, 2015 at 3:03 PM, Fujii Masao wrote:
> Do we always need extra two bytes for compressed backup block?
> ISTM that extra bytes are not necessary when the hole length is zero.
> In this case the length of the original backup block (i.e.,
> uncompressed) must be BLCKSZ, so we don't need to save the original
> size in the extra bytes.

Yes, we would need a additional bit to identify that. We could steal it from length in XLogRecordBlockImageHeader.

> Furthermore, when fpw compression is disabled and the hole length is
> zero, we seem to be able to save one byte from the header of backup
> block. Currently we use 4 bytes for the header, 2 bytes for the length
> of backup block, 15 bits for the hole offset and 1 bit for the flag
> indicating whether block is compressed or not. But in that case, the
> length of backup block doesn't need to be stored because it must be
> BLCKSZ. Shouldn't we optimize the header in this way? Thought?

If we do it, that's something to tackle even before this patch on HEAD, because you could use the 16th bit of the first 2 bytes of XLogRecordBlockImageHeader to do necessary sanity checks, to actually not reduce record by 1 byte, but 2 bytes as hole-related data is not necessary. I imagine that a patch optimizing that wouldn't be that hard to write as well.

> + int page_len = BLCKSZ - hole_length;
> + char *scratch_buf;
> + if (hole_length != 0)
> + {
> + scratch_buf = compression_scratch;
> + memcpy(scratch_buf, page, hole_offset);
> + memcpy(scratch_buf + hole_offset,
> + page + (hole_offset + hole_length),
> + BLCKSZ - (hole_length + hole_offset));
> + }
> + else
> + scratch_buf = page;
> +
> + /* Perform compression of block */
> + if (XLogCompressBackupBlock(scratch_buf,
> + page_len,
> + regbuf->compressed_page,
> + &compress_len))
> + {
> + /* compression is done, add record */
> + is_compressed = true;
> + }
>
> You can refactor XLogCompressBackupBlock() and move all the above code
> to it for more simplicity.

Sure.
--
Michael

______________________________________________________________________
Disclaimer: This email and any attachments are sent in strictest confidence
for the sole use of the addressee and may contain legally privileged,
confidential, and proprietary data. If you are not the intended recipient,
please advise the sender by replying promptly to this email and then delete
and destroy this email and any attachments without any further use, copying
or forwarding.