Re: A bug in mapping attributes in ATExecAttachPartition()

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.

>> -            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