Re: Redundant check of em_is_child

On 2017/06/23 0:00, Robert Haas wrote:
> On Fri, May 19, 2017 at 3:46 AM, Amit Langote
> <Langote_Amit_f8(at)lab(dot)ntt(dot)co(dot)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