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

On Thu, May 17, 2018 at 4:50 PM, Etsuro Fujita
<fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)jp> wrote:
> (2018/05/16 22:49), Ashutosh Bapat wrote:
>>
>> On Wed, May 16, 2018 at 4:01 PM, Etsuro Fujita
>> <fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)jp> wrote:
>>>
>>> However, considering that
>>> pull_var_clause is used in many places where partitioning is *not*
>>> involved,
>>> I still think it's better to avoid spending extra cycles needed only for
>>> partitioning in that function.
>>
>>
>> Right. The changes done in inheritance_planner() imply that we can
>> encounter a ConvertRowtypeExpr even in the expressions for a relation
>> which is not a child relation in the translated query. It was a child
>> in the original query but after translating it becomes a parent and
>> has ConvertRowtypeExpr somewhere there.
>
>
> Sorry, I don't understand this. Could you show me such an example?

Even without inheritance we can induce a ConvertRowtypeExpr on a base
relation which is not "other" relation

create table parent (a int, b int);
create table child () inherits(parent);
alter table child add constraint whole_par_const check ((child::parent).a = 1);

There is no way to tell by looking at the parsed query whether
pull_var_clause() from StoreRelCheck() will encounter a
ConvertRowtypeExpr or not. We could check whether the tables involved
are inherited or not, but that's more expensive than
is_converted_whole_row_reference().

>
>> So, there is no way to know
>> whether a given expression will have ConvertRowtypeExpr in it. That's
>> my understanding. But if we can device such a test, I am happy to
>> apply that test before or witin the call to pull_var_clause().
>>
>> We don't need that expensive test if user has passed
>> PVC_RECURSE_CONVERTROWTYPE. So we could test that first and then check
>> the shape of expression tree. It would cause more asymmetry in
>> pull_var_clause(), but we can live with it or change the order of
>> tests for all the three options. I will differ that to a committer.
>
>
> Sounds more invasive. Considering where we are in the development cycle for
> PG11, I think it'd be better to address this by something like the approach
> I proposed. Anyway, +1 for asking the committer's opinion.

Thanks.

>
> - On patch 0001-Handle-ConvertRowtypeExprs-in-pull_vars_clause.patch:
>
> +extern bool
> +is_converted_whole_row_reference(Node *node)
>
> I think we should remove "extern" from the definition.

Done.

>
> - On patch 0003-postgres_fdw-child-join-with-ConvertRowtypeExprs-cau.patch:
>
>
> I think we can fix this by adding another flag to indicate whether we
> deparsed the first live column of the relation, as in the "first" bool flag
> in deparseTargetList.

Thanks. Fixed.

>
> One more thing to add is: the patch adds support for deparsing a
> ConvertRowtypeExpr that translates a wholerow of a childrel into a wholerow
> of its parentrel's rowtype, so by modifying is_foreign_expr accordingly, we
> could push down such CREs in JOIN ON conditions to the remote for example.
> But that would be an improvement, not a fix, so I think we should leave that
> for another patch for PG12.

Right.

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