Re: Expression errors with "FOR UPDATE" and postgres_fdw with partition wise join enabled.

On Tue, Apr 24, 2018 at 4:49 PM, Etsuro Fujita
<fujita.etsuro@lab.ntt.co.jp> wrote:
> (2018/04/17 19:00), Etsuro Fujita wrote:
>>
>> (2018/04/17 18:43), Ashutosh Bapat wrote:
>>>
>>> Here's updated patch-set.
>>
>>
>> Will review.
>
>
> I started reviewing this.
>
> o 0001-Handle-ConvertRowtypeExprs-in-pull_vars_clause.patch:
>
> +   else if (IsA(node, ConvertRowtypeExpr))
> +   {
> +#ifdef USE_ASSERT_CHECKING
> +       ConvertRowtypeExpr *cre = castNode(ConvertRowtypeExpr, node);
> +       Var        *var;
> +
> +       /*
> +        * ConvertRowtypeExprs only result when parent's whole-row reference
> is
> +        * translated for a child using adjust_appendrel_attrs(). That
> function
> +        * does not handle any upper level Var references.
> +        */
> +       while (IsA(cre->arg, ConvertRowtypeExpr))
> +           cre = castNode(ConvertRowtypeExpr, cre->arg);
> +       var = castNode(Var, cre->arg);
> +       Assert (var->varlevelsup == 0);
> +#endif /* USE_ASSERT_CHECKING */
>
> Isn't it better to throw ERROR as in other cases in pull_vars_clause()?

Actually I noticed that ConvertRowtypeExpr are used to cast a child's
whole row reference expression (not just a Var node) into that of its
parent and not. For example a cast like NULL::child::parent produces a
ConvertRowtypeExpr encapsulating a NULL constant node and not a Var
node. We are interested only in ConvertRowtypeExprs embedding Var
nodes with Var::varattno = 0. I have changed this code to use function
is_converted_whole_row_reference() instead of the above code with
Assert. In order to use that function, I had to take it out of
setrefs.c and put it in nodeFuncs.c which seemed to be a better fit.

> Another thing I noticed about this patch is this:
>
> postgres=# create table prt1 (a int, b int, c varchar) partition by range
> (a);
> postgres=# create table prt1_p1 partition of prt1 FOR VALUES FROM (0) TO
> (250);
> postgres=# create table prt1_p2 partition of prt1 FOR VALUES FROM (250) TO
> (500)
> ;
> postgres=# insert into prt1 select i, i % 25, to_char(i, 'FM0000') from
> generate
> _series(0, 499) i where i % 2 = 0;
> postgres=# analyze prt1;
> postgres=# create table prt2 (a int, b int, c varchar) partition by range
> (b);
> postgres=# create table prt2_p1 partition of prt2 FOR VALUES FROM (0) TO
> (250);
> postgres=# create table prt2_p2 partition of prt2 FOR VALUES FROM (250) TO
> (500)
> ;
> postgres=# insert into prt2 select i % 25, i, to_char(i, 'FM0000') from
> generate
> _series(0, 499) i where i % 3 = 0;
> postgres=# analyze prt2;
>
> postgres=# update prt1 t1 set c = 'foo' from prt2 t2 where t1::text =
> t2::text a
> nd t1.a = t2.b;
> ERROR:  ConvertRowtypeExpr found where not expected
>
> To fix this, I think we need to pass PVC_RECURSE_CONVERTROWTYPES to
> pull_vars_clause() in distribute_qual_to_rels() and
> generate_base_implied_equalities_no_const() as well.

Thanks for the catch. I have updated patch with your suggested fix.

-- 
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company