From: | Michael Paquier <michael(dot)paquier(at)gmail(dot)com> |
---|---|
To: | Rahila Syed <rahilasyed(dot)90(at)gmail(dot)com> |
Cc: | PostgreSQL mailing lists <pgsql-hackers(at)postgresql(dot)org> |
Subject: | Re: [REVIEW] Re: Compression of full-page-writes |
Date: | 2015-01-07 04:02:29 |
Message-ID: | CAB7nPqQkupnzfJAmT881YQJMsHtnkQCOr8_GyfOCzOjPxChbhw@mail.gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
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
Attachment | Content-Type | Size |
---|---|---|
20150107_fpw_compression_v14.tar.gz | application/x-gzip | 23.6 KB |
From | Date | Subject | |
---|---|---|---|
Next Message | Ashutosh Bapat | 2015-01-07 04:14:21 | Re: Patch to add functionality to specify ORDER BY in CREATE FUNCTION for SRFs |
Previous Message | Amit Kapila | 2015-01-07 03:36:08 | Re: pg_basebackup fails with long tablespace paths |