Re: Unused member root in foreign_glob_cxt

On Thu, Jan 12, 2017 at 6:39 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Ashutosh Bapat <ashutosh(dot)bapat(at)enterprisedb(dot)com> writes:
>> The member root in foreign_glob_cxt isn't used anywhere by
>> postgres_fdw code. Without that member the code compiles and
>> regression passes. The member was added by d0d75c40. I looked at that
>> commit briefly but did not find any code using it there. So, possibly
>> it's unused since it was introduced. Should we drop that member?
>
> I think you'd just end up putting it back at some point. It's the only
> means that foreign_expr_walker() has for getting at the root pointer,
> and nearly all planner code needs that. Seems to me it's just chance
> that foreign_expr_walker() doesn't need it right now.

I agree with you that most of the planner code needs that but not
every planner function accepts root as an argument. The commit was 4
years before and since then we have added support for Analyze, WHERE,
join, aggregate, DML pushdown. None of them required it and it's
likely that none of the future work would require it as we have
covered almost all kinds of expressions in there.

>
>> PFA the patch to remove that member. If we decide to drop that member,
>> we can drop root argument to is_foreign_expr() and clean up some more
>> code. I volunteer to do that, if we agree.
>
> That would *really* be doubling down on the assumption that
> is_foreign_expr() will never ever need access to the root.
>

I think the cleanup won't expand beyond is_foreign_expr() itself.
Almost all of its callers, use root for some other reason. We will add
root to is_foreign_expr() when we need it, but that time we will know
"why" it's there.

--
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company