Re: postgres_fdw join pushdown (was Re: Custom/Foreign-Join-APIs) - Mailing list pgsql-hackers
| From | Ashutosh Bapat |
|---|---|
| Subject | Re: postgres_fdw join pushdown (was Re: Custom/Foreign-Join-APIs) |
| Date | |
| Msg-id | CAFjFpRchnjEK9PVnady92eP2UCQd8hLPZ1qb6jFJQMTVc5_cmA@mail.gmail.com Whole thread Raw |
| In response to | Re: postgres_fdw join pushdown (was Re: Custom/Foreign-Join-APIs) (Robert Haas <robertmhaas@gmail.com>) |
| Responses |
Re: postgres_fdw join pushdown (was Re: Custom/Foreign-Join-APIs)
(Robert Haas <robertmhaas@gmail.com>)
|
| List | pgsql-hackers |
On Thu, Jan 21, 2016 at 3:03 AM, Robert Haas <robertmhaas@gmail.com> wrote:
deparseVar() is called for any Var node that's encountered. deparseJoinVar is called to deparse a Var from join relation, which is supposed to output INNER or OUTER var's alias as used in INNER or OUTER subqueries. It does not output the real column names. deparseColumnRef() however is the same old thing; it deparses column of given base relation. They are not similar things.
-- On Mon, Jan 18, 2016 at 6:47 AM, Ashutosh Bapat
<ashutosh.bapat@enterprisedb.com> wrote:
> 2. pg_fdw_join_v2.patch: postgres_fdw changes for supporting join pushdown
The very first hunk of this patch contains annoying whitespace
changes. Even if the result is what pgindent is going to do anyway,
such changes should be stripped out of patches for ease of review. In
this case, though, I'm pretty sure it isn't what pgindent is going to
do, so it's just useless churn. Please remove all such changes from
the patch.
Done.
find_var_pos() looks like it should just be inlined into its only caller.
Node *node = (Node *) var;
TargetListEntry *tle = tlist_member(node, context->outerlist);
if (tle)
{
side = OUTER_ALIAS;
pos = tle->resno;
}
else
{
side = INNER_ALIAS;
tle = tlist_member(node, context->innertlist);
pos = tle->resno;
}
Why are we calling the return value "pos" instead of "resno" as we
typically do elsewhere?
I have rewritten deparseJoinVar as
/*
* Deparse given Var required for a joinrel into buf.
*/
static void
deparseJoinVar(Var *node, deparse_expr_cxt *context)
{
char *side;
TargetEntry *tle;
/* Lookup outer side */
tle = tlist_member((Node *)node, context->outertlist);
if (tle)
side = OUTER_ALIAS;
else
{
/* Not found on outer side; lookup inner */
side = INNER_ALIAS;
tle = tlist_member((Node *)node, context->innertlist);
}
/* The input var should be either on left or right side */
Assert(tle && side);
appendStringInfo(context->buf, "%s.%s%d", side, COL_ALIAS_PREFIX, tle->resno);
}
/*
* Deparse given Var required for a joinrel into buf.
*/
static void
deparseJoinVar(Var *node, deparse_expr_cxt *context)
{
char *side;
TargetEntry *tle;
/* Lookup outer side */
tle = tlist_member((Node *)node, context->outertlist);
if (tle)
side = OUTER_ALIAS;
else
{
/* Not found on outer side; lookup inner */
side = INNER_ALIAS;
tle = tlist_member((Node *)node, context->innertlist);
}
/* The input var should be either on left or right side */
Assert(tle && side);
appendStringInfo(context->buf, "%s.%s%d", side, COL_ALIAS_PREFIX, tle->resno);
}
get_jointype_name() would probably be better written as a switch.
Done.
On
the flip side, deparseColumnRef() would have a lot less code churn if
it *weren't* written as a switch.
Done.
What this patch does to the naming and calling conventions in
deparse.c does not good. Previously, we had deparseTargetList().
Now, we sometimes use that and sometimes deparseAttrsUsed() for almost
the same thing.
Previously deparseTargetList deparsed the SELECT or RETURNING clause by including list of name of attributes provided by attrs_used. That's now done by deparseAttrsUsed(). In current path deparseTargetList() deparses the targetlist i.e. list of TargetEntry nodes (right now only Vars). Although these two functions return comma separated string of column names, their inputs are different. deparseAttrsUsed() can never be called for more than one base relation. deparseTargetList() on the other hand can deparse a targetlist with Var nodes from multiple relations. We need those two functionalities separately. We might convert attrs_used into a list of TargetEntry nodes using build_tlist_to_deparse() and use deparseTargetList everywhere. A side effect of that would be separating retrieved_attrs collection from deparsing code. I didn't do that change in this version to avoid large code changes. But I am fine doing that, if that makes code readable.
If we have to keep old deparseTargetList as is (and don't rename it as deparseAttrsUsed), we can rename the new deparseTargetList as something different may be deparseSelectList. I am fine with that too. But we need the later functionality, whatever its name be.
If we have to keep old deparseTargetList as is (and don't rename it as deparseAttrsUsed), we can rename the new deparseTargetList as something different may be deparseSelectList. I am fine with that too. But we need the later functionality, whatever its name be.
Previously, we had deparseColumnRef(); now we have
both that and deparseJoinVar() doing very similar things. But in each
case, the function names are not parallel and the calling conventions
are totally different. Like here:
+ if (context->foreignrel->reloptkind == RELOPT_JOINREL)
+ deparseJoinVar(node, context);
+ else
+ deparseColumnRef(buf, node->varno,
node->varattno, context->root);
We pass the buf separately to deparseColumnRef(), but for some reason
not to deparseJoinVar().I wonder if these functions need to be two
separate things or if the work done by deparseJoinVar() should
actually be part of deparseColumnRef(). But even if it needs to be
separate, I wonder why we can't arrange things so that they get the
same arguments, more or less.
deparseVar() is called for any Var node that's encountered. deparseJoinVar is called to deparse a Var from join relation, which is supposed to output INNER or OUTER var's alias as used in INNER or OUTER subqueries. It does not output the real column names. deparseColumnRef() however is the same old thing; it deparses column of given base relation. They are not similar things.
deparseJoinVar gets its buf from context, which we do not pass to deparseColumnRef(). Not all callers of deparseColumnRef have a deparse_expr_cxt with them. Also, outertlist and innertlist required by deparseJoinVar are passed through deparse_expr_cxt. It doesn't look worth to create a context just for the sake of making function definitions look similar. So, we need to have these two functions separate,
Generally, I think this patch is on the right track, but I think
there's a good bit of work to be done to make it clearer and more
understandable.
I agree that the code is complex for a reader. One of the reasons is recursive nature of deparsing. I will try to make it more cleaner and easier to understand. Would adding a call tree for deparsing routines help here? Or improving function prologues of even the existing functions?
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company
pgsql-hackers by date: