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