Обсуждение: [HACKERS] Redundant check of em_is_child

In match_eclasses_to_foreign_key_col(), there is this:

            if (em->em_is_child)
                continue;       /* ignore children here */

ISTM, it might as well be:

            Assert(!em->em_is_child);    /* no children yet */

That's because, I think it's still too early in query_planner() to be
expecting any child EC members.

Thanks,
Amit

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Fri, May 19, 2017 at 3:46 AM, Amit Langote
<Langote_Amit_f8@lab.ntt.co.jp> wrote:
> In match_eclasses_to_foreign_key_col(), there is this:
>
>             if (em->em_is_child)
>                 continue;       /* ignore children here */
>
> ISTM, it might as well be:
>
>             Assert(!em->em_is_child);    /* no children yet */
>
> That's because, I think it's still too early in query_planner() to be
> expecting any child EC members.

I'm not sure there's really any benefit to this change.  In the
future, somebody might want to use the function from someplace later
on in the planner.  If the logic as-written would work correctly in
that case now, I can't see why we should turn it into an assertion
failure instead.  This isn't really costing us anything, is it?

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 2017/06/23 0:00, Robert Haas wrote:
> On Fri, May 19, 2017 at 3:46 AM, Amit Langote
> <Langote_Amit_f8@lab.ntt.co.jp> wrote:
>> In match_eclasses_to_foreign_key_col(), there is this:
>>
>>             if (em->em_is_child)
>>                 continue;       /* ignore children here */
>>
>> ISTM, it might as well be:
>>
>>             Assert(!em->em_is_child);    /* no children yet */
>>
>> That's because, I think it's still too early in query_planner() to be
>> expecting any child EC members.
> 
> I'm not sure there's really any benefit to this change.  In the
> future, somebody might want to use the function from someplace later
> on in the planner.  If the logic as-written would work correctly in
> that case now, I can't see why we should turn it into an assertion
> failure instead.  This isn't really costing us anything, is it?

Not much perhaps.  Just thought it might be an oversight (and potentially
a source of confusion today to someone trying to understand this code),
but no harm in leaving it the way it is, as you say, so that someone in
the future doesn't have to worry about checking here.

Sorry for the noise.

Thanks,
Amit