Re: Push down more full joins in postgres_fdw - Mailing list pgsql-hackers

On 2016/10/22 17:19, Ashutosh Bapat wrote:
> Review for postgres-fdw-more-full-join-pushdown-v6 patch.
>
> The patch applies cleanly and regression is clean (make check in
> regress directory and that in postgres_fdw).

Thanks for the review!

> Here are some comments.
> 1. Because of the following code change, for a joinrel we might end up using
> attrs_used, which will be garbage for a join rel. The assumption that tlist can
> not be NIL for a join relation is wrong. Even for a join relation, tlist can be
> NULL.  Consider query 'explain verbose select count(*) from ft1, ft2' Since
> count(*) doesn't need any columns, the tlist is NIL. With the patch the server
> crashes for this query.
> -    if (foreignrel->reloptkind == RELOPT_JOINREL)
> +    /*
> +     * Note: tlist for a base relation might be non-NIL.  For example, if the
> +     * base relation is an operand of a foreign join performing a full outer
> +     * join and has non-NIL remote_conds, the base relation will be deparsed
> +     * as a subquery, so the tlist for the base relation could be non-NIL.
> +     */
> +    if (tlist != NIL)
>      {
> -        /* For a join relation use the input tlist */
>
> We can not decide whether to use deparseExplicitTargetList or
> deparse it from attrs_used based on the tlist. SELECT lists, whether it's
> topmost SELECT or a subquery (even on a base relation), for deparsing a
> JOIN query need to use deparseExplicitTargetList.

Will fix.

> 2. The code in deparseRangeTblRef() dealing with subqueries, is very similar to
> deparseSelectStmtForRel(). The only thing deparseRangeTblRef() does not require
> is the appending ORDER BY, which is determined by existence of pathkeys
> argument. Probably we should reuse deparseSelectStmtForRel(), instead of
> duplicating the code. This will also make the current changes to
> deparseSelectSql unnecessary.

Will do.

> 3. Why do we need following change? The elog() just few lines above says that
> we expect only Var nodes. Why then we use deparseExpr() instead of
> deparseVar()?
>          if (i > 0)
>              appendStringInfoString(buf, ", ");
> -        deparseVar(var, context);
> +        deparseExpr((Expr *) var, context);
>
>          *retrieved_attrs = lappend_int(*retrieved_attrs, i + 1);
>
> And I get the answer for that question, somewhere down in the patch
> +    /*
> +     * If the given expression is an output column of a subquery-in-FROM,
> +     * deparse the alias to the expression instead.
> +     */
> +    if (IsA(node, Var))
> +    {
> +        int            tabno;
> +        int            colno;
> +
> +        if (isSubqueryExpr(node, context->foreignrel, &tabno, &colno))
> +        {
> +            appendStringInfo(context->buf, "%s%d.%s%d",
> +                             SS_TAB_ALIAS_PREFIX, tabno,
> +                             SS_COL_ALIAS_PREFIX, colno);
> +            return;
> +        }
> +    }
> +
>
> Functionally, this code belongs to deparseVar() and not deparseExpr(). Like all
> other functions which handle Expr nodes, deparseExpr() is expected to be a
> dispatcher to the node specific function.

OK, will change that way.

> 4. This comment is good to have there, but is unrelated to this patch. May be a
> separate patch?
> +    /* Don't generate bad syntax if no columns */
>
> 5. Changed comment doesn't say anything different from the original comment. It
> may have been good to have it this way to start with, but changing it now
> doesn't bring anything new. You seem to have just merged prologue of the
> function with a comment in that function. I think, this change is unnecessary.
> - * The function constructs ... JOIN ... ON ... for join relation. For a base
> - * relation it just returns schema-qualified tablename, with the appropriate
> - * alias if so requested.
> + * For a join relation the clause of the following form is appended to buf:
> + * ((outer relation) <join type> (inner relation) ON (joinclauses))
> + * For a base relation the function just adds the schema-qualified tablename,
> + * with the appropriate alias if so requested.
> +    /* Don't generate bad syntax if no columns */
>
> 6. Change may be good but unrelated to this patch. May be material for a
> separate patch. There are few such changes in this function. While these
> changes may be good by themselves, they distract reviewer from the goal of the
> patch.
> -            appendStringInfo(buf, "(");
> +            appendStringInfoChar(buf, '(');

OK on #4, #5, and #6, Will remove all the changes.  I will leave those 
for separate patches.

> 7. I don't understand why you need to change this function so much. Take for
> example the following change.
> -        RelOptInfo *rel_o = fpinfo->outerrel;
> -        RelOptInfo *rel_i = fpinfo->innerrel;
> -        StringInfoData join_sql_o;
> -        StringInfoData join_sql_i;
> +        /* Begin the FROM clause entry */
> +        appendStringInfoChar(buf, '(');
>
>          /* Deparse outer relation */
> -        initStringInfo(&join_sql_o);
> -        deparseFromExprForRel(&join_sql_o, root, rel_o, true, params_list);
> +        deparseRangeTblRef(buf, root,
> +                           fpinfo->outerrel,
> +                           fpinfo->make_outerrel_subquery,
> +                           params_list);
>          /* Deparse outer relation */
> -        initStringInfo(&join_sql_o);
> -        deparseFromExprForRel(&join_sql_o, root, rel_o, true, params_list);
> +        deparseRangeTblRef(buf, root,
> +                           fpinfo->outerrel,
> +                           fpinfo->make_outerrel_subquery,
> +                           params_list);
>
> It removes few variables, instead directly accesses the members from fpinfo.
> Also, deparses the individual relations in the provided buffer directly, rather
> than in separate buffers in the original code. Instead of this, all you had to
> do was replace a call
> -        deparseFromExprForRel(&join_sql_o, root, rel_o, true, params_list);
> with
> +        deparseRangeTblRef(&join_sql_o, root, rel_o,
> fpinfo->make_outerrel_subquery, params_list)
> Similarly for the inner relation. Again, the changes you have done might have
> been good, if done in the original code, but doing those in this patch just
> creates distractions and increases the size of the patch.

I did that for efficiency because deparsing the relation directly into 
the given buffer would save cycles, but I agree that that is another 
issue.  Will remove that change, and leave that for another patch.

> 8. Why have you changed the name of variable here?
> -        if (use_alias)
> +        if (add_rel_alias)

Sorry, I forgot to remove this change from the patch.  Will fix.

> 9. In deparseRangeTblRef(), the targetlist for the subquery is obtained by
> calling build_tlist_to_deparse(), but appendSubselectAlias() constructs the
> column aliases based on foreignrel->reltarget->exprs. Those two are usually
> same, but there is no code which guarantees that. Instead, pass tlist to
> appendSubselectAlias() or simply pass the number of entries in that list
> (list_length(tlist), which is all it needs. OR use foreignrel->reltarget->exprs
> directly instead of calling build_tlist_to_deparse(). Similar problem exists
> with getSubselectAliasInfo().

Actually, the patch guarantees that since in that case (1) 
foreignrel->reltarget->exprs doesn't contain any PHVs (note that passing 
the following check in foreign_join_ok implies that 
foreignrel->reltarget->exprs of the input rels for a given foreign join 
doesn't contain any PHVs and (2) we have fpinfo->local_conds == NIL, so 
build_tlist_to_deparse() would produce a tlist equivalent to the 
foreignrel->reltarget->exprs.  But as you proposed, to make the code 
easier to understand, I will change to use foreignrel->reltarget->exprs 
directly.
    /*     * deparseExplicitTargetList() isn't smart enough to handle 
anything other     * than a Var.  In particular, if there's some PlaceHolderVar that 
would     * need to be evaluated within this join tree (because there's an upper     * reference to a quantity that may
goto NULL as a result of an outer     * join), then we can't try to push the join down because we'll 
 
fail when     * we get to deparseExplicitTargetList().  However, a 
PlaceHolderVar that     * needs to be evaluated *at the top* of this join tree is OK, 
because we     * can do that locally after fetching the results from the remote side.     */    relids =
joinrel->relids;   foreach(lc, root->placeholder_list)    {        PlaceHolderInfo *phinfo = lfirst(lc);
 
        if (bms_is_subset(phinfo->ph_eval_at, relids) &&            bms_nonempty_difference(relids,
phinfo->ph_eval_at))           return false;    }
 

> 10. Deparsing a query in the form of a subquery makes it hard-to-read. The
> original code aimed at avoiding a subquery. Also, this change has created many
> expected output changes, which again seem unnecessary. In fact, having the
> pushable join clauses of an inner join in ON clause, which is closer to JOIN
> clause, is better than having them farther in the WHERE clause.
> -    /*
> -     * For an inner join, all restrictions can be treated alike. Treating the
> -     * pushed down conditions as join conditions allows a top level full outer
> -     * join to be deparsed without requiring subqueries.
> -     */
> -    if (jointype == JOIN_INNER)
> -    {
> -        Assert(!fpinfo->joinclauses);
> -        fpinfo->joinclauses = fpinfo->remote_conds;
> -        fpinfo->remote_conds = NIL;
> -    }

I added this change for another patch that I proposed for extending the 
DML pushdown in 9.6 so that it can perform an update/delete on a join 
remotely.  To create a remote query for that, I think we need to pull up 
inner join conditions mentioning the target relation in the JOIN/ON 
clauses into the WHERE clause.  But I have to admit that's unrelated to 
this patch, so I will leave that as-is.

> 11. I have reworded following comment and restructured the code that follows it
> in the attached patch.
> +    /*
> +     * Set the subquery information.  If the relation performs a full outer
> +     * join and if the input relations have non-NIL remote_conds, the input
> +     * relations need to be deparsed as a subquery.
> +     */
> The code esp. the if .. else .. block followed by another one, made it a bit
> unreadable. Hence I restructured it so that it's readable and doesn't require
> variable "relids" from earlier code, which was being reused. Let me know if
> this change looks good.

That's a good idea.  Thanks!

> 12. The code below deletes the condition, which is understandable.
> -            if (fpinfo_i->remote_conds || fpinfo_o->remote_conds)
> But why does it add a new unrelated condition here? What the comment claims,
> looks like an existing bug, unrelated to the patch. Can you please give an
> example? If that it's an existing bug, it should be fixed as a separate patch.
> I don't understand the relation of this code with what the patch is doing.
> +            /*
> +             * We can't do anything here, and if there are any non-Vars in the
> +             * outerrel/innerrel's reltarget, give up pushing down this join,
> +             * because the deparsing logic can't support such a case currently.
> +             */
> +            if (reltarget_has_non_vars(outerrel))
> +                return false;
> +            if (reltarget_has_non_vars(innerrel))
>                  return false;

I thought the current deparsing logic wouldn't work well if the target 
list of a given subquery contained eg, system columns other than ctid 
and oid, but I noticed that I was wrong; it can, so we don't need this 
restriction.  Will remove.  (I don't think there is any bug.)

> 13. The comment below is missing the main purpose i.e. creating a a unique
> alias, in case the relation gets converted into a subquery. Lowest or highest
> relid will create a unique alias at given level of join and that would be more
> future proof. If we decide to consider paths for every join order, following
> comment will no more be true.
> +    /*
> +     * Set the relation index.  This is defined as the position of this
> +     * joinrel in the join_rel_list list plus the length of the rtable list.
> +     * Note that since this joinrel is at the end of the list when we are
> +     * called, we can get the position by list_length.
> +     */
> +    fpinfo->relation_index =
> +        list_length(root->parse->rtable) + list_length(root->join_rel_list);

I don't agree on that point.  As I said before, the approach using the 
lowest/highest relid would make a remote query having nested subqueries 
difficult to read.  And even if we decided to consider paths for every 
join order, we could define the relation_index safely, by modifying 
postgresGetForeignJoinPaths so that it (1) sets the relation_index the 
proposed way at the first time it is called and (2) avoids setting the 
relation_index after the first call, by checking to see 
fpinfo->relation_index > 0.

> 14. The function name talks about non-vars but the Assert few lines below
> asserts that every node there is a Var. Need better naming.
> +reltarget_has_non_vars(RelOptInfo *foreignrel)
> +{
> +    ListCell   *lc;
> +
> +    foreach(lc, foreignrel->reltarget->exprs)
> +    {
> +        Var           *var = (Var *) lfirst(lc);
> +
> +        Assert(IsA(var, Var));
> And also an explanation for the claim
> + * Note: currently deparseExplicitTargetList can't properly handle such Vars.

Will remove this function for the reason described in #12.

> 15. While deparsing every Var, we are descending the RelOptInfo hierarchy.

Right.  I think that not only avoids creating redundant data to find the 
alias of a given Var node but also would be able to find it in optimal cost.

> Instead of that, we should probably gather all the alias information once and
> store it in context as an array indexed by relid. While deparsing a Var we can
> get the targetlist and alias for a given relation by using var->varno as index.
> And then search for given Var node in the targetlist to deparse the column name
> by its position in the targetlist. For the relations that are not converted
> into subqueries, this array will not hold any information and the Var will be
> deparsed as it's being done today.

Sorry, I don't understand this part.  How does that work when creating a 
remote query having nested subqueries?  Is that able to search for a 
given Var node efficiently?

> Testing
> -------
> 1. The code is changing deparser but doesn't have testcases
> testing impact of the code. For example, we should have testcases with remote
> query deparsed as nested subqueries or nested subqueries within subqueries and
> so on May be testcases where a relation deeper in the RelOptInfo hierarchy has
> conditions but it's immediate upper relation does not have those.

Will do.

> 2. The only testcase added by this patch, copies an existing case and adds a
> whole row reference to one of the relations being joined. Instead we could add
> that whole-row reference to the existing testcase itself.

Will do.

Best regards,
Etsuro Fujita