On Wed, Jan 7, 2015 at 12:51 AM, Rahila Syed <rahilasyed...@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 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
20150107_fpw_compression_v14.tar.gz
Description: GNU Zip compressed data
-- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers