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

Hello,

Thank you for reviewing and testing the patch.

>+ /* leave if data cannot be compressed */
>+ if (compressed_len == 0)
>+ return false;
>This should be < 0, pglz_compress returns -1 when compression fails.
>
>+ if (pglz_decompress(block_image, bkpb->bkp_len, record->uncompressBuf,
>+ bkpb->bkp_uncompress_len) == 0)
>Similarly, this should be < 0.

These have been corrected in the attached.

>Regarding the sanity checks that have been added recently. I think that they are useful but I am suspecting as well that only a check on the record CRC is done because that's reliable enough and not doing those checks accelerates a bit replay. So I am thinking that we should simply replace >them by assertions.
Removing the checks makes sense as CRC ensures correctness . Moreover ,as error message for invalid length of record is present in the code , messages for invalid block length can be redundant.
Checks have been replaced by assertions in the attached patch.

Following if condition in XLogCompressBackupBlock has been modified as follows

Previous
/*
+ * We recheck the actual size even if pglz_compress() reports success and
+ * see if at least 2 bytes of length have been saved, as this corresponds
+ * to the additional amount of data stored in WAL record for a compressed
+ * block via raw_length when block contains hole..
+ */
+ *len = (uint16) compressed_len;
+ if (*len >= orig_len - SizeOfXLogRecordBlockImageCompressionInfo)
+ return false;
+ return true;

Current
if ((hole_length != 0) &&
+ (*len >= orig_len - SizeOfXLogRecordBlockImageCompressionInfo))
+ return false;
+return true

This is because the extra information raw_length is included only if compressed block has hole in it.

>Once we get those small issues fixes, I think that it is with having a committer look at this patch, presumably Fujii-san
Agree. I will mark this patch as ready for committer

Thank you,
Rahila Syed

______________________________________________________________________
Disclaimer: This email and any attachments are sent in strictest confidence
for the sole use of the addressee and may contain legally privileged,
confidential, and proprietary data. If you are not the intended recipient,
please advise the sender by replying promptly to this email and then delete
and destroy this email and any attachments without any further use, copying
or forwarding.