On Mon, Feb 03, 2014 at 07:36:22PM +0900, Kyotaro HORIGUCHI wrote:
> > > 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.
The first union branch should return two rows, and the second union branch
should return one row, for a total of three. In any case, I see later in your
mail that you fixed this. The larger point is that this patch has no business
changing the output rows of *any* query. Its goal is to pick a more-efficient
plan for arriving at the same answer. If there's a bug in our current output
for some query, that's a separate discussion from the topic of this thread.
> 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.
> + /*
> + * Appendrels which does whole-row-var
> conversion cannot be
> + * removed. ConvertRowtypeExpr can convert only
> RELs which can
> + * be referred to using relid.
We have parent and child relids, so it is not clear to me how imposing that
restriction helps us. I replaced transvars_merge_mutator() with a call to
adjust_appendrel_attrs(). This reduces code duplication, and it handles
whole-row references. (I don't think the other nodes adjust_appendrel_attrs()
can handle matter to this caller. translated_vars will never contain join
tree nodes, and I doubt it could contain a PlaceHolderVar with phrels
requiring translation.)
The central design question for this patch seems to be how to represent, in
the range table, the fact that we expanded an inheritance parent despite its
children ending up as appendrel children of a freestanding UNION ALL. The v6
patch detaches the original RTE from the join tree and clears its "inh" flag.
This breaks sepgsql_dml_privileges(), which looks for RTE_RELATION with inh =
true and consults selinux concerning every child table. We could certainly
change the way sepgsql discovers inheritance hierarchies, but nothing clearly
better comes to mind. I selected the approach of preserving the RTE's "inh"
flag, removing the AppendRelInfo connecting that RTE to its enclosing UNION
ALL, and creating no AppendRelInfo children for that RTE. An alternative was
to introduce a new RTE flag, say "append". An inheritance parent under a
UNION ALL would have append = false, inh = true; other inheritance parent RTEs
would have append = true, inh = true; an RTE for UNION ALL itself would have
append = true, inh = false.
> > > > > > 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.
> [details]
Interesting. Today, the parent_reltype and child_reltype fields of
AppendRelInfo are either both valid or both invalid. Your v6 patch allowed us
to have a valid child_reltype with an invalid parent_reltype. At the moment,
we can't benefit from just one valid reltype. I restored the old invariant.
If the attached patch version looks reasonable, I will commit it.
Incidentally, I tried adding an assertion that append_rel_list does not show
one appendrel as a direct child of another. The following query, off-topic
for the patch at hand, triggered that assertion:
SELECT 0 FROM (SELECT 0 UNION ALL SELECT 0) t0
UNION ALL
SELECT 0 FROM (SELECT 0 UNION ALL SELECT 0) t0;
--
Noah Misch
EnterpriseDB http://www.enterprisedb.com
diff --git a/src/backend/optimizer/prep/prepunion.c
b/src/backend/optimizer/prep/prepunion.c
index 52dcc72..129354a 100644
--- a/src/backend/optimizer/prep/prepunion.c
+++ b/src/backend/optimizer/prep/prepunion.c
@@ -1223,8 +1223,17 @@ expand_inherited_tables(PlannerInfo *root)
* table, but with inh = false, to represent the parent table in its role
* as a simple member of the inheritance set.
*
+ * Most commonly, the added AppendRelInfo nodes name the original RTE as their
+ * parent_relid, making the original RTE an appendrel parent. An exception
+ * arises when the original RTE is itself an appendrel member, as with
+ * "... UNION ALL SELECT * FROM inhparent" syntax. In such situations,
+ * preserve appendrel flatness by detaching the original RTE's AppendRelInfo
+ * and pointing each new parent_relid to the existing UNION ALL appendrel.
+ * The original RTE remains in the range table for permission-checking
+ * purposes only.
+ *
* A childless table is never considered to be an inheritance set; therefore
- * a parent RTE must always have at least two associated AppendRelInfos.
+ * a parent RTE must never have exactly one associated AppendRelInfo.
*/
static void
expand_inherited_rtentry(PlannerInfo *root, RangeTblEntry *rte, Index rti)
@@ -1237,6 +1246,8 @@ expand_inherited_rtentry(PlannerInfo *root, RangeTblEntry
*rte, Index rti)
List *inhOIDs;
List *appinfos;
ListCell *l;
+ ListCell *prev;
+ AppendRelInfo *parent_appinfo = NULL;
/* Does RT entry allow inheritance? */
if (!rte->inh)
@@ -1300,6 +1311,27 @@ expand_inherited_rtentry(PlannerInfo *root,
RangeTblEntry *rte, Index rti)
if (oldrc)
oldrc->isParent = true;
+ /* Find and detach from an existing UNION ALL parent appendrel, if any.
*/
+ prev = NULL;
+ foreach(l, root->append_rel_list)
+ {
+ AppendRelInfo *appinfo = (AppendRelInfo *) lfirst(l);
+
+ if (appinfo->child_relid == rti)
+ {
+ /*
+ * A PlanRowMark should have blocked pull-up of the
UNION ALL
+ * subquery referencing this inheritance parent table.
+ */
+ Assert(!oldrc);
+
+ parent_appinfo = appinfo;
+ list_delete_cell(root->append_rel_list, l, prev);
+ break;
+ }
+ prev = l;
+ }
+
/*
* Must open the parent relation to examine its tupdesc. We need not
lock
* it; we assume the rewriter already did.
@@ -1361,9 +1393,10 @@ expand_inherited_rtentry(PlannerInfo *root,
RangeTblEntry *rte, Index rti)
appinfos = lappend(appinfos, appinfo);
/*
- * Translate the column permissions bitmaps to the child's
attnums (we
- * have to build the translated_vars list before we can do
this). But
- * if this is the parent table, leave copyObject's result alone.
+ * Translate the column permissions bitmaps to the child's
attnums. We
+ * must do this after building the initial translated_vars and
before
+ * replacing it with translated_vars derived from a parent
appendrel.
+ * But if this is the parent table, leave copyObject's result
alone.
*
* Note: we need to do this even though the executor won't run
any
* permissions checks on the child RTE. The modifiedCols
bitmap may
@@ -1377,6 +1410,30 @@ expand_inherited_rtentry(PlannerInfo *root,
RangeTblEntry *rte, Index rti)
appinfo->translated_vars);
}
+ if (parent_appinfo)
+ {
+ Node *parent_tv = (Node *)
parent_appinfo->translated_vars;
+ Node *child_tv;
+
+ /*
+ * Adjust the inheritance parent table's UNION ALL
translated_vars
+ * to suit this child. Drive it using the appinfo we
constructed
+ * based on the inheritance parent table being the
appendrel
+ * parent; this is like copying a parent targetlist to
a child.
+ */
+ child_tv = adjust_appendrel_attrs(root, parent_tv,
appinfo);
+
+ /*
+ * Adapt appinfo to its final appendrel parent. We
could leave
+ * child_reltype valid, but no code would presently
benefit.
+ */
+ Assert(!OidIsValid(parent_appinfo->parent_reltype));
+ appinfo->parent_relid = parent_appinfo->parent_relid;
+ appinfo->parent_reltype = InvalidOid;
+ appinfo->child_reltype = InvalidOid;
+ appinfo->translated_vars = (List *) child_tv;
+ }
+
/*
* Build a PlanRowMark if parent is marked FOR UPDATE/SHARE.
*/
@@ -1580,8 +1637,10 @@ translate_col_privs(const Bitmapset *parent_privs,
* child rel instead. We also update rtindexes appearing outside Vars,
* such as resultRelation and jointree relids.
*
- * Note: this is only applied after conversion of sublinks to subplans,
- * so we don't need to cope with recursion into sub-queries.
+ * Note: this is applied after conversion of sublinks to subplans. It is
also
+ * applied shortly before that conversion, at which point every sublink is
+ * destined to produce a subplan. Therefore, we don't need to cope with
+ * recursion into sub-queries.
*
* Note: this is not hugely different from what pullup_replace_vars() does;
* maybe we should try to fold the two routines together.
diff --git a/src/include/nodes/relation.h b/src/include/nodes/relation.h
index 8aa40d0..4f738d8 100644
--- a/src/include/nodes/relation.h
+++ b/src/include/nodes/relation.h
@@ -1409,13 +1409,15 @@ typedef struct LateralJoinInfo
* will not have many different append parents, it doesn't seem worthwhile
* to complicate things.
*
- * Note: after completion of the planner prep phase, any given RTE is an
- * append parent having entries in append_rel_list if and only if its
- * "inh" flag is set. We clear "inh" for plain tables that turn out not
- * to have inheritance children, and (in an abuse of the original meaning
- * of the flag) we set "inh" for subquery RTEs that turn out to be
- * flattenable UNION ALL queries. This lets us avoid useless searches
- * of append_rel_list.
+ * Note: after completion of the planner prep phase, every RTE having entries
+ * in append_rel_list has its "inh" flag set. This lets us avoid useless
+ * searches of append_rel_list. We clear "inh" for plain tables that turn out
+ * not to have inheritance children. In an abuse of the original meaning of
+ * the flag, we set "inh" for subquery RTEs that turn out to be flattenable
+ * UNION ALL queries. After attaching inheritance children to an independent
+ * UNION ALL, we leave "inh" set on the original inheritance parent RTE
+ * despite that RTE having no entries in append_rel_list; see
+ * expand_inherited_rtentry().
*
* Note: the data structure assumes that append-rel members are single
* baserels. This is OK for inheritance, but it prevents us from pulling
diff --git a/src/test/regress/expected/union.out
b/src/test/regress/expected/union.out
index 6f9ee5e..24a13bc 100644
--- a/src/test/regress/expected/union.out
+++ b/src/test/regress/expected/union.out
@@ -502,9 +502,56 @@ 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 (b text, a text);
+ALTER TABLE t1c INHERIT t1;
+CREATE TEMP TABLE t2c (primary key (ab)) INHERITS (t2);
+INSERT INTO t1c VALUES ('v', 'w'), ('c', 'd');
+INSERT INTO t2c VALUES ('vw'), ('cd');
+CREATE INDEX t1c_ab_idx on t1c ((a || b));
+set enable_seqscan = on;
+set enable_indexonlyscan = off;
+explain (costs off)
+ SELECT * FROM
+ (SELECT a || b AS ab, t1 FROM t1
+ UNION ALL
+ SELECT ab, null FROM t2) t
+ ORDER BY 1 LIMIT 10;
+ QUERY PLAN
+------------------------------------------------
+ Limit
+ -> Merge Append
+ Sort Key: ((t1.a || t1.b))
+ -> Index Scan using t1_ab_idx on t1
+ -> Index Scan using t1c_ab_idx on t1c
+ -> Index Scan using t2_pkey on t2
+ -> Index Scan using t2c_pkey on t2c
+(7 rows)
+
+ SELECT * FROM
+ (SELECT a || b AS ab, t1 FROM t1
+ UNION ALL
+ SELECT ab, null FROM t2) t
+ ORDER BY 1,2 LIMIT 10;
+ ab | t1
+----+-------
+ ab | (a,b)
+ ab |
+ cd |
+ dc | (d,c)
+ vw |
+ wv | (w,v)
+ xy | (x,y)
+ xy |
+(8 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..13f29ab 100644
--- a/src/test/regress/sql/union.sql
+++ b/src/test/regress/sql/union.sql
@@ -198,9 +198,37 @@ 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 (b text, a text);
+ALTER TABLE t1c INHERIT t1;
+CREATE TEMP TABLE t2c (primary key (ab)) INHERITS (t2);
+INSERT INTO t1c VALUES ('v', 'w'), ('c', 'd');
+INSERT INTO t2c VALUES ('vw'), ('cd');
+CREATE INDEX t1c_ab_idx on t1c ((a || b));
+set enable_seqscan = on;
+set enable_indexonlyscan = off;
+
+explain (costs off)
+ SELECT * FROM
+ (SELECT a || b AS ab, t1 FROM t1
+ UNION ALL
+ SELECT ab, null FROM t2) t
+ ORDER BY 1 LIMIT 10;
+
+ SELECT * FROM
+ (SELECT a || b AS ab, t1 FROM t1
+ UNION ALL
+ SELECT ab, null FROM t2) t
+ ORDER BY 1,2 LIMIT 10;
+
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
