Re: postgres_fdw join pushdown (was Re: Custom/Foreign-Join-APIs)

On Thu, Jan 21, 2016 at 3:03 AM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:

> On Mon, Jan 18, 2016 at 6:47 AM, Ashutosh Bapat
> <ashutosh(dot)bapat(at)enterprisedb(dot)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);
}

> 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.

> 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