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

(2018/07/12 13:38), Ashutosh Bapat wrote:
> On Thu, Jul 12, 2018 at 9:02 AM, Etsuro Fujita
> <fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)jp> wrote:
>> (2018/07/11 20:02), Ashutosh Bapat wrote:
>>> On Wed, Jul 11, 2018 at 1:23 PM, Etsuro Fujita
>>> <fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)jp> wrote:
>>>> Actually, even if we could create such an index on the child table and
>>>> the
>>>> targetlist had the ConvertRowtypeExpr, the planner would still not be
>>>> able
>>>> to use an index-only scan with that index; because check_index_only would
>>>> not consider that an index-only scan is possible for that index because
>>>> that
>>>> index is an expression index and that function currently does not
>>>> consider
>>>> that index expressions are able to be returned back in an index-only
>>>> scan.
>>>> That behavior of the planner might be improved in future, though.
>>
>>
>>> Right and when we do so, not having ConvertRowtypeExpr in the
>>> targetlist will be a problem.
>>
>>
>> Yeah, but I don't think that that's unsolvable; because in that case the CRE
>> as an index expression could be converted back to the whole-row Var's
>> rowtype by adding another CRE to the index expression for that conversion, I
>> suspect that that special handling could allow us to support an index-only
>> scan even when having the whole-row Var instead of the CRE in the
>> targetlist.
>
> I am not able to understand this. Can you please provide an example?

Sorry, my explanation was not good. Let me try. We can't create an
inherited index on the whole-row reference in a partitioned table, but
actually, we can directly create the corresponding index on a child table:

postgres=# create table pt (a int, b text) partition by list (a);
CREATE TABLE
postgres=# create table ptp1 (b text, a int check (a in (1)));
CREATE TABLE
postgres=# alter table pt attach partition ptp1 for values in (1);
ALTER TABLE
postgres=# create index ptp1_conv_wr_idx on ptp1 ((ptp1::pt));
CREATE INDEX
postgres=# insert into pt values (1, 'foo');
INSERT 0 1
postgres=# select *, ptp1 as wr, ptp1::pt as conv_wr from ptp1;
b | a | wr | conv_wr
-----+---+---------+---------
foo | 1 | (foo,1) | (1,foo)
(1 row)

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

>> (Having said that, I'm not 100% sure we need to solve that
>> problem when we improve the planner, because there doesn't seem to me to be
>> enough use-case to justify making the code complicated for that.) Anyway, I
>> think that that would be a matter of future versions of PG.
>
> I am afraid that a fix which just works only till we change the other
> parts of the system is useful only till that time. At that time, it
> needs to be replaced with a different fix.

I agree on that point.

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

Best regards,
Etsuro Fujita

[1] https://www.postgresql.org/message-id/1867.1509032831%40sss.pgh.pa.us