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

(2018/05/16 22:49), Ashutosh Bapat wrote:
> On Wed, May 16, 2018 at 4:01 PM, Etsuro Fujita
> <fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)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