Re: [HACKERS] A bug in mapping attributes in ATExecAttachPartition()

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