Re: Expression errors with "FOR UPDATE" and postgres_fdw with partitionwise join enabled. - Mailing list pgsql-hackers

(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?

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

>> +   /* 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).
>
> Done.

Thanks!

>> -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.
>
> I think pull_var_clause is the only place where we do that and I don't
> like that very much. I also don't like is_subquery_var() name since
> it's too specific. We might want that kind of functionality to ship
> generic expressions and not just Vars in future. Usually we would be
> searching for columns in the subquery targetlist so the name change
> looks good to me. IIRC, the function was added one release back, so
> backpatching pain, if any, won't be much. But I don't think we should
> carry a misnomer for many releases to come. Let's differ this to a
> committer.

OK

> Here's set of updated patches.

Thanks for updating the patch set!  I noticed followings.  Sorry for not 
having noticed that before.

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

- On patch 0003-postgres_fdw-child-join-with-ConvertRowtypeExprs-cau.patch:

+   /* Construct ROW expression according to the conversion map. */
+   appendStringInfoString(buf, "ROW(");
+   for (cnt = 0; cnt < parent_desc->natts; cnt++)
+   {
+       /* Ignore dropped columns. */
+       if (attrMap[cnt] == 0)
+           continue;
+
+       if (cnt > 0)
+           appendStringInfoString(buf, ", ");
+       deparseColumnRef(buf, child_var->varno, attrMap[cnt],
+                        planner_rt_fetch(child_var->varno, root),
+                        qualify_col);
+   }
+   appendStringInfoChar(buf, ')');

The result would start with ", " in the case where the cnt=0 column of 
the parent relation is a dropped one.  Here is an example:

postgres=# create table pt1 (a int, b int) partition by range (b);
CREATE TABLE
postgres=# alter table pt1 drop column a;
ALTER TABLE
postgres=# alter table pt1 add column a int;
ALTER TABLE
postgres=# create table loct11 (like pt1);
CREATE TABLE
postgres=# create foreign table pt1p1 partition of pt1 for values from 
(0) to (100) server loopback options (table_name 'loct11', 
use_remote_estimate 'true');
CREATE FOREIGN TABLE
postgres=# insert into pt1 select i, i from generate_series(0, 99, 2) i;
INSERT 0 50
postgres=# analyze pt1;
ANALYZE
postgres=# analyze pt1p1;
ANALYZE
postgres=# create table pt2 (a int, b int) partition by range (b);
CREATE TABLE
postgres=# create table loct21 (like pt2);
CREATE TABLE
postgres=# create foreign table pt2p1 partition of pt2 for values from 
(0) to (100) server loopback options (table_name 'loct21', 
use_remote_estimate 'true');
CREATE FOREIGN TABLE
postgres=# insert into pt2 select i, i from generate_series(0, 99, 3) i;
INSERT 0 34
postgres=# analyze pt2;
ANALYZE
postgres=# analyze pt2p1;
ANALYZE
postgres=# set enable_partitionwise_join to true;
SET
postgres=# explain verbose select pt1.* from pt1, pt2 where pt1.b = 
pt2.b for update of pt1;
ERROR:  syntax error at or near ","
CONTEXT:  remote SQL command: EXPLAIN SELECT r4.b, r4.a, r4.ctid, CASE 
WHEN (r4.*)::text IS NOT NULL THEN ROW(, r4.b, r4.a) END, CASE WHEN 
(r4.*)::text IS NOT NULL THEN 57467 END, r6.ctid, CASE WHEN (r6.*)::text 
IS NOT NULL THEN ROW(r6.a, r6.b) END, CASE WHEN (r6.*)::text IS NOT NULL 
THEN 57476 END FROM (public.loct11 r4 INNER JOIN public.loct21 r6 ON 
(((r4.b = r6.b)))) FOR UPDATE OF r4

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.

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.

Other than that, the patch set looks good to me.

Best regards,
Etsuro Fujita