(2018/05/16 22:49), Ashutosh Bapat wrote:
On Wed, May 16, 2018 at 4:01 PM, Etsuro Fujita
<fujita.ets...@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
