Horton

Re: WIP: Covering + unique indexes.

Robert Haas <robertmhaas@gmail.com>

View thread View on postgr.es
From: Robert Haas <robertmhaas@gmail.com>
To: Anastasia Lubennikova <a.lubennikova@postgrespro.ru>
Cc: Teodor Sigaev <teodor@sigaev.ru>, "pgsql-hackers@postgresql.org" <pgsql-hackers@postgresql.org>, Anastasia Lubennikova <lubennikovaav@gmail.com>, Peter Geoghegan <pg@heroku.com>, David Steele <david@pgmasters.net>, Peter Eisentraut <peter.eisentraut@2ndquadrant.com>, Aleksander Alekseev <a.alekseev@postgrespro.ru>
Date: 2017-03-31T10:55:33Z
Lists: pgsql-hackers 
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

Commits

  1. Adjust INCLUDE index truncation comments and code.

  2. Add commentary explaining why MaxIndexTuplesPerPage calculation is safe.

  3. Indexes with INCLUDE columns and their support in B-tree

  4. Add amcheck verification of heap relations belonging to btree indexes.

  5. Doc: move info for btree opclass implementors into main documentation.

  6. Doc: mention that you can't PREPARE TRANSACTION after NOTIFY.

  7. Remove dedicated B-tree root-split record types.

  8. Restructure index access method API to hide most of it at the C level.

  9. Split _bt_insertonpg to two functions.

  10. Major overhaul of btree index code. Eliminate special BTP_CHAIN logic for