Here's v2 based on your feedback.
I pruned test coverage down to just the highlights. By the end of patch series,
the net change becomes +67 to alter_table.sql and +111 to alter_table.out. The
alter_table.out delta is larger in this patch (+150), because the optimizations
don't yet apply and more objects are reported as rebuilt.
If this looks sane, I'll rebase the rest of the patches accordingly.
On Tue, Jan 11, 2011 at 09:41:35PM -0500, Robert Haas wrote:
> On Tue, Jan 11, 2011 at 7:25 AM, Noah Misch <n...@leadboat.com> wrote:
> I'm wondering if we should consider moving this call to index_build()
> so that it appears everywhere that we build an index rather than just
> in ALTER TABLE, and saying something like:
>
> building index "%s" on table "%s"
Done.
I also fixed the leading capital letter on the other new messages.
> > The theoretical basis is that users can do little to directly change the
> > need to
> > rebuild a TOAST index. ?We'll rebuild the TOAST index if and only if we
> > rewrote
> > the table. ?The practical basis is that the TOAST relation names contain
> > parent
> > relation OIDs, so we don't want them mentioned in regression test output.
>
> Perhaps in this case we could write:
>
> building TOAST index for table "%s"
>
> There's limited need for users to know the actual name of the TOAST
> table, but I think it's better to log a line for all the actions we're
> performing or none of them, rather than some but not others.
That was a good idea, but the implementation is awkward. index_build has the
TOAST table and index relations, and there's no good way to find the parent
table from either. No index covers pg_class.reltoastrelid, so scanning by that
would be a bad idea. Autovacuum solves this by building its own hash table with
the mapping; that wouldn't fit well here. We could parse the parent OID out of
the TOAST name (heh, heh). I suppose we could pass the parent relation from
create_toast_table down through index_create to index_build. Currently,
index_create knows nothing of the fact that it's making a TOAST index, and
changing that to improve this messages seems a bit excessive. Thoughts?
For this version, I've kept the DEBUG1/DEBUG2 split by TOAST.
diff --git a/src/backend/catalog/index.c b/src/backend/catalog/index.c
index 5790f87..8ff6927 100644
*** a/src/backend/catalog/index.c
--- b/src/backend/catalog/index.c
***************
*** 1420,1425 **** index_build(Relation heapRelation,
--- 1420,1430 ----
procedure = indexRelation->rd_am->ambuild;
Assert(RegProcedureIsValid(procedure));
+ ereport(IsToastRelation(indexRelation) ? DEBUG2 : DEBUG1,
+ (errmsg("building index \"%s\" on table \"%s\"",
+ RelationGetRelationName(indexRelation),
+
RelationGetRelationName(heapRelation))));
+
/*
* Switch to the table owner's userid, so that any index functions are
run
* as that user. Also lock down security-restricted operations and
diff --git a/src/backend/commands/index f3bd565..e3921e4 100644
*** a/src/backend/commands/tablecmds.c
--- b/src/backend/commands/tablecmds.c
***************
*** 3443,3448 **** ATRewriteTable(AlteredTableInfo *tab, Oid OIDNewHeap,
LOCKMODE lockmode)
--- 3443,3457 ----
List *dropped_attrs = NIL;
ListCell *lc;
+ if (newrel)
+ ereport(DEBUG1,
+ (errmsg("rewriting table \"%s\"",
+
RelationGetRelationName(oldrel))));
+ else
+ ereport(DEBUG1,
+ (errmsg("verifying table \"%s\"",
+
RelationGetRelationName(oldrel))));
+
econtext = GetPerTupleExprContext(estate);
/*
***************
*** 3836,3841 **** ATTypedTableRecursion(List **wqueue, Relation rel,
AlterTableCmd *cmd,
--- 3845,3854 ----
* Eventually, we'd like to propagate the check or rewrite operation
* into other such tables, but for now, just error out if we find any.
*
+ * Table, NOT NULL and DEFAULT constraints and the "oid" system column do
+ * not (currently) follow the row type, so they require no attention here.
+ * The non-propagation of DEFAULT and NOT NULL make ADD COLUMN safe, too.
+ *
* Caller should provide either a table name or a type name (not both) to
* report in the error message, if any.
*
***************
*** 5789,5794 **** validateForeignKeyConstraint(Constraint *fkconstraint,
--- 5802,5811 ----
HeapTuple tuple;
Trigger trig;
+ ereport(DEBUG1,
+ (errmsg("validating foreign key constraint \"%s\"",
+ fkconstraint->conname)));
+
/*
* Build a trigger call structure; we'll need it either way.
*/
diff --git a/src/test/regress/GNUmakefiindex 15b9ec4..c33ecb9 100644
diff --git a/src/test/regress/expecindex 3d126bb..4002f7f 100644
*** a/src/test/regress/expected/alter_table.out
--- b/src/test/regress/expected/alter_table.out
***************
*** 1477,1482 **** create table tab1 (a int, b text);
--- 1477,1632 ----
create table tab2 (x int, y tab1);
alter table tab1 alter column b type varchar; -- fails
ERROR: cannot alter table "tab1" because column "tab2"."y" uses its rowtype
+ alter table tab1 add check (b <> 'foo');
+ alter table tab1 add c int not null;
+ alter table tab1 add d int not null default 1; -- fails
+ ERROR: cannot alter table "tab1" because column "tab2"."y" uses its rowtype
+ alter table tab1 drop a;
+ alter table tab1 set with oids; -- fails
+ ERROR: cannot alter table "tab1" because column "tab2"."y" uses its rowtype
+ --
+ -- ALTER COLUMN ... SET DATA TYPE optimizations
+ --
+ SET client_min_messages = debug1; -- Track rewrites.
+ -- Model a type change that throws the semantics of dependent expressions.
+ CREATE DOMAIN trickint AS int;
+ CREATE FUNCTION touchy_f(trickint) RETURNS int4 LANGUAGE sql AS 'SELECT
100';
+ CREATE FUNCTION touchy_f(int4) RETURNS int4 LANGUAGE sql AS
'SELECT $1';
+ CREATE DOMAIN lendom AS varchar(8);
+ CREATE DOMAIN checkdom AS text CHECK (VALUE LIKE '<%');
+ CREATE TABLE parent (keycol numeric PRIMARY KEY);
+ NOTICE: CREATE TABLE / PRIMARY KEY will create implicit index "parent_pkey"
for table "parent"
+ DEBUG: building index "parent_pkey" on table "parent"
+ INSERT INTO parent VALUES (0.12), (1.12);
+ CREATE TABLE t (
+ integral int4 NOT NULL,
+ rational numeric(9,4) UNIQUE NOT NULL REFERENCES
parent,
+ string varchar(4) NOT NULL,
+ strarr varchar(2)[] NOT NULL,
+ CHECK (touchy_f(integral) < 10),
+ EXCLUDE (integral WITH =)
+ );
+ NOTICE: CREATE TABLE / UNIQUE will create implicit index "t_rational_key"
for table "t"
+ DEBUG: building index "t_rational_key" on table "t"
+ NOTICE: CREATE TABLE / EXCLUDE will create implicit index "t_integral_excl"
for table "t"
+ DEBUG: building index "t_integral_excl" on table "t"
+ CREATE INDEX ON t USING gin (strarr);
+ DEBUG: building index "t_strarr_idx" on table "t"
+ INSERT INTO t VALUES (1, 0.12, '<a/>', '{ab,cd}'), (2, 1.12, '<b/>',
'{ef,gh}');
+ -- Comments "rewrite", "verify" and "noop" signify whether ATRewriteTables
+ -- rewrites, scans or does nothing to the table proper. An "-e" suffix
denotes
+ -- an error outcome.
+ ALTER TABLE t ALTER integral TYPE trickint;
-- verify-e
+ DEBUG: rewriting table "t"
+ ERROR: check constraint "t_integral_check" is violated by some row
+ ALTER TABLE t DROP CONSTRAINT t_integral_check;
+ ALTER TABLE t ALTER integral TYPE abstime USING integral::abstime; -- noop
+ DEBUG: rewriting table "t"
+ DEBUG: building index "t_rational_key" on table "t"
+ DEBUG: building index "t_strarr_idx" on table "t"
+ DEBUG: building index "t_integral_excl" on table "t"
+ ALTER TABLE t ALTER integral TYPE oid USING integral::int4;
-- noop
+ DEBUG: rewriting table "t"
+ DEBUG: building index "t_rational_key" on table "t"
+ DEBUG: building index "t_strarr_idx" on table "t"
+ DEBUG: building index "t_integral_excl" on table "t"
+ ALTER TABLE t ALTER integral TYPE regtype;
-- noop
+ DEBUG: rewriting table "t"
+ DEBUG: building index "t_rational_key" on table "t"
+ DEBUG: building index "t_strarr_idx" on table "t"
+ DEBUG: building index "t_integral_excl" on table "t"
+ ALTER TABLE t ALTER rational TYPE numeric(7,4);
-- verify
+ DEBUG: rewriting table "t"
+ DEBUG: building index "t_strarr_idx" on table "t"
+ DEBUG: building index "t_integral_excl" on table "t"
+ DEBUG: building index "t_rational_key" on table "t"
+ DEBUG: validating foreign key constraint "t_rational_fkey"
+ ALTER TABLE t ALTER rational TYPE numeric(8,4);
-- noop
+ DEBUG: rewriting table "t"
+ DEBUG: building index "t_strarr_idx" on table "t"
+ DEBUG: building index "t_integral_excl" on table "t"
+ DEBUG: building index "t_rational_key" on table "t"
+ DEBUG: validating foreign key constraint "t_rational_fkey"
+ ALTER TABLE t ALTER rational TYPE numeric(8,1);
-- rewrite-e
+ DEBUG: rewriting table "t"
+ DEBUG: building index "t_strarr_idx" on table "t"
+ DEBUG: building index "t_integral_excl" on table "t"
+ DEBUG: building index "t_rational_key" on table "t"
+ DEBUG: validating foreign key constraint "t_rational_fkey"
+ ERROR: insert or update on table "t" violates foreign key constraint
"t_rational_fkey"
+ DETAIL: Key (rational)=(0.1) is not present in table "parent".
+ ALTER TABLE t ALTER string TYPE varchar(6);
-- noop
+ DEBUG: rewriting table "t"
+ DEBUG: building index "t_strarr_idx" on table "t"
+ DEBUG: building index "t_integral_excl" on table "t"
+ DEBUG: building index "t_rational_key" on table "t"
+ ALTER TABLE t ALTER string TYPE lendom;
-- noop
+ DEBUG: rewriting table "t"
+ DEBUG: building index "t_strarr_idx" on table "t"
+ DEBUG: building index "t_integral_excl" on table "t"
+ DEBUG: building index "t_rational_key" on table "t"
+ ALTER TABLE t ALTER string TYPE xml USING string::xml;
-- verify
+ DEBUG: rewriting table "t"
+ DEBUG: building index "t_strarr_idx" on table "t"
+ DEBUG: building index "t_integral_excl" on table "t"
+ DEBUG: building index "t_rational_key" on table "t"
+ ALTER TABLE t ALTER string TYPE checkdom;
-- verify
+ DEBUG: rewriting table "t"
+ DEBUG: building index "t_strarr_idx" on table "t"
+ DEBUG: building index "t_integral_excl" on table "t"
+ DEBUG: building index "t_rational_key" on table "t"
+ ALTER TABLE t ALTER string TYPE text USING 'foo'::varchar;
-- rewrite
+ DEBUG: rewriting table "t"
+ DEBUG: building index "t_strarr_idx" on table "t"
+ DEBUG: building index "t_integral_excl" on table "t"
+ DEBUG: building index "t_rational_key" on table "t"
+ ALTER TABLE t ALTER strarr TYPE varchar(4)[];
-- noop
+ DEBUG: rewriting table "t"
+ DEBUG: building index "t_integral_excl" on table "t"
+ DEBUG: building index "t_rational_key" on table "t"
+ DEBUG: building index "t_strarr_idx" on table "t"
+ ALTER TABLE t ADD CONSTRAINT u0 UNIQUE (integral), -- build index exactly once
+ ALTER integral TYPE int8;
-- rewrite
+ NOTICE: ALTER TABLE / ADD UNIQUE will create implicit index "u0" for table
"t"
+ DEBUG: rewriting table "t"
+ DEBUG: building index "t_rational_key" on table "t"
+ DEBUG: building index "t_strarr_idx" on table "t"
+ DEBUG: building index "t_integral_excl" on table "t"
+ DEBUG: building index "u0" on table "t"
+ -- Data and catalog end state. We omit the columns that bear unstable OIDs.
+ SELECT * FROM t ORDER BY 1;
+ integral | rational | string | strarr
+ ----------+----------+--------+---------
+ 1 | 0.1200 | foo | {ab,cd}
+ 2 | 1.1200 | foo | {ef,gh}
+ (2 rows)
+
+ SELECT relname, indclass FROM pg_index JOIN pg_class c ON c.oid = indexrelid
+ WHERE indrelid = 't'::regclass ORDER BY 1;
+ relname | indclass
+ -----------------+----------
+ t_integral_excl | 10029
+ t_rational_key | 10037
+ t_strarr_idx | 10103
+ u0 | 10029
+ (4 rows)
+
+ SELECT relname, attname, atttypid, atttypmod
+ FROM pg_attribute JOIN pg_class c ON c.oid = attrelid
+ WHERE attnum > 0 AND
+ attrelid IN (SELECT indexrelid FROM pg_index WHERE indrelid =
't'::regclass)
+ ORDER BY 1, 2;
+ relname | attname | atttypid | atttypmod
+ -----------------+----------+----------+-----------
+ t_integral_excl | integral | 20 | -1
+ t_rational_key | rational | 1700 | 524296
+ t_strarr_idx | strarr | 1043 | -1
+ u0 | integral | 20 | -1
+ (4 rows)
+
+ -- Done. Retain the table under a less-generic name.
+ ALTER TABLE t RENAME TO alter_type_test;
+ RESET client_min_messages;
--
-- lock levels
--
diff --git a/src/test/regress/expected/big_alternew file mode 100644
index 0000000..1609c01
diff --git a/src/test/regress/sql/alter_table.sql b/index 4895768..8e5090e
100644
*** a/src/test/regress/sql/alter_table.sql
--- b/src/test/regress/sql/alter_table.sql
***************
*** 1094,1099 **** drop table another;
--- 1094,1166 ----
create table tab1 (a int, b text);
create table tab2 (x int, y tab1);
alter table tab1 alter column b type varchar; -- fails
+ alter table tab1 add check (b <> 'foo');
+ alter table tab1 add c int not null;
+ alter table tab1 add d int not null default 1; -- fails
+ alter table tab1 drop a;
+ alter table tab1 set with oids; -- fails
+
+ --
+ -- ALTER COLUMN ... SET DATA TYPE optimizations
+ --
+ SET client_min_messages = debug1; -- Track rewrites.
+
+ -- Model a type change that throws the semantics of dependent expressions.
+ CREATE DOMAIN trickint AS int;
+ CREATE FUNCTION touchy_f(trickint) RETURNS int4 LANGUAGE sql AS 'SELECT
100';
+ CREATE FUNCTION touchy_f(int4) RETURNS int4 LANGUAGE sql AS
'SELECT $1';
+ CREATE DOMAIN lendom AS varchar(8);
+ CREATE DOMAIN checkdom AS text CHECK (VALUE LIKE '<%');
+
+ CREATE TABLE parent (keycol numeric PRIMARY KEY);
+ INSERT INTO parent VALUES (0.12), (1.12);
+
+ CREATE TABLE t (
+ integral int4 NOT NULL,
+ rational numeric(9,4) UNIQUE NOT NULL REFERENCES
parent,
+ string varchar(4) NOT NULL,
+ strarr varchar(2)[] NOT NULL,
+ CHECK (touchy_f(integral) < 10),
+ EXCLUDE (integral WITH =)
+ );
+ CREATE INDEX ON t USING gin (strarr);
+ INSERT INTO t VALUES (1, 0.12, '<a/>', '{ab,cd}'), (2, 1.12, '<b/>',
'{ef,gh}');
+
+ -- Comments "rewrite", "verify" and "noop" signify whether ATRewriteTables
+ -- rewrites, scans or does nothing to the table proper. An "-e" suffix
denotes
+ -- an error outcome.
+ ALTER TABLE t ALTER integral TYPE trickint;
-- verify-e
+ ALTER TABLE t DROP CONSTRAINT t_integral_check;
+ ALTER TABLE t ALTER integral TYPE abstime USING integral::abstime; -- noop
+ ALTER TABLE t ALTER integral TYPE oid USING integral::int4;
-- noop
+ ALTER TABLE t ALTER integral TYPE regtype;
-- noop
+ ALTER TABLE t ALTER rational TYPE numeric(7,4);
-- verify
+ ALTER TABLE t ALTER rational TYPE numeric(8,4);
-- noop
+ ALTER TABLE t ALTER rational TYPE numeric(8,1);
-- rewrite-e
+ ALTER TABLE t ALTER string TYPE varchar(6);
-- noop
+ ALTER TABLE t ALTER string TYPE lendom;
-- noop
+ ALTER TABLE t ALTER string TYPE xml USING string::xml;
-- verify
+ ALTER TABLE t ALTER string TYPE checkdom;
-- verify
+ ALTER TABLE t ALTER string TYPE text USING 'foo'::varchar;
-- rewrite
+ ALTER TABLE t ALTER strarr TYPE varchar(4)[];
-- noop
+ ALTER TABLE t ADD CONSTRAINT u0 UNIQUE (integral), -- build index exactly once
+ ALTER integral TYPE int8;
-- rewrite
+
+ -- Data and catalog end state. We omit the columns that bear unstable OIDs.
+ SELECT * FROM t ORDER BY 1;
+
+ SELECT relname, indclass FROM pg_index JOIN pg_class c ON c.oid = indexrelid
+ WHERE indrelid = 't'::regclass ORDER BY 1;
+
+ SELECT relname, attname, atttypid, atttypmod
+ FROM pg_attribute JOIN pg_class c ON c.oid = attrelid
+ WHERE attnum > 0 AND
+ attrelid IN (SELECT indexrelid FROM pg_index WHERE indrelid =
't'::regclass)
+ ORDER BY 1, 2;
+
+ -- Done. Retain the table under a less-generic name.
+ ALTER TABLE t RENAME TO alter_type_test;
+ RESET client_min_messages;
--
-- lock levels
diff --git a/src/test/regress/sql/big_alternew file mode 100644
index 0000000..3824d96
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
