Re: [HACKERS] Surjective functional indexes

On 13.12.2017 14:29, Simon Riggs wrote:
> On 4 December 2017 at 15:35, Konstantin Knizhnik
> <k.knizhnik@postgrespro.ru> wrote:
>> On 30.11.2017 05:02, Michael Paquier wrote:
>>> On Wed, Sep 27, 2017 at 4:07 PM, Simon Riggs <simon@2ndquadrant.com>
>>> wrote:
>>>> On 15 September 2017 at 16:34, Konstantin Knizhnik
>>>> <k.knizhnik@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
>

Attached please find new patch which fix all the reported issues except 
disabling autotune.
If you still thing that additional 16 bytes per relation in statistic is 
too high overhead, then I will also remove autotune.


-- 
Konstantin Knizhnik
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company