Thread: [HACKERS] Unused member root in foreign_glob_cxt
Hi, 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? 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. -- Best Wishes, Ashutosh Bapat EnterpriseDB Corporation The Postgres Database Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Attachment
Ashutosh Bapat <ashutosh.bapat@enterprisedb.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.
> 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.
regards, tom lane
On Thu, Jan 12, 2017 at 6:39 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Ashutosh Bapat <ashutosh.bapat@enterprisedb.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
Ashutosh Bapat <ashutosh.bapat@enterprisedb.com> writes:
> On Thu, Jan 12, 2017 at 6:39 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> 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.
[ shrug... ] The general trend in the planner is that things get more
complicated because we start accounting for new effects. I don't have
any faith in the claim that because is_foreign_expr hasn't needed
PlannerInfo yet, it never will, because what it's required to do is
inherently spongy and complicated. It wouldn't particularly surprise
me if someone wanted to start putting cost considerations in there,
for example.
In short, I think that removing this argument is just make-work that
we're very likely to have to undo at some point; and the further you
extend the changes, the larger a hazard for back-patching it will be.
If you can convince some other committer that this is a good idea, fine,
but I won't commit it.
regards, tom lane