From: | Rahila Syed <rahilasyed(dot)90(at)gmail(dot)com> |
---|---|
To: | pgsql-hackers(at)postgresql(dot)org |
Subject: | Re: [REVIEW] Re: Compression of full-page-writes |
Date: | 2015-01-06 15:51:03 |
Message-ID: | 1420559463860-5833025.post@n5.nabble.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
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.
From | Date | Subject | |
---|---|---|---|
Next Message | Alvaro Herrera | 2015-01-06 16:32:29 | Re: Re: Patch to add functionality to specify ORDER BY in CREATE FUNCTION for SRFs |
Previous Message | Jehan-Guillaume de Rorthais | 2015-01-06 15:42:00 | Re: [RFC] Incremental backup v3: incremental PoC |