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

Hello,

>/*
>+ * We recheck the actual size even if pglz_compress() report success,
>+ * because it might be satisfied with having saved as little as one byte
>+ * in the compressed data.
>+ */
>+ *len = (uint16) compressed_len;
>+ if (*len >= orig_len - 1)
>+ return false;
>+ return true;
>+}

As per latest code ,when compression is 'on' we introduce additional 2 bytes in the header of each block image for storing raw_length of the compressed block.
In order to achieve compression while accounting for these two additional bytes, we must ensure that compressed length is less than original length - 2.
So , IIUC the above condition should rather be

If (*len >= orig_len -2 )
return false;
return true;

The attached patch contains this. It also has a cosmetic change- renaming compressBuf to uncompressBuf as it is used to store uncompressed page.

Thank you,
Rahila Syed

-----Original Message-----
From: pgsql-hackers-owner(at)postgresql(dot)org [mailto:pgsql-hackers-owner(at)postgresql(dot)org] On Behalf Of Michael Paquier
Sent: Wednesday, January 07, 2015 9:32 AM
To: Rahila Syed
Cc: PostgreSQL mailing lists
Subject: Re: [HACKERS] [REVIEW] Re: Compression of full-page-writes

On Wed, Jan 7, 2015 at 12:51 AM, Rahila Syed <rahilasyed(dot)90(at)gmail(dot)com> wrote:
> Following are some comments,
Thanks for the feedback.

>>uint16 hole_offset:15, /* number of bytes in "hole" */
> Typo in description of hole_offset
Fixed. That's "before hole".

>> 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.
Fixed.

>>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?
You are missing nothing. I just introduced this structure for a matter of readability to show the two-byte difference between non-compressed and compressed header information. It is true that doing it my way makes the structures duplicated, so let's simply add the compression-related information as an extra structure added after XLogRecordBlockImageHeader if the block is compressed. I hope this addresses your concerns.

>> /*
>> * 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)
> {
Yes, this comment should not be here. I replaced it with the comment in HEAD.

>>+ *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.
Aah, true. This was originally present in the header of PGLZ that has been removed to make it available for frontends.

Updated patches are attached.
Regards,
--
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.