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

(2018/05/14 15:34), Ashutosh Bapat wrote:
> 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:
>> 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;

Thanks!

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

Actually, we'd get the same error without using anything in
partitioning. Here is an example:

postgres=# create table parent (a int, b text);
CREATE TABLE
postgres=# create table child (a int, b text);
CREATE TABLE
postgres=# alter table child inherit parent ;
ALTER TABLE
postgres=# alter table child add constraint child_check check
(child::parent::text != NULL);
ERROR: ConvertRowtypeExpr found where not expected

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

Sorry, I don't understand this fully.

>> So, it's not clear to me when we add
>> a ConvertRowtypeExpr and we don't.

I think it'd be OK to rewrite so at least in prepare_sort_from_pathkeys
and build_tlist_to_deparse.

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

I have to admit that the approach I proposed is a band-aid fix.

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

Yeah, I really agree on that point! 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.

> Here's set of updated patches.

Thanks for updating the patch!

Here are some other minor comments on patch
0003-postgres_fdw-child-join-with-ConvertRowtypeExprs-cau.patch:

+ /* Construct the conversion map. */
+ parent_desc = lookup_rowtype_tupdesc(cre->resulttype, 0);

I think it'd be better to pass -1, not 0, as the typmod argument for
that function because that would be consistent with other places where
we use that function with named rowtypes (eg, ruleutils.c).

-is_subquery_var(Var *node, RelOptInfo *foreignrel, int *relno, int *colno)
+is_subquery_column(Node *node, Index varno, RelOptInfo *foreignrel,
+ int *relno, int *colno)

-1 for that change; because 1) we use "var" for implying many things as
in eg, pull_var_clause and 2) I think it'd make back-patching easy to
keep the name as-is.

Other than that the patch set looks good to me.

Best regards,
Etsuro Fujita