On 2017/08/02 10:27, Robert Haas wrote:
> On Tue, Aug 1, 2017 at 9:23 PM, Amit Langote
> <Langote_Amit_f8@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