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

On Mon, May 14, 2018 at 10:21 AM, Ashutosh Bapat
<ashutosh(dot)bapat(at)enterprisedb(dot)com> wrote:
> On Fri, May 11, 2018 at 6:31 PM, Etsuro Fujita
> <fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)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.

Here's set of updated patches.

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