Re: Expression errors with "FOR UPDATE" and postgres_fdw withpartition wise join enabled. - Mailing list pgsql-hackers

On Fri, May 11, 2018 at 6:31 PM, Etsuro Fujita
<fujita.etsuro@lab.ntt.co.jp> wrote:
>
> I guess you forgot to show the query.

Sorry. Here's the query.

explain verbose select * from fprt1 t1 join fprt2 t2 on t1.a = t2.b
where t1 = row_as_is(row(t2.a, t2.b, t2.c)::ftprt2_p1)::fprt2;

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

With all the support that we have added in partitioning for v11, I
think we need to add PVC_RECURSE_CONVERTROWTYPE at all the places,
which were left unchanged in the earlier versions of patches, which
were written when all that support wasn't in v11.

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

Other reason why we can't use your approach is we can not decide
whether ConvertRowtypeExpr is needed by just looking at the Var node.
You might say, that we add a ConvertRowtypeExpr if the Var::varno
references a child relation. But that's not true. For example a whole
row reference embedded in ConvertRowtypeExpr added by query
adjustments in inheritance_planner() is not a child's whole-row
reference in the adjusted query. So, it's not clear to me when we add
a ConvertRowtypeExpr and we don't. I am not sure if those are the only
two places which require a fix. Right now it looks like those are the
places which need PVC_INCLUDE_CONVERTROWTYPE, but that may not be true
in the future, esp. given the amount of work we expect to happen in
the partitioning area. When that happens, fixing all those places in
that way wouldn't be good.

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

Partition-wise join and partition-wise aggregation are disabled by
default temporarily. We will change it in near future once the memory
consumption issue has been tackled. The features are useful and users
would want those to be enabled by default like other query
optimizations. So, we can't take a decision based on that.

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