On 2017/08/02 10:27, Robert Haas wrote: > 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.
I too dislike the shape of attachRel. How about we rename attachRel to attachrel? So, attachrel_children, attachrel_constr, etc. It's still long though... :) >>> - 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(); > > ? Hmm, The latter is better. I guess I just get confused with long &&, ||, () chains. If you're fine with the s/attachRel/attachrel/g suggestion above, I will update the patch along with reverting to if (a && b) c(). Thanks, Amit -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers