(2018/05/11 16:17), Ashutosh Bapat wrote:
On Thu, May 10, 2018 at 6:23 PM, Etsuro Fujita
<fujita.ets...@lab.ntt.co.jp> wrote:
Yeah, but I think the IsA test would be sufficient to make things work
correctly because we can assume that there aren't any other nodes than Var,
PHV, and CRE translating a wholerow value of a child table into a wholerow
value of its parent table's rowtype in the expression clause to which we
apply pull_var_clause with PVC_INCLUDE_CONVERTROWTYPES.
Not really.
Consider following case using the same tables fprt1 and fprt2 in test
sql/postgres_fdw.sql
create function row_as_is(a ftprt2_p1) returns ftprt2_p1 as $$begin
return a; end;$$ language plpgsql;
With the change you propose i.e.
diff --git a/src/backend/optimizer/util/var.c b/src/backend/optimizer/util/var.c
index f972712..088da14 100644
--- a/src/backend/optimizer/util/var.c
+++ b/src/backend/optimizer/util/var.c
@@ -646,8 +646,9 @@ pull_var_clause_walker(Node *node,
pull_var_clause_context *context)
else
elog(ERROR, "PlaceHolderVar found where not expected");
}
- else if (is_converted_whole_row_reference(node))
+ else if (IsA(node, ConvertRowtypeExpr))
{
+ Assert(is_converted_whole_row_reference(node));
if (context->flags& PVC_INCLUDE_CONVERTROWTYPES)
{
context->varlist = lappend(context->varlist, node);
following query crashes at Assert(is_converted_whole_row_reference(node));
I guess you forgot to show the query.
If we remove that assert, it would end up pushing down row_as_is(),
which is a local user defined function, to the foreign server. That's
not desirable since the foreign server may not have that user defined
function.
I don't understand the counter-example you tried to show, but I think
that I was wrong here; the IsA test isn't sufficient.
BTW, one thing I noticed about the new version of the patch is: there is yet
another case where pull_var_clause produces an error. Here is an example:
To produce this, apply the patch in [1] as well as the new version; without
that patch in [1], the update query would crash the server (see [1] for
details). To fix this, I think we need to pass PVC_RECURSE_CONVERTROWTYPES
to pull_var_clause in build_remote_returning in postgres_fdw.c as well.
I missed that call to PVC since it was added after I had created my
patches. That's a disadvantage of having changed PVC to use flags
instead of bools. We won't notice such changes. Thanks for pointing it
out. Changed as per your suggestion.
Thanks for that change and the updated version!
Yet yet another case where pull_var_clause produces an error:
postgres=# create table list_pt (a int, b text) partition by list (a);
CREATE TABLE
postgres=# create table list_ptp1 partition of list_pt for values in (1);
CREATE TABLE
postgres=# alter table list_ptp1 add constraint list_ptp1_check check
(list_ptp1::list_pt::text != NULL);
ERROR: ConvertRowtypeExpr found where not expected
The patch kept the flags passed to pull_var_clause in
src/backend/catalog/heap.c as-is, but I think we need to modify that
accordingly. Probably, the flags passed to pull_var_clause in
CreateTrigger as well.
Having said that, I started to think this approach would make code much
complicated. Another idea I came up with to avoid that would be to use
pull_var_clause as-is and then rewrite wholerows of partitions back to
ConvertRowtypeExprs translating those wholerows to wholerows of their
parents tables' rowtypes if necessary. That would be annoying and a
little bit inefficient, but the places where we need to rewrite is
limited: prepare_sort_from_pathkeys and build_tlist_to_deparse, IIUC.
So we could really minimize the code change and avoid the additional
overhead caused by the is_converted_whole_row_reference test added to
pull_var_clause. I think the latter would be rather important, because
PWJ is disabled by default. What do you think about that approach?
Best regards,
Etsuro Fujita
