On 13 November 2017 at 12:55, Alvaro Herrera <alvhe...@alvh.no-ip.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
