On Thu, Feb 12, 2015 at 8:08 PM, Syed, Rahila <
Rahila.Syed@nttdata.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@mail.gmail.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
