Tom Lane wrote:
> Alvaro Herrera <alvhe...@2ndquadrant.com> writes:
> > Alvaro Herrera wrote:
> >> Here's a first attempt at fixing this. It makes the test pass, but I
> >> have the feeling that more complex ones might need more work.
>
> > Here's another one with three main differences:
>
> Hmm. The bespoke code for constructing the attno map bothers me;
> surely there is existing code that does that? If not, it'd still
> make more sense to factor it out, I think, because there will be
> other needs for it in future.
There isn't any that I could find -- all the existing callers of
map_variable_attnos build their map in other ways (while walking an
attribute array at construction time).
So I did as you suggest, 'cause it sounds like a good idea, but the
problem crops up of where to put the new function. The obvious
candidate is rewriteManip.c next to map_variable_attnos itself, but the
include creep is a bit bothersome -- maybe it indicates that the new
function should be elsewhere. But then, the whole of rewriteManip seems
not terribly well delimited to the rewriter itself but just an assorted
collection of walkers, mutators, and similar utilities used by code all
over the place, so perhaps this is not a problem.
I also modified the algorithm to use the relcache instead of walking the
child's attribute list for each parent attribute (that was silly).
Here's the new version.
> Otherwise, this seems sound in terms of fixing the observed problem,
> but what are the implications for event triggers exactly? Does a
> trigger see only the original expression, or only the modified expression,
> or ???
My rationale when writing the event trigger code was that each command
would only be published once, for the parent table, not recursively for
each child. So only the original expression should be seen. I have not
yet verified the actual behavior in the differing attnums case. One
problem at a time ...
--
Álvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index e217f57..2977b59 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -7999,12 +7999,68 @@ ATPrepAlterColumnType(List **wqueue,
ReleaseSysCache(tuple);
/*
- * The recursion case is handled by ATSimpleRecursion. However, if we
are
- * told not to recurse, there had better not be any child tables; else
the
- * alter would put them out of step.
+ * Recurse manually by queueing a new command for each child, if
+ * necessary. We cannot apply ATSimpleRecursion here because we need to
+ * remap attribute numbers in the USING expression, if any.
+ *
+ * If we are told not to recurse, there had better not be any child
+ * tables; else the alter would put them out of step.
*/
if (recurse)
- ATSimpleRecursion(wqueue, rel, cmd, recurse, lockmode);
+ {
+ Oid relid = RelationGetRelid(rel);
+ ListCell *child;
+ List *children;
+
+ children = find_all_inheritors(relid, lockmode, NULL);
+
+ /*
+ * find_all_inheritors does the recursive search of the
inheritance
+ * hierarchy, so all we have to do is process all of the relids
in the
+ * list that it returns.
+ */
+ foreach(child, children)
+ {
+ Oid childrelid = lfirst_oid(child);
+ Relation childrel;
+
+ if (childrelid == relid)
+ continue;
+
+ /* find_all_inheritors already got lock */
+ childrel = relation_open(childrelid, NoLock);
+ CheckTableNotInUse(childrel, "ALTER TABLE");
+
+ /*
+ * Remap the attribute numbers. If no USING expression
was
+ * specified, there is no need for this step.
+ */
+ if (def->cooked_default)
+ {
+ AttrNumber *attmap;
+ int maplength;
+ bool found_whole_row;
+
+ /* create a copy to scribble on */
+ cmd = copyObject(cmd);
+
+ attmap = build_attno_map(rel, childrel,
&maplength);
+ ((ColumnDef *) cmd->def)->cooked_default =
+ map_variable_attnos(def->cooked_default,
+
1, 0,
+
attmap, maplength,
+
&found_whole_row);
+ if (found_whole_row)
+ ereport(ERROR,
+
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+ errmsg("cannot convert
whole-row table reference"),
+ errdetail("USING
expression contains a whole-row table reference.")));
+ pfree(attmap);
+ }
+ ATPrepCmd(wqueue, childrel, cmd, false, true, lockmode);
+ relation_close(childrel, NoLock);
+ }
+ }
else if (!recursing &&
find_inheritance_children(RelationGetRelid(rel),
NoLock) != NIL)
ereport(ERROR,
diff --git a/src/backend/rewrite/rewriteManip.c
b/src/backend/rewrite/rewriteManip.c
index 6f22168..7c778fa 100644
--- a/src/backend/rewrite/rewriteManip.c
+++ b/src/backend/rewrite/rewriteManip.c
@@ -22,6 +22,8 @@
#include "parser/parse_relation.h"
#include "parser/parsetree.h"
#include "rewrite/rewriteManip.h"
+#include "utils/lsyscache.h"
+#include "utils/rel.h"
typedef struct
@@ -1305,6 +1307,35 @@ map_variable_attnos(Node *node,
0);
}
+/*
+ * Given a parent relation rel1 and a descendant rel2, return a palloc'd map
+ * for map_variable_attnos to map attributes of the child table onto the
+ * parent's.
+ */
+AttrNumber *
+build_attno_map(Relation rel1, Relation rel2, int *maplength)
+{
+ AttrNumber *attmap;
+ AttrNumber anum1;
+ TupleDesc desc1 = RelationGetDescr(rel1);
+ Oid rel2id = RelationGetRelid(rel2);
+
+ attmap = (AttrNumber *) palloc(sizeof(AttrNumber) * desc1->natts);
+ *maplength = desc1->natts;
+
+ for (anum1 = 1; anum1 <= desc1->natts; anum1++)
+ {
+ AttrNumber anum2;
+
+ anum2 = get_attnum(rel2id, NameStr(desc1->attrs[anum1 -
1]->attname));
+ if (anum2 == InvalidAttrNumber)
+ elog(ERROR, "column \"%s\" not found",
+ NameStr(desc1->attrs[anum1 - 1]->attname));
+ attmap[anum1 - 1] = anum2;
+ }
+
+ return attmap;
+}
/*
* ReplaceVarsFromTargetList - replace Vars with items from a targetlist
diff --git a/src/include/rewrite/rewriteManip.h
b/src/include/rewrite/rewriteManip.h
index bb56fa0..6ec3b22 100644
--- a/src/include/rewrite/rewriteManip.h
+++ b/src/include/rewrite/rewriteManip.h
@@ -15,6 +15,7 @@
#define REWRITEMANIP_H
#include "nodes/parsenodes.h"
+#include "utils/relcache.h"
typedef struct replace_rte_variables_context replace_rte_variables_context;
@@ -73,6 +74,7 @@ extern Node *map_variable_attnos(Node *node,
int target_varno, int sublevels_up,
const AttrNumber *attno_map, int
map_length,
bool *found_whole_row);
+extern AttrNumber *build_attno_map(Relation rel1, Relation rel2, int
*maplength);
extern Node *ReplaceVarsFromTargetList(Node *node,
int target_varno, int
sublevels_up,
diff --git a/src/test/regress/expected/alter_table.out
b/src/test/regress/expected/alter_table.out
index de3c69a..1125517 100644
--- a/src/test/regress/expected/alter_table.out
+++ b/src/test/regress/expected/alter_table.out
@@ -2018,6 +2018,28 @@ select relname, conname, coninhcount, conislocal,
connoinherit
test_inh_check_child | test_inh_check_a_check | 1 | f | f
(6 rows)
+-- ALTER COLUMN TYPE with different schema in children
+-- Bug at https://postgr.es/m/20170102225618.ga10...@telsasoft.com
+CREATE TABLE test_type_diff (f1 int);
+CREATE TABLE test_type_diff_c (extra smallint) INHERITS (test_type_diff);
+ALTER TABLE test_type_diff ADD COLUMN f2 int;
+INSERT INTO test_type_diff_c VALUES (1, 2, 3);
+ALTER TABLE test_type_diff ALTER COLUMN f2 TYPE bigint USING f2::bigint;
+CREATE TABLE test_type_diff2 (int_two int2, int_four int4, int_eight int8);
+CREATE TABLE test_type_diff2_c1 (int_four int4, int_eight int8, int_two int2);
+CREATE TABLE test_type_diff2_c2 (int_eight int8, int_two int2, int_four int4);
+CREATE TABLE test_type_diff2_c3 (int_two int2, int_four int4, int_eight int8);
+ALTER TABLE test_type_diff2_c1 INHERIT test_type_diff2;
+ALTER TABLE test_type_diff2_c2 INHERIT test_type_diff2;
+ALTER TABLE test_type_diff2_c3 INHERIT test_type_diff2;
+INSERT INTO test_type_diff2_c1 VALUES (1, 2, 3);
+INSERT INTO test_type_diff2_c2 VALUES (4, 5, 6);
+INSERT INTO test_type_diff2_c3 VALUES (7, 8, 9);
+ALTER TABLE test_type_diff2 ALTER COLUMN int_four TYPE int8 USING
int_four::int8;
+-- whole-row references are disallowed
+ALTER TABLE test_type_diff2 ALTER COLUMN int_four TYPE int4 USING
(pg_column_size(test_type_diff2));
+ERROR: cannot convert whole-row table reference
+DETAIL: USING expression contains a whole-row table reference.
-- check for rollback of ANALYZE corrupting table property flags (bug #11638)
CREATE TABLE check_fk_presence_1 (id int PRIMARY KEY, t text);
CREATE TABLE check_fk_presence_2 (id int REFERENCES check_fk_presence_1, t
text);
diff --git a/src/test/regress/sql/alter_table.sql
b/src/test/regress/sql/alter_table.sql
index 1459311..b593163 100644
--- a/src/test/regress/sql/alter_table.sql
+++ b/src/test/regress/sql/alter_table.sql
@@ -1321,6 +1321,28 @@ select relname, conname, coninhcount, conislocal,
connoinherit
where relname like 'test_inh_check%' and c.conrelid = r.oid
order by 1, 2;
+-- ALTER COLUMN TYPE with different schema in children
+-- Bug at https://postgr.es/m/20170102225618.ga10...@telsasoft.com
+CREATE TABLE test_type_diff (f1 int);
+CREATE TABLE test_type_diff_c (extra smallint) INHERITS (test_type_diff);
+ALTER TABLE test_type_diff ADD COLUMN f2 int;
+INSERT INTO test_type_diff_c VALUES (1, 2, 3);
+ALTER TABLE test_type_diff ALTER COLUMN f2 TYPE bigint USING f2::bigint;
+
+CREATE TABLE test_type_diff2 (int_two int2, int_four int4, int_eight int8);
+CREATE TABLE test_type_diff2_c1 (int_four int4, int_eight int8, int_two int2);
+CREATE TABLE test_type_diff2_c2 (int_eight int8, int_two int2, int_four int4);
+CREATE TABLE test_type_diff2_c3 (int_two int2, int_four int4, int_eight int8);
+ALTER TABLE test_type_diff2_c1 INHERIT test_type_diff2;
+ALTER TABLE test_type_diff2_c2 INHERIT test_type_diff2;
+ALTER TABLE test_type_diff2_c3 INHERIT test_type_diff2;
+INSERT INTO test_type_diff2_c1 VALUES (1, 2, 3);
+INSERT INTO test_type_diff2_c2 VALUES (4, 5, 6);
+INSERT INTO test_type_diff2_c3 VALUES (7, 8, 9);
+ALTER TABLE test_type_diff2 ALTER COLUMN int_four TYPE int8 USING
int_four::int8;
+-- whole-row references are disallowed
+ALTER TABLE test_type_diff2 ALTER COLUMN int_four TYPE int4 USING
(pg_column_size(test_type_diff2));
+
-- check for rollback of ANALYZE corrupting table property flags (bug #11638)
CREATE TABLE check_fk_presence_1 (id int PRIMARY KEY, t text);
CREATE TABLE check_fk_presence_2 (id int REFERENCES check_fk_presence_1, t
text);
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
