Re: UNION ALL on partitioned tables won't use indices. - Mailing list pgsql-hackers

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 | 102 | 201 | 102 | 20
(4 rows)
select a, b from parent union all select a, b from child;a | b  
---+----2 | 201 | 102 | 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
alreadydid.     */
 
@@ -1308,6 +1376,7 @@ expand_inherited_rtentry(PlannerInfo *root, RangeTblEntry *rte, Index rti)    /* Scan the
inheritanceset 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
theparent'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 subqueriesexplain (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 subqueriesexplain (costs off)