Re: A bug in mapping attributes in ATExecAttachPartition()

Thanks for reviewing.

On 2017/08/02 2:54, Robert Haas wrote:
> On Mon, Jul 31, 2017 at 11:10 PM, Amit Langote
> <Langote_Amit_f8(at)lab(dot)ntt(dot)co(dot)jp> wrote:
>> OK, these cosmetic changes are now in attached patch 0001.
>
> Regarding 0001:
>
> - List *childrels;
> + List *attachRel_children;
>
> I sorta don't see why this is necessary, or better.

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.

> /* It's safe to skip the validation scan after all */
> if (skip_validate)
> + {
> + /* No need to scan the table after all. */
>
> The existing comment should be removed along with adding the new one, I think.

Oops, fixed.

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

Thanks,
Amit