Re: [HACKERS] Surjective functional indexes

Thank you for review.

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

Right now size of this structure is 168 bytes. Adding two extra fields
increase it to 184 bytes - 9%. Is it really so critical?
What is the typical/maximal number of relation in a database?
I can not believe that there can be more than thousand non-temporary
relations in any database.
Certainly application can generate arbitrary number of temporary tables.
But millions of such tables cause catalog bloating and will have awful
impact on performance in any case.
And for any reasonable number of relations (< million), extra memory
used by this statistic is negligible.
Also I think that these two fields can be useful not only for projection
indexes.
It can be also used by DBA and optimizer because them more precisely
explain relation update pattern.

I really love you idea with autotune and it will be pity to reject it
just because of extra 16 bytes per relation.
> * 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.
Sorry, I do not completely understand what you mean by "parameter coding".
Do you mean IsProjectionFunctionalIndex function and filling relation
options in it?
It is the only reported issue which I do not understand how to fix.

> * 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.

Thank you once again for your efforts in improving this patch.
I will fix all reported issues.

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