On Tue, Aug 1, 2017 at 9:23 PM, Amit Langote <langote_amit...@lab.ntt.co.jp> wrote: > Since ATExecAttachPartition() deals with the possibility that the table > being attached itself might be partitioned, someone reading the code might > find it helpful to get some clue about whose partitions/children a > particular piece of code is dealing with - AT's target table's (rel's) or > those of the table being attached (attachRel's)? IMHO, attachRel_children > makes it abundantly clear that it is in fact the partitions of the table > being attached that are being manipulated.
True, but it's also long and oddly capitalized and punctuated. Seems like a judgement call which way is better, but I'm allergic to fooBar_baz style names. >> - if (part_rel != attachRel && >> - part_rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE) >> + if (part_rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE) >> { >> - heap_close(part_rel, NoLock); >> + if (part_rel != attachRel) >> + heap_close(part_rel, NoLock); >> >> This works out to a cosmetic change, I guess, but it makes it worse... > > Not sure what you mean by "makes it worse". The comment above says that > we should skip partitioned tables from being scheduled for heap scan. The > new code still does that. We should close part_rel before continuing to > consider the next partition, but mustn't do that if part_rel is really > attachRel. The new code does that too. Stylistically worse? Yeah. I mean, do you write: if (a) if (b) c(); rather than if (a && b) c(); ? -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers