In
http://www.postgresql.org/message-id/cak_s-g3-fwveer1c0idvtz0745-7ryifi8whbzcnmsn+hwc...@mail.gmail.com
it's pointed out that commit 2ffa740b was a few bricks shy of a load,
because it failed to cope with the possibility of a joinaliasvars item
containing an implicit coercion. That's not too hard to fix by adding a
strip_implicit_coercions() call, but while testing it I realized that
there's a second bug in the same place: the code also fails to cope with
a Const arising from a dropped input-relation column. (The comment
claiming that this can't happen is flat wrong, because we *have* passed
the view query through AcquireRewriteLocks().) I fixed that too, and
improved the comment in parsenodes.h that led me to overlook implicit
coercions in the first place, as per the attached WIP patch.
I then went looking for other places that might be assuming too much
about what is in joinaliasvars lists, and found several pre-existing
bugs :-(. The nastiest one is in flatten_join_alias_vars_mutator's code
to expand a whole-row reference, which supposes that if it finds a Const
item then that must represent a dropped column. That would be true in
the parser or rewriter, but at this point in planning we have already
done subquery pullup, which means we could find anything including a
Const there. The code would thus mistake a pulled-up constant subquery
output for a dropped column. An example in the regression database is
explain verbose select j from (int4_tbl join (select q1 as f1, q2 as z, 42 as c
from int8_tbl) ss using(f1)) j;
QUERY PLAN
---------------------------------------------------------------------------
Hash Join (cost=1.11..2.23 rows=5 width=16)
Output: ROW(int8_tbl.q1, int8_tbl.q2)
Hash Cond: (int8_tbl.q1 = int4_tbl.f1)
-> Seq Scan on public.int8_tbl (cost=0.00..1.05 rows=5 width=16)
Output: int8_tbl.q1, int8_tbl.q2
-> Hash (cost=1.05..1.05 rows=5 width=4)
Output: int4_tbl.f1
-> Seq Scan on public.int4_tbl (cost=0.00..1.05 rows=5 width=4)
Output: int4_tbl.f1
(9 rows)
The 42 has disappeared entirely from the ROW() construct :-(. This can
be reproduced in all active branches.
After some reflection I think that the best fix is to redefine
AcquireRewriteLocks' processing of dropped columns so that it puts an
actual null pointer, not a consed-up Const, into the joinaliasvars list
item. This is reliably distinguishable from anything we might pull up
from a subquery output, and it doesn't really lose information since the
Const was completely phony anyway. (I think the original design
envisioned having the Const carrying column datatype info, but we
abandoned that idea upon realizing that the dropped column might be of a
dropped type. The phony Const is currently always of type INT4.)
This definitional change will not affect any rules-on-disk since the
dropped-column substitution is only done on rule trees when they are
loaded back into memory.
A risk we'd be taking with this change is that any extension code that
looks at post-rewriter joinaliasvars lists might be confused. However,
I'm not sure why extension code would be looking at those. In any case,
I can't see a different back-patchable change that would reduce such a
risk; we have to change the representation *somehow* if we're going to
distinguish these cases correctly.
Thoughts?
regards, tom lane
diff --git a/src/backend/utils/adt/ruleutils.c b/src/backend/utils/adt/ruleutils.c
index 40b565a11d6ddf40b3cd8e692e625efe1f6dc3a7..3292278774f15851f3ad7ae000139278c3f94fd8 100644
*** a/src/backend/utils/adt/ruleutils.c
--- b/src/backend/utils/adt/ruleutils.c
*************** typedef struct
*** 235,240 ****
--- 235,241 ----
* child RTE's attno and rightattnos[i] is zero; and conversely for a
* column of the right child. But for merged columns produced by JOIN
* USING/NATURAL JOIN, both leftattnos[i] and rightattnos[i] are nonzero.
+ * Also, if the column has been dropped, both are zero.
*
* If it's a JOIN USING, usingNames holds the alias names selected for the
* merged columns (these might be different from the original USING list,
*************** set_join_column_names(deparse_namespace
*** 3053,3058 ****
--- 3054,3066 ----
char *colname = colinfo->colnames[i];
char *real_colname;
+ /* Ignore dropped column (only possible for non-merged column) */
+ if (colinfo->leftattnos[i] == 0 && colinfo->rightattnos[i] == 0)
+ {
+ Assert(colname == NULL);
+ continue;
+ }
+
/* Get the child column name */
if (colinfo->leftattnos[i] > 0)
real_colname = leftcolinfo->colnames[colinfo->leftattnos[i] - 1];
*************** set_join_column_names(deparse_namespace
*** 3061,3075 ****
else
{
/* We're joining system columns --- use eref name */
! real_colname = (char *) list_nth(rte->eref->colnames, i);
! }
!
! /* Ignore dropped columns (only possible for non-merged column) */
! if (real_colname == NULL)
! {
! Assert(colname == NULL);
! continue;
}
/* In an unnamed join, just report child column names as-is */
if (rte->alias == NULL)
--- 3069,3077 ----
else
{
/* We're joining system columns --- use eref name */
! real_colname = strVal(list_nth(rte->eref->colnames, i));
}
+ Assert(real_colname != NULL);
/* In an unnamed join, just report child column names as-is */
if (rte->alias == NULL)
*************** identify_join_columns(JoinExpr *j, Range
*** 3402,3407 ****
--- 3404,3412 ----
{
Var *aliasvar = (Var *) lfirst(lc);
+ /* get rid of any implicit coercion above the Var */
+ aliasvar = (Var *) strip_implicit_coercions((Node *) aliasvar);
+
if (IsA(aliasvar, Var))
{
Assert(aliasvar->varlevelsup == 0);
*************** identify_join_columns(JoinExpr *j, Range
*** 3421,3436 ****
* let the code below identify the merged columns.
*/
}
! else
{
! /*
! * Although NULL constants can appear in joinaliasvars lists
! * during planning, we shouldn't see any here, since the Query
! * tree hasn't been through AcquireRewriteLocks().
! */
elog(ERROR, "unrecognized node type in join alias vars: %d",
(int) nodeTag(aliasvar));
- }
i++;
}
--- 3426,3438 ----
* let the code below identify the merged columns.
*/
}
! else if (IsA(aliasvar, Const))
{
! /* It's a dropped column; nothing to do here */
! }
! else
elog(ERROR, "unrecognized node type in join alias vars: %d",
(int) nodeTag(aliasvar));
i++;
}
diff --git a/src/include/nodes/parsenodes.h b/src/include/nodes/parsenodes.h
index 9415e2c636eab741e2216e6c23dc929e7f4f9494..ce8007b950e3c629d9d3c83509cd030a600d8e63 100644
*** a/src/include/nodes/parsenodes.h
--- b/src/include/nodes/parsenodes.h
*************** typedef struct RangeTblEntry
*** 731,743 ****
/*
* Fields valid for a join RTE (else NULL/zero):
*
! * joinaliasvars is a list of Vars or COALESCE expressions corresponding
! * to the columns of the join result. An alias Var referencing column K
! * of the join result can be replaced by the K'th element of joinaliasvars
! * --- but to simplify the task of reverse-listing aliases correctly, we
! * do not do that until planning time. In a Query loaded from a stored
! * rule, it is also possible for joinaliasvars items to be NULL Consts,
! * denoting columns dropped since the rule was made.
*/
JoinType jointype; /* type of join */
List *joinaliasvars; /* list of alias-var expansions */
--- 731,748 ----
/*
* Fields valid for a join RTE (else NULL/zero):
*
! * joinaliasvars is a list of (usually) Vars corresponding to the columns
! * of the join result. An alias Var referencing column K of the join
! * result can be replaced by the K'th element of joinaliasvars --- but to
! * simplify the task of reverse-listing aliases correctly, we do not do
! * that until planning time. In detail: an element of joinaliasvars can
! * be a Var of one of the join's input relations, or such a Var with an
! * implicit coercion to the join's output column type, or a COALESCE
! * expression containing the two input column Vars (possibly coerced).
! * Within a Query loaded from a stored rule, it is also possible for
! * joinaliasvars items to be NULL Consts, denoting columns dropped since
! * the rule was made. Also, once planning begins, joinaliasvars items can
! * be almost anything, as a result of subquery-flattening substitutions.
*/
JoinType jointype; /* type of join */
List *joinaliasvars; /* list of alias-var expansions */
diff --git a/src/test/regress/expected/create_view.out b/src/test/regress/expected/create_view.out
index 4fa774950345808d9455abf06fddbed15933dfaf..8b451429674a6153dcf914ab2753787365cd6cb8 100644
*** a/src/test/regress/expected/create_view.out
--- b/src/test/regress/expected/create_view.out
*************** select pg_get_viewdef('vv4', true);
*** 1243,1248 ****
--- 1243,1275 ----
FULL JOIN tt8 tt8y(x_1, z, z2) USING (x_1);
(1 row)
+ --
+ -- Also check dropping a column that existed when the view was made
+ --
+ create table tt9 (x int, xx int, y int);
+ create table tt10 (x int, z int);
+ create view vv5 as select x,y,z from tt9 join tt10 using(x);
+ select pg_get_viewdef('vv5', true);
+ pg_get_viewdef
+ -------------------------
+ SELECT tt9.x, +
+ tt9.y, +
+ tt10.z +
+ FROM tt9 +
+ JOIN tt10 USING (x);
+ (1 row)
+
+ alter table tt9 drop column xx;
+ select pg_get_viewdef('vv5', true);
+ pg_get_viewdef
+ -------------------------
+ SELECT tt9.x, +
+ tt9.y, +
+ tt10.z +
+ FROM tt9 +
+ JOIN tt10 USING (x);
+ (1 row)
+
-- clean up all the random objects we made above
set client_min_messages = warning;
DROP SCHEMA temp_view_test CASCADE;
diff --git a/src/test/regress/sql/create_view.sql b/src/test/regress/sql/create_view.sql
index 3d85d9cfdc50b454fb38d8a742770eda1e119a41..4fbd5a5e6f8baa3d41f1a201fbb99ad4bc3bf3da 100644
*** a/src/test/regress/sql/create_view.sql
--- b/src/test/regress/sql/create_view.sql
*************** select pg_get_viewdef('vv2', true);
*** 389,394 ****
--- 389,409 ----
select pg_get_viewdef('vv3', true);
select pg_get_viewdef('vv4', true);
+ --
+ -- Also check dropping a column that existed when the view was made
+ --
+
+ create table tt9 (x int, xx int, y int);
+ create table tt10 (x int, z int);
+
+ create view vv5 as select x,y,z from tt9 join tt10 using(x);
+
+ select pg_get_viewdef('vv5', true);
+
+ alter table tt9 drop column xx;
+
+ select pg_get_viewdef('vv5', true);
+
-- clean up all the random objects we made above
set client_min_messages = warning;
DROP SCHEMA temp_view_test CASCADE;
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
