The following review has been posted through the commitfest application:
make installcheck-world: tested, passed
Implements feature: tested, failed
Spec compliant: not tested
Documentation: not tested
Hi!
We've looked through the code and everything looks good except few minor things:
1). Using dedicated bg worker seems not optimal, it introduces a lot of
redundant code for little single action.
We'd join initial proposal of Andres to implement it as an extra fuction of
checkpointer.
2). In our view, it is better shift #define PREALLOCSEGDIR outside the function
body.
3). It is better to have at least small comments on functions
GetNumPreallocatedWalSegs, SetNumPreallocatedWalSegs,
We've also tested performance difference between master branch and this patch
and noticed no significant difference in performance.
We used pgbench with some sort of "standard" settings:
$ pgbench -c50 -s50 -T200 -P1 -r postgres
...and with...
$ pgbench -c100 -s50 -T200 -P1 -r postgres
When looking at every second output of pgbench we saw regular spikes of latency
(aroud 5-10 times increase) and this pattern was similar with and without
patch. Overall average latency stat for 200 sec of pgbench also looks pretty
much the same with and without patch. Could you provide your testing setup to
see the effect, please.
The check-world was successfull.
Overall patch looks good, but in our view it's better to have experimental
support of the performance improvements to be commited.
---
Best regards,
Maxim Orlov, Pavel Borisov.
The new status of this patch is: Waiting on Author
