On Thu, Dec 13, 2012 at 7:06 AM, Hari Babu <haribabu.kommi@huawei.com> wrote:
> Please find the review of the patch.
Thanks for detailed review!
> Basic stuff:
> ------------
> - Patch applies with offsets.
> - Compiles cleanly with no warnings
> - Regression Test pass.
>
> Code Review:
> -------------
> 1. Better to set the hint bits for the tuples in a page, if the page
> is already dirty.
This is true today but likely less true if/when page checksums come
out. Also it complicates the code a little bit.
> Default tables select : 64980.736149 64550.118693
> Unlogged tables select: 64874.974334 64550.118693
So it looks like the extra tests visibility routines are causing 0.7%
performance hit.
> Multiple transaction bulk inserts: Select performance (refer script -1 & 2
> which attached)
> sequential scan: 6.492680 6.382014
> Index scan: 1.386851 1.36234
>
> Single transaction bulk inserts: Select performance (refer script - 3 & 4
> which attached)
> sequential scan: 6.49319 6.3800147
> Index scan: 1.384121 1.3615277
The performance hit is higher here. Almost 2%. This is troubling.
> Long transaction open then Vacuum & select performance in milli seconds.
> (refer reports output)
> Testcase - 3:
> Single Vacuum Perf : 128.302 ms 181.292 ms
> Single select perf : 214.107 ms 177.057 ms
> Total : 342.409 ms 358.349 ms
>
> I was not able to find the reason why in some of cases results are low so
> please use the attached scripts to validate the same.
I need to validate the vacuum results. It's possible that this is
solvable by tweaking xmin check inside vacuum. Assuming that's fixed,
the question stands: do the results justify the change? I'd argue
'maybe' -- I'd like to see the bulk insert performance hit reduced if
possible. Let's see what we can do in the short term (and, if no
improvement can be found, I think this patch should be marked
'returned with feedback').
merlin
