On Thu, Mar 30, 2017 at 5:22 PM, Anastasia Lubennikova
<a.lubennikova@postgrespro.ru> wrote:
> 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).
Sure, but the amount of in-depth review seems to have been limited.
Just because somebody put their name down in the CommitFest
application doesn't mean that they did a detailed review of all the
code.
>> 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?
Probably. I mean, for an INCLUDE column, it wouldn't matter if a
collation or opclass change happened, but if the base data type had
changed, you'd still need to rebuild the index. So presumably
CheckIndexCompatible() ought to be comparing some things, but not
everything, for INCLUDE columns.
>> 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.
Great.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
