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
postgres-fdw-more-full-join-pushdown-v2.patch
Description: binary/octet-stream
-- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
