>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 -
>-       }
>+               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: 
Sent from the PostgreSQL - hackers mailing list archive at Nabble.com.

Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:

Reply via email to