Re: UNION ALL on partitioned tables won't use indices.

Hello, Thank you, and I' sorry to have kept you waiting.

> Let's focus on type 3; Tom and I both found it most promising.

Agreed.

> > 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?
> 
> >  - unionall_inh_idx_typ3_v4_20140114.patch
> 
> You have not addressed my review comments from November:
> http://www.postgresql.org/message-id/20131123073913.GA1008138@tornado.leadboat.com

mmm. Sorry, I've overlooked most of it..

em_is_child is no more a issue, and the rest seem to cited below, thanks.

> Specifically, these:
> 
> [transvar_merge_mutator()] has roughly the same purpose as
> adjust_appendrel_attrs_mutator(), but it propagates the change to far fewer
> node types.  Why this case so much simpler?  The parent translated_vars of
> parent_appinfo may bear mostly-arbitrary expressions.

There are only two places where AppendRelInfo node is generated,
expand_inherited_rtentry and
pull_up_union_leaf_queries.(_copyAppendrelInfo is irrelevant to
this discussion.) The core part generating translated_vars for
them are make_inh_translation_list and
make_setop_translation_list respectively. That's all. And they
are essentially works on the same way, making a Var for every
referred target entry of children like following.

| makeVar(varno,
|         tle->resno,
|         exprType((Node *) tle->expr),
|         exprTypmod((Node *) tle->expr),
|         exprCollation((Node *) tle->expr),
|         0);

So all we should do to collapse nested appendrels is simplly
connecting each RTEs directly to the root (most ancient?) RTE in
the relationship, resolving Vars like above, as seen in patched
expand_inherited_rtentry.

# If translated_vars are generated always in the way shown above,
# using tree walker might be no use..

This can be done apart from all other stuffs compensating
translation skew done in adjust_appendrel_attrs. I believe they
are in orthogonal relationship.


> Approaches (2) and (3) leave the inheritance parent with rte->inh == true
> despite no AppendRelInfo pointing to it as their parent.  Until now,
> expand_inherited_rtentry() has been careful to clear rte->inh in such cases.

Thank you. I missed that point. rte->inh at first works as a
trigger to try expand inheritance and create append_rel_list
entries, and later works to say "dig me through appinfos". From
that point of view, inh of the RTEs whose children took away
should be 0. The two meanings of inh are now become different
from each other so I suppose it'd better rename it, but I don't
come up with good alternatives..

Anyway this is corrected in the patch attached and works as
follows,

BEFORE:rte[1] Subquery "SELECT*1", inh = 1   +- appinfo[0] - rte[4] Relation "p1", inh = 1   |               +-
appinfo[2]- rte[6]  Relation "p1", inh = 0   |               +- appinfo[3] - rte[7]  Relation "c11", inh = 0   |
      +- appinfo[4] - rte[8]  Relation "c12", inh = 0   +- appinfo[1] - rte[5] Relation "p2", inh = 1
+-appinfo[5] - rte[9]  Relation "p1", inh = 0                   +- appinfo[6] - rte[10] Relation "c11", inh = 0
         +- appinfo[7] - rte[11] Relation "c12", inh = 0
 

COLLAPSED:rte[1] Subquery "SELECT*1", inh = 1   +- appinfo[0] - rte[4]  Relation "p1",  inh = 1 => 0   +- appinfo[2] -
rte[6] Relation "p1",  inh = 0   +- appinfo[3] - rte[7]  Relation "c11", inh = 0   +- appinfo[4] - rte[8]  Relation
"c12",inh = 0   +- appinfo[1] - rte[5]  Relation "p2",  inh = 1 => 0   +- appinfo[5] - rte[9]  Relation "p1",  inh = 0
+- appinfo[6] - rte[10] Relation "c11", inh = 0   +- appinfo[7] - rte[11] Relation "c12", inh = 0
 


> I get this warning:
> 
> prepunion.c: In function `expand_inherited_rtentry':
> prepunion.c:1450: warning: passing argument 1 of `expression_tree_mutator' from incompatible pointer type

Sorry, I forgot to put a casting to generic type. It is fixed in
the attached version.

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..6ef82d7 100644
--- a/src/backend/optimizer/prep/prepunion.c
+++ b/src/backend/optimizer/prep/prepunion.c
@@ -57,6 +57,11 @@ typedef struct    AppendRelInfo *appinfo;} adjust_appendrel_attrs_context;
+typedef struct {
+    AppendRelInfo    *child_appinfo;
+    Index             target_rti;
+} transvars_merge_context;
+static Plan *recurse_set_operations(Node *setOp, PlannerInfo *root,                       double tuple_fraction,
               List *colTypes, List *colCollations,
 
@@ -98,6 +103,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 +1217,34 @@ expand_inherited_tables(PlannerInfo *root)    }}
+static Node *
+transvars_merge_mutator(Node *node, transvars_merge_context *ctx)
+{
+    if (node == NULL)
+        return NULL;
+
+    if (IsA(node, Var))
+    {
+        Var *oldv = (Var*)node;
+
+        if (!oldv->varlevelsup && oldv->varno == ctx->target_rti)
+        {
+            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 +1272,7 @@ expand_inherited_rtentry(PlannerInfo *root, RangeTblEntry *rte, Index rti)    List
*inhOIDs;   List       *appinfos;    ListCell   *l;
 
+    AppendRelInfo *parent_appinfo = NULL;    /* Does RT entry allow inheritance? */    if (!rte->inh)
@@ -1301,6 +1337,21 @@ 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;
+            break;
+        }
+    }
+    
+    /*     * Must open the parent relation to examine its tupdesc.  We need not lock     * it; we assume the rewriter
alreadydid.     */
 
@@ -1308,6 +1359,14 @@ expand_inherited_rtentry(PlannerInfo *root, RangeTblEntry *rte, Index rti)    /* Scan the
inheritanceset and expand it */    appinfos = NIL;
 
+
+    /*
+     * This rte will lose all children being pulled up later if it has further
+     * parent
+     */
+    if (parent_appinfo)
+        rte->inh = false;
+    foreach(l, inhOIDs)    {        Oid            childOID = lfirst_oid(l);
@@ -1378,6 +1437,28 @@ 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;
+
+            appinfo->parent_relid = parent_appinfo->parent_relid;
+            appinfo->parent_reltype = parent_appinfo->parent_reltype;
+            appinfo->parent_reloid = parent_appinfo->parent_reloid;
+            appinfo->translated_vars = (List*)expression_tree_mutator(
+                (Node*)parent_appinfo->translated_vars,
+                transvars_merge_mutator, &ctx);
+        }
+
+        /*         * Build a PlanRowMark if parent is marked FOR UPDATE/SHARE.         */        if (oldrc)
@@ -1662,7 +1743,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..f69e92b 100644
--- a/src/test/regress/expected/union.out
+++ b/src/test/regress/expected/union.out
@@ -502,9 +502,42 @@ 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 t2_pkey on t2
+         ->  Index Scan using t1_ab_idx on t1 t1_1
+         ->  Index Scan using t1c_expr_idx on t1c
+         ->  Index Scan using t2_pkey on t2 t2_1
+         ->  Index Scan using t2c_pkey on t2c
+(9 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)