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

(2018/05/17 23:19), Ashutosh Bapat wrote:
> On Thu, May 17, 2018 at 4:50 PM, Etsuro Fujita
> <fujita.etsuro@lab.ntt.co.jp>  wrote:
>> (2018/05/16 22:49), Ashutosh Bapat wrote:
>>> On Wed, May 16, 2018 at 4:01 PM, Etsuro Fujita
>>> <fujita.etsuro@lab.ntt.co.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().

Yeah, ISTM that the inheritance test makes things worse.

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

Let's ask that.

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

Thanks for updating the patch set!  Other than pull_var_clause things, 
the updated version looks good to me, so I'll mark this as Ready for 
Committer.  As I said before, I think this issue should be addressed in 
advance of the PG11 release.

Best regards,
Etsuro Fujita