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

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.

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?

get_jointype_name() would probably be better written as a switch. On
the flip side, deparseColumnRef() would have a lot less code churn if
it *weren't* written as a switch.

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

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.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company