Re: [HACKERS] Proposal: Local indexes for partitioned table

On 13 November 2017 at 12:55, Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org> wrote:
> Somehow I managed to include an unrelated patch as attachment. Here's
> another attempt (on which I also lightly touched ddl.sgml with relevant
> changes).

Looks good. Some minor comments below.

0001- Simplify
Seems useful as separate step; agree with everything, no further comments
Why uint16? Why not just uint?

0003-export
I don't like Assert((heapRel != NULL) ^ OidIsValid(heapRelid));
Standard boolean logic is clearer
I couldn't see why this was a separate patch

0004-Allow
If we do ATTACH PARTITION, do we also need to do a separate ATTACH
INDEX step to allow an index to join the main index? Hopefully not.
The tests seem to indicate to me that it does work that way, just no
docs in altertable.sgml to describe that
typo "they all have a matching indexes" - no plural needed
typo "whether equivalent" - insert "an"
unsure what "regardless of whether this option was specified" means,
probably just remove

0005-Allow
RelationCacheInitializePhase3() asserts that rd_partkey is not NULL
for a partitoned table after RelationBuildPartitionKey() runs

You removed the test to check whether "create unique index" works, not
sure if that was intentional
There is no test for attach index to a unique index

--
Simon Riggs http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services