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

On Thu, Feb 12, 2015 at 8:08 PM, Syed, Rahila <Rahila(dot)Syed(at)nttdata(dot)com>
wrote:
>
>
>
> Thank you for comments. Please find attached the updated patch.
>
>
>
> >This patch fails to compile:
> >xlogreader.c:1049:46: error: extraneous ')' after condition, expected a
statement
> > blk->with_hole &&
blk->hole_offset <= 0))
>
> This has been rectified.
>
>
>
> >Note as well that at least clang does not like much how the sanity check
with with_hole are done. You should place parentheses around the '&&'
expressions. Also, I would rather define with_hole == 0 or with_hole == 1
explicitly int those checks
>
> The expressions are modified accordingly.
>
>
>
> >There is a typo:
>
> >s/true,see/true, see/
>
> >[nitpicky]Be as well aware of the 80-character limit per line that is
usually normally by comment blocks.[/]
>
>
>
> Have corrected the typos and changed the comments as mentioned. Also ,
realigned certain lines to meet the 80-char limit.

Thanks for the updated 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.

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.

I have as well re-run my small test case, with the following results
(scripts and results attached)
=# select test, user_diff,system_diff, pg_size_pretty(pre_update -
pre_insert),
pg_size_pretty(post_update - pre_update) from results;
test | user_diff | system_diff | pg_size_pretty | pg_size_pretty
---------+-----------+-------------+----------------+----------------
FPW on | 46.134564 | 0.823306 | 429 MB | 566 MB
FPW on | 16.307575 | 0.798591 | 171 MB | 229 MB
FPW on | 8.325136 | 0.848390 | 86 MB | 116 MB
FPW off | 29.992383 | 1.100458 | 440 MB | 746 MB
FPW off | 12.237578 | 1.027076 | 171 MB | 293 MB
FPW off | 6.814926 | 0.931624 | 86 MB | 148 MB
HEAD | 26.590816 | 1.159255 | 440 MB | 746 MB
HEAD | 11.620359 | 0.990851 | 171 MB | 293 MB
HEAD | 6.300401 | 0.904311 | 86 MB | 148 MB
(9 rows)
The level of compression reached is the same as previous mark, 566MB for
the case of fillfactor=50 (
CAB7nPqSc97o-UE5paxfMUKWcxE_JioyxO1M4A0pMnmYqAnec2g(at)mail(dot)gmail(dot)com) with a
similar CPU usage.

Once we get those small issues fixes, I think that it is with having a
committer look at this patch, presumably Fujii-san.
Regards,
--
Michael