From: | Robert Haas <robertmhaas(at)gmail(dot)com> |
---|---|
To: | Amit Langote <Langote_Amit_f8(at)lab(dot)ntt(dot)co(dot)jp> |
Cc: | Ashutosh Bapat <ashutosh(dot)bapat(at)enterprisedb(dot)com>, pgsql-hackers <pgsql-hackers(at)postgresql(dot)org>, Amit Langote <amitlangote09(at)gmail(dot)com> |
Subject: | Re: A bug in mapping attributes in ATExecAttachPartition() |
Date: | 2017-08-02 01:27:43 |
Message-ID: | CA+TgmobOLfvU_98QmmTbepOF9X4SWBYN1Ane85_9TAiK8rkdFA@mail.gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
On Tue, Aug 1, 2017 at 9:23 PM, Amit Langote
<Langote_Amit_f8(at)lab(dot)ntt(dot)co(dot)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
From | Date | Subject | |
---|---|---|---|
Next Message | Amit Langote | 2017-08-02 01:44:58 | Re: A bug in mapping attributes in ATExecAttachPartition() |
Previous Message | Masahiko Sawada | 2017-08-02 01:23:24 | Re: BUG #14758: Segfault with logical replication on a function index |