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.

> 4. The changes related to retrieved_attrs look unrelated to the patch.
> Either your patch should use the current method of handling
> retrieved_attrs or there should be a separate patch for retrieved_attrs
> changes. May be you want to take a look at the discussion in join
> pushdown thread as to why we assume retrieved_attrs to be non-NIL always.

OK, I removed those changes from the patch.

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

Attached is an updated version of the patch.

Other changes:

* Add a bit more regression test
* Revise code/comments
* Cleanups

Thanks for the comments!

Best regards,
Etsuro Fujita