On 4 December 2017 at 15:35, Konstantin Knizhnik <k.knizh...@postgrespro.ru> wrote: > On 30.11.2017 05:02, Michael Paquier wrote: >> >> On Wed, Sep 27, 2017 at 4:07 PM, Simon Riggs <si...@2ndquadrant.com> >> wrote: >>> >>> On 15 September 2017 at 16:34, Konstantin Knizhnik >>> <k.knizh...@postgrespro.ru> wrote: >>> >>>> Attached please find yet another version of the patch. >>> >>> Thanks. I'm reviewing it. >> >> Two months later, this patch is still waiting for a review (you are >> listed as well as a reviewer of this patch). The documentation of the >> patch has conflicts, please provide a rebased version. I am moving >> this patch to next CF with waiting on author as status. > > Attached please find new patch with resolved documentation conflict.
I’ve not posted a review because it was my hope that I could fix up the problems with this clever and useful patch and just commit it. I spent 2 days trying to do that but some problems remain and I’ve run out of time. Patch 3 has got some additional features that on balance I don’t think we need. Patch 1 didn’t actually work which was confusing when I tried to go back to that. Patch 3 Issues * Auto tuning added 2 extra items to Stats for each relation. That is too high a price to pay, so we should remove that. If we can’t do autotuning without that, please remove it. * Patch needs a proper test case added. We shouldn’t just have a DEBUG1 statement in there for testing - very ugly. Please rewrite tests using functions that show how many changes have occurred during the current statement/transaction. * Parameter coding was specific to that section of code. We need a capability to parse that parameter from other locations, just as we do with all other reloptions. It looks like it was coded that way because of difficulties with code placement. Please solve this with generic code, not just code that solves only the current issue. I’d personally be happier without any option at all, but if y’all want it, then please add the code in the right place. * The new coding for heap_update() uses the phrase “warm”, which is already taken by another patch. Please avoid confusing things by re-using the same term for different things. The use of these technical terms like projection and surjective doesn’t seem to add anything useful to the patch * Rename parameter to recheck_on_update * Remove random whitespace changes in the patch So at this stage the concept is good and works, but the patch is only just at the prototype stage and nowhere near committable. I hope you can correct these issues and if you do I will be happy to review and perhaps commit. Thanks -- Simon Riggs http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
