Re: WIP: Covering + unique indexes. - Mailing list pgsql-hackers

30.03.2017 19:49, Robert Haas:
> On Thu, Mar 30, 2017 at 11:26 AM, Teodor Sigaev <teodor@sigaev.ru> wrote:
>> I had a look on patch and played with it, seems, it looks fine. I splitted
>> it to two patches: core changes (+bloom index fix) and btree itself. All
>> docs are left in first patch - I'm too lazy to rewrite documentation which
>> is changed in second patch.
>> Any objection from reviewers to push both patches?
> Has this really had enough review and testing?  The last time it was
> pushed, it didn't go too well.  And laziness is not a very good excuse
> for not dividing up patches properly.

Well,
I don't know how can we estimate the quality of the review or testing.
The patch was reviewed by many people.
Here are those who marked themselves as reviewers on this and previous 
committfests: Stephen Frost (sfrost), Andrew Dunstan (adunstan), 
Aleksander Alekseev (a.alekseev), Amit Kapila (amitkapila), Andrey 
Borodin (x4m), Peter Geoghegan (pgeoghegan), David Rowley (davidrowley).

For me it looks serious enough. These people, as well as many others, 
shared their thoughts on this topic and pointed out various mistakes.
I fixed all the issues as soon as I could. And I'm not going to 
disappear when it will be committed. Personally, I always thought that 
we have Alpha and Beta releases for integration testing.

Speaking of the feature itself, it is included into our fork of 
PostgreSQL 9.6 since it was released.
And as far as I know, there were no complaints from users. It makes me 
believe that there are no critical bugs there.
While there may be conflicts with some other features of v10.0.

> It seems highly surprising to me that CheckIndexCompatible() only gets
> a one line change in this patch.  That seems unlikely to be correct.
What makes you think so? CheckIndexCompatible() only cares about 
possible opclasses' changes.
For covering indexes opclasses are only applicable to indnkeyatts. And 
that is exactly what was changed in this patch.
Do you think it needs some other changes?

> Has anybody done some testing of this patch with the WAL consistency
> checker?  Like, create some tables with indexes that have INCLUDE
> columns, set up a standby, enable consistency checking, pound the
> master, and see if the standby bails?
Good point. I missed this feature, I wish someone mentioned this issue a 
bit earlier.
And as Alexander's test shows there is some problem with my patch, indeed.
I'll fix it and send updated patch.

> Has anybody tested this patch with amcheck?  Does it break amcheck?
Yes, it breaks amcheck. Amcheck should be patched in order to work with 
covering indexes.
We've discussed it with Peter before and I even wrote small patch.
I'll attach it in the following message.

> A few minor comments:
>
> -    foreach(lc, constraint->keys)
> +    else foreach(lc, constraint->keys)
>
> That doesn't look like a reasonable way of formatting the code.
>
> +    /* Here is some code duplication. But we do need it. */
>
> That is not a very informative comment.
>
> +                        * NOTE It is not crutial for reliability in present,
>
> Spelling, punctuation.
>

Will be fixed as well.

-- 
Anastasia Lubennikova
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company