Re: Push down more full joins in postgres_fdw

Hi Ashutosh,

On 2016/08/22 15:49, Ashutosh Bapat wrote:

1. deparsePlaceHolderVar looks odd - each of the deparse* function is
named as deparse + <name of the parser node the string would parse
into>. PlaceHolderVar is not a parser node, so no string is going to be
parsed as a PlaceHolderVar. May be you want to name it as
deparseExerFromPlaceholderVar or something like that.

The name "deparseExerFromPlaceholderVar" seems long to me.  How about "deparsePlaceHolderExpr"?

2. You will need to check phlevelsup member while assessing whether a
PHV is safe to push down.

Good catch!  In addition to that, I added another check that the eval_at set for the PHV should be included in the relids set for the foreign relation.  I think that would make the shippability check more robust.

3. I think registerAlias stuff should happen really at the time of
creating paths and should be stored in fpinfo. Without that it will be
computed every time we deparse the query. This means every time we try
to EXPLAIN the query at various levels in the join tree and once for the
query to be sent to the foreign server.

Hmm.  I think the overhead in calculating aliases would be negligible compared to the overhead in explaining each remote query for costing or sending the remote query for execution.  So, I created aliases in the same way as remote params created and stored into params_list in deparse_expr_cxt.  I'm not sure it's worth complicating the code.

5. The blocks related to inner and outer relations in
deparseFromExprForRel() look same. We should probably separate that code
out into a function and call it at two places.

Done.

6.
!     if (is_placeholder)
!         errcontext("placeholder expression at position %d in select list",
!                    errpos->cur_attno);
A user wouldn't know what a placeholder expression is, there is no such
term in the documentation. We have to device a better way to provide an
error context for an expression in general.

Though I proposed that, I don't think that it's that important to let users know that the expression is from a PHV.  How about just saying "expression", not "placeholder expression", so that we have the message "expression at position %d in select list" in the context?