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

On Thu, Jul 12, 2018 at 4:32 PM, Etsuro Fujita
<fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)jp> wrote:
>
> In this example, the value of the whole-row reference to the child table
> ptp1 for that record is ('foo',1), and that of the index expression for that
> record is (1,'foo'). Those have different column orders, but the latter
> could be mapped to the former by a technique like do_convert_tuple. So,
> what I suspect is: by producing the value of the whole-row reference from
> that of the index expression like that,

The expression in this case would look like ptp1::pt::ptp1 which won't
match targetlist expression ptp1. I am also doubtful that the planner
will be able to deduce that it need to apply an inverse function of
::pt and what exactly such an inverse function is. So index only scan
won't be picked.

> we could support index-only scans
> with such an index in the case where we have the whole-row reference in the
> targetlist, not the index expression itself.

Can you please show an index only scan path being created in this case?

>
>> If that time is long
>> enough, that's ok. In this case, I agree that if we haven't heard from
>> users till now that they need to create indexes on whole-row
>> expressions, there's chance that we will never implement this feature.
>
>
> I don't object to adding that feature to future versions of PG if required.
>
>>>>> So, I think it is safe to have whole-row
>>>>> Vars instead of ConvertRowtypeExprs in the targetlist.
>>>>
>>>>
>>>>
>>>> when it's looking for an expression, it finds a whole-row expression
>>>> so it think it needs to add a ConvertRowtypeExpr on that. But when the
>>>> plan is created, there is ConvertRowtypeExpr already, but there is no
>>>> way to know that a new ConvertRowtypeExpr is not needed anymore. So,
>>>> we may have two ConvertRowtypeExprs giving wrong results.
>>>
>>>
>>>
>>> Maybe I'm missing something, but I don't think that we need to worry
>>> about
>>> that, because in the approach I proposed, we only add CREs above
>>> whole-row
>>> Vars in the targetlists for subplans of an Append/MergeAppend for a
>>> partitioned relation at plan creation time.
>>
>>
>> There's a patch in an adjacent thread started by David Rowley to rip
>> out Append/MergeAppend when there is only one subplan. So, your
>> solution won't work there.
>
>
> Thanks for sharing that information! I skimmed the thread. I haven't yet
> caught up with all the discussions there, so I think I'm missing something,
> but it looks like that we haven't yet reached any consensus on the way to
> go. In my opinion, I like the approach mentioned in [1]. And if we go that
> way, my patch seems to fit into that, because in that approach the
> Append/MergeAppend could be removed after adjusting the targetlists for its
> subplans in create_append_plan/create_merge_append_plan. Anyway, I'd like
> to join in that work for PG12.

Whatever may be the outcome of that work, I think what we fix here
shouldn't require to reverted in a few months from now, just so that
that patch works.

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