Re: [REVIEW] Re: Compression of full-page-writes - Mailing list pgsql-hackers

From Syed, Rahila
Subject Re: [REVIEW] Re: Compression of full-page-writes
Date
Msg-id C3C878A2070C994B9AE61077D46C3846589AC4E0@MAIL703.KDS.KEANE.COM
Whole thread Raw
In response to Re: [REVIEW] Re: Compression of full-page-writes  (Michael Paquier <michael.paquier@gmail.com>)
Responses Re: [REVIEW] Re: Compression of full-page-writes  (Michael Paquier <michael.paquier@gmail.com>)
List pgsql-hackers
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
storingraw_length of the compressed block.  
In order to achieve compression while accounting for these two additional bytes, we must ensure that compressed length
isless 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
tostore uncompressed page. 

Thank you,
Rahila Syed

-----Original Message-----
From: pgsql-hackers-owner@postgresql.org [mailto:pgsql-hackers-owner@postgresql.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.90@gmail.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
betweennon-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
XLogRecordBlockImageHeaderif 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.
Attachment

pgsql-hackers by date:

Previous
From: Francesco Canovai
Date:
Subject: Re: File based Incremental backup v9
Next
From: Michael Meskes
Date:
Subject: Re: Unlikely-to-happen crash in ecpg driver caused by NULL-pointer check not done