Hello, The attached file
"unionall_inh_idx_typ3_v6_20140203.patch" is the new version of
this patch fixed the 'whole-row-var' bug.
First of all, on second thought about this,
> > create table parent (a int, b int);
> > create table child () inherits (parent);
> > insert into parent values (1,10);
> > insert into child values (2,20);
> > select a, b from parent union all select a, b from child;
>
> Mmm. I had the same result. Please let me have a bit more time.
This turned out to be a correct result. The two tables have
following records after the two INSERTs.
| =# select * from only parent;
| 1 | 10
| (1 row)
|
| =# select * from child;
| 2 | 20
| (1 row)
Then it is natural that the parent-side in the UNION ALL returns
following results.
| =# select * from parent;
| a | b
| ---+----
| 1 | 10
| 2 | 20
| (2 rows)
Finally, the result we got has proved not to be a problem.
====
Second, about the crash in this sql,
> select parent from parent union all select parent from parent;
It is ignored whole-row reference (=0) which makes the index of
child translated-vars list invalid (-1). I completely ignored it
in spite that myself referred to before.
Unfortunately ConvertRowtypeExpr prevents appendrels from being
removed currently, and such a case don't seem to take place so
often, so I decided to exclude the case. In addition, I found
that only setting "rte->inh = false" causes duplicate scanning on
the same relations through abandoned appendrels so I set
parent/child_relid to InvalidOid so as no more to referred to
from the ex-parent (and ex-children).
The following queries seems to work correctly (but with no
optimization) after these fixes.
create table parent (a int, b int);
create table child () inherits (parent);
insert into parent values (1,10);
insert into child values (2,20);
select parent from parent union all select parent from parent;
parent
--------
(1,10)
(2,20)
(1,10)
(2,20)
(4 rows)
select a, parent from parent union all select a,parent from parent;
a | parent
---+--------
1 | (1,10)
2 | (2,20)
1 | (1,10)
2 | (2,20)
(4 rows)
select a, b from parent union all select a, b from parent;
a | b
---+----
1 | 10
2 | 20
1 | 10
2 | 20
(4 rows)
select a, b from parent union all select a, b from child;
a | b
---+----
2 | 20
1 | 10
2 | 20
(3 rows)
> > > > > The attached two patches are rebased to current 9.4dev HEAD and
> > > > > make check at the topmost directory and src/test/isolation are
> > > > > passed without error. One bug was found and fixed on the way. It
> > > > > was an assertion failure caused by probably unexpected type
> > > > > conversion introduced by collapse_appendrels() which leads
> > > > > implicit whole-row cast from some valid reltype to invalid
> > > > > reltype (0) into adjust_appendrel_attrs_mutator().
> > > >
> > > > What query demonstrated this bug in the previous type 2/3 patches?
> >
> > I would still like to know the answer to the above question.
I rememberd after some struggles. It failed during 'make check',
on the following query in inherit.sql.
| update bar set f2 = f2 + 100
| from
| ( select f1 from foo union all select f1+3 from foo ) ss
| where bar.f1 = ss.f1;
The following SQLs causes the same type of crash.
create temp table foo(f1 int, f2 int);
create temp table foo2(f3 int) inherits (foo);
create temp table bar(f1 int, f2 int);
update bar set f2 = 1
from
( select f1 from foo union all select f1+3 from foo ) ss
where bar.f1 = ss.f1;
The tipped stone was "wholerow1" for the subquery created in
preprocess_targetlist.
| /* Not a table, so we need the whole row as a junk var */
| var = makeWholeRowVar(rt_fetch(rc->rti, range_table),
..
| snprintf(resname, sizeof(resname), "wholerow%u", rc->rowmarkId);
Then the finishing blow was a appendRelInfo corresponding to the
var above, whose parent_reltype = 0 and child_reltype != 0, and
of course introduced by my patch. Since such a situation takes
place even for this patch, the modification is left alone.
regards,
--
Kyotaro Horiguchi
NTT Open Source Software Center
diff --git a/src/backend/optimizer/prep/prepunion.c b/src/backend/optimizer/prep/prepunion.c
index 52dcc72..ece9318 100644
--- a/src/backend/optimizer/prep/prepunion.c
+++ b/src/backend/optimizer/prep/prepunion.c
@@ -57,6 +57,12 @@ typedef struct
AppendRelInfo *appinfo;
} adjust_appendrel_attrs_context;
+typedef struct {
+ AppendRelInfo *child_appinfo;
+ Index target_rti;
+ bool failed;
+} transvars_merge_context;
+
static Plan *recurse_set_operations(Node *setOp, PlannerInfo *root,
double tuple_fraction,
List *colTypes, List *colCollations,
@@ -98,6 +104,8 @@ static List *generate_append_tlist(List *colTypes, List *colCollations,
List *input_plans,
List *refnames_tlist);
static List *generate_setop_grouplist(SetOperationStmt *op, List *targetlist);
+static Node *transvars_merge_mutator(Node *node,
+ transvars_merge_context *ctx);
static void expand_inherited_rtentry(PlannerInfo *root, RangeTblEntry *rte,
Index rti);
static void make_inh_translation_list(Relation oldrelation,
@@ -1210,6 +1218,48 @@ expand_inherited_tables(PlannerInfo *root)
}
}
+static Node *
+transvars_merge_mutator(Node *node, transvars_merge_context *ctx)
+{
+ if (node == NULL)
+ return NULL;
+
+ /* fast path after failure */
+ if (ctx->failed)
+ return node;
+
+ if (IsA(node, Var))
+ {
+ Var *oldv = (Var*)node;
+
+ if (!oldv->varlevelsup && oldv->varno == ctx->target_rti)
+ {
+ if (oldv->varattno == 0)
+ {
+ /*
+ * Appendrels which does whole-row-var conversion cannot be
+ * removed. ConvertRowtypeExpr can convert only RELs which can
+ * be referred to using relid.
+ */
+ ctx->failed = true;
+ return node;
+ }
+ if (oldv->varattno >
+ list_length(ctx->child_appinfo->translated_vars))
+ elog(ERROR,
+ "attribute %d of relation \"%s\" does not exist",
+ oldv->varattno,
+ get_rel_name(ctx->child_appinfo->parent_reloid));
+
+ return (Node*)copyObject(
+ list_nth(ctx->child_appinfo->translated_vars,
+ oldv->varattno - 1));
+ }
+ }
+ return expression_tree_mutator(node,
+ transvars_merge_mutator, (void*)ctx);
+}
+
/*
* expand_inherited_rtentry
* Check whether a rangetable entry represents an inheritance set.
@@ -1237,6 +1287,8 @@ expand_inherited_rtentry(PlannerInfo *root, RangeTblEntry *rte, Index rti)
List *inhOIDs;
List *appinfos;
ListCell *l;
+ AppendRelInfo *parent_appinfo = NULL;
+ bool detach_parent = false;
/* Does RT entry allow inheritance? */
if (!rte->inh)
@@ -1301,6 +1353,22 @@ expand_inherited_rtentry(PlannerInfo *root, RangeTblEntry *rte, Index rti)
oldrc->isParent = true;
/*
+ * If parent relation is appearing in a subselect of UNION ALL, it has
+ * further parent appendrelinfo. Save it to pull up inheritance children
+ * later.
+ */
+ foreach(l, root->append_rel_list)
+ {
+ AppendRelInfo *appinfo = (AppendRelInfo *)lfirst(l);
+ if(appinfo->child_relid == rti)
+ {
+ parent_appinfo = appinfo;
+ detach_parent = true;
+ break;
+ }
+ }
+
+ /*
* Must open the parent relation to examine its tupdesc. We need not lock
* it; we assume the rewriter already did.
*/
@@ -1308,6 +1376,7 @@ expand_inherited_rtentry(PlannerInfo *root, RangeTblEntry *rte, Index rti)
/* Scan the inheritance set and expand it */
appinfos = NIL;
+
foreach(l, inhOIDs)
{
Oid childOID = lfirst_oid(l);
@@ -1378,6 +1447,46 @@ expand_inherited_rtentry(PlannerInfo *root, RangeTblEntry *rte, Index rti)
}
/*
+ * Pull up this appinfo onto just above of the parent. The parent
+ * relation has its own parent when it appears as a subquery of UNION
+ * ALL. Pulling up these children gives a chance to consider
+ * MergeAppend on whole the UNION ALL tree.
+ */
+
+ if (parent_appinfo)
+ {
+ transvars_merge_context ctx;
+
+ ctx.child_appinfo = appinfo;
+ ctx.target_rti = rti;
+
+ if (parent_appinfo->parent_reltype == 0 &&
+ parent_appinfo->child_reltype == 0)
+ {
+ List *new_transvars;
+
+ /*
+ * Connect this appinfo up to the parent RTE of
+ * parent_appinfo.
+ */
+ ctx.failed = false;
+ new_transvars = (List*)expression_tree_mutator(
+ (Node*)parent_appinfo->translated_vars,
+ transvars_merge_mutator, &ctx);
+
+ if (ctx.failed)
+ /* Some children remain so this parent cannot detach */
+ detach_parent = false;
+ else
+ {
+ appinfo->parent_relid = parent_appinfo->parent_relid;
+ appinfo->parent_reltype = parent_appinfo->parent_reltype;
+ appinfo->translated_vars = new_transvars;
+ }
+ }
+ }
+
+ /*
* Build a PlanRowMark if parent is marked FOR UPDATE/SHARE.
*/
if (oldrc)
@@ -1399,6 +1508,14 @@ expand_inherited_rtentry(PlannerInfo *root, RangeTblEntry *rte, Index rti)
heap_close(newrelation, NoLock);
}
+ /* Remove childless appinfo from inheritance tree */
+ if (detach_parent)
+ {
+ rte->inh = false;
+ parent_appinfo->parent_relid = InvalidOid;
+ parent_appinfo->child_relid = InvalidOid;
+ }
+
heap_close(oldrelation, NoLock);
/*
@@ -1662,7 +1779,8 @@ adjust_appendrel_attrs_mutator(Node *node,
* step to convert the tuple layout to the parent's rowtype.
* Otherwise we have to generate a RowExpr.
*/
- if (OidIsValid(appinfo->child_reltype))
+ if (OidIsValid(appinfo->child_reltype) &&
+ OidIsValid(appinfo->parent_reltype))
{
Assert(var->vartype == appinfo->parent_reltype);
if (appinfo->parent_reltype != appinfo->child_reltype)
diff --git a/src/test/regress/expected/union.out b/src/test/regress/expected/union.out
index 6f9ee5e..d254ff3 100644
--- a/src/test/regress/expected/union.out
+++ b/src/test/regress/expected/union.out
@@ -502,9 +502,40 @@ explain (costs off)
Index Cond: (ab = 'ab'::text)
(7 rows)
+--
+-- Test that ORDER BY for UNION ALL can be pushed down on inheritance
+-- tables.
+--
+CREATE TEMP TABLE t1c (like t1 including indexes) inherits (t1);
+NOTICE: merging column "a" with inherited definition
+NOTICE: merging column "b" with inherited definition
+CREATE TEMP TABLE t2c (like t2 including indexes) inherits (t2);
+NOTICE: merging column "ab" with inherited definition
+INSERT INTO t1c VALUES ('v', 'w'), ('c', 'd');
+INSERT INTO t2c VALUES ('vw'), ('cd');
+set enable_seqscan = on;
+set enable_indexonlyscan = off;
+explain (costs off)
+ SELECT * FROM
+ (SELECT a || b AS ab FROM t1
+ UNION ALL
+ SELECT * FROM t2) t
+ ORDER BY ab LIMIT 1;
+ QUERY PLAN
+--------------------------------------------------
+ Limit
+ -> Merge Append
+ Sort Key: ((t1.a || t1.b))
+ -> Index Scan using t1_ab_idx on t1
+ -> Index Scan using t1c_expr_idx on t1c
+ -> Index Scan using t2_pkey on t2
+ -> Index Scan using t2c_pkey on t2c
+(7 rows)
+
reset enable_seqscan;
reset enable_indexscan;
reset enable_bitmapscan;
+reset enable_indexonlyscan;
-- Test constraint exclusion of UNION ALL subqueries
explain (costs off)
SELECT * FROM
diff --git a/src/test/regress/sql/union.sql b/src/test/regress/sql/union.sql
index d567cf1..7f9a9b1 100644
--- a/src/test/regress/sql/union.sql
+++ b/src/test/regress/sql/union.sql
@@ -198,9 +198,29 @@ explain (costs off)
SELECT * FROM t2) t
WHERE ab = 'ab';
+--
+-- Test that ORDER BY for UNION ALL can be pushed down on inheritance
+-- tables.
+--
+
+CREATE TEMP TABLE t1c (like t1 including indexes) inherits (t1);
+CREATE TEMP TABLE t2c (like t2 including indexes) inherits (t2);
+INSERT INTO t1c VALUES ('v', 'w'), ('c', 'd');
+INSERT INTO t2c VALUES ('vw'), ('cd');
+set enable_seqscan = on;
+set enable_indexonlyscan = off;
+
+explain (costs off)
+ SELECT * FROM
+ (SELECT a || b AS ab FROM t1
+ UNION ALL
+ SELECT * FROM t2) t
+ ORDER BY ab LIMIT 1;
+
reset enable_seqscan;
reset enable_indexscan;
reset enable_bitmapscan;
+reset enable_indexonlyscan;
-- Test constraint exclusion of UNION ALL subqueries
explain (costs off)
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
