Re: [REVIEW] Re: Compression of full-page-writes

Hello,

Thank you for review.

>1) I don't think it's a good idea to put the full page write compression
into struct XLogRecord.

Full page write compression information can be stored in varlena struct of
compressed blocks as done for toast data in pluggable compression support
patch. If I understand correctly, it can be done similar to the manner in
which compressed Datum is modified to contain information about compression
algorithm in pluggable compression support patch.

>2) You've essentially removed a lot of checks about the validity of bkp
blocks in xlogreader. I don't think that's acceptable

To ensure this, the raw size stored in first four byte of compressed datum
can be used to perform error checking for backup blocks
Currently, the error checking for size of backup blocks happens individually
for each block.
If backup blocks are compressed together , it can happen once for the entire
set of backup blocks in a WAL record. The total raw size of compressed
blocks can be checked against the total size stored in WAL record header.

>3) You have both FullPageWritesStr() and full_page_writes_str().

full_page_writes_str() is true/false version of FullPageWritesStr macro. It
is implemented for backward compatibility with pg_xlogdump

>4)I don't like FullPageWritesIsNeeded(). For one it, at least to me,
sounds grammatically wrong. More importantly when reading it I'm
thinking of it being about the LSN check. How about instead directly
checking whatever != FULL_PAGE_WRITES_OFF?

I will modify this.

>5) CompressBackupBlockPagesAlloc is declared static but not defined as
such.
>7) Unless I miss something CompressBackupBlock should be plural, right?
ATM it compresses all the blocks?
I will correct these.

>6)You call CompressBackupBlockPagesAlloc() from two places. Neither is
> IIRC within a critical section. So you imo should remove the outOfMem
> handling and revert to palloc() instead of using malloc directly.

Yes neither is in critical section. outOfMem handling is done in order to
proceed without compression of FPW in case sufficient memory is not
available for compression.

Thank you,
Rahila Syed

--
View this message in context: http://postgresql.1045698.n5.nabble.com/Compression-of-full-page-writes-tp5769039p5822391.html
Sent from the PostgreSQL - hackers mailing list archive at Nabble.com.