On Tue, Jan 11, 2011 at 10:03:11PM -0500, Robert Haas wrote:
> On Sun, Jan 9, 2011 at 5:00 PM, Noah Misch <n...@leadboat.com> wrote:
> > When ALTER TABLE rewrites a table, it reindexes, but the reindex does not
> > revalidate UNIQUE/EXCLUDE constraints. ?This behaves badly in cases like
> > this,
> > neglecting to throw an error on the new UNIQUE violation:
> >
> > CREATE TABLE t (c numeric UNIQUE);
> > INSERT INTO t VALUES (1.1),(1.2);
> > ALTER TABLE t ALTER c TYPE int;
> >
> > The comment gave a reason for skipping the checks: it would cause deadlocks
> > when
> > we rewrite a system catalog. ?So, this patch changes things to only skip the
> > check for system catalogs.
>
> It looks like this behavior was introduced by Tom's commit
> 1ddc2703a936d03953657f43345460b9242bbed1 on 2010-02-07, and it appears
> to be quite broken. The behavior is reasonable in the case of VACUUM
> FULL and CLUSTER, but your example demonstrates that it's completely
> broken in the case of ALTER TABLE. This strikes me as something we
> need to fix in REL9_0_STABLE as well as master, and my gut feeling is
> that we ought to fix it not by excluding system relations but by
> making ALTER TABLE work differently from VACUUM FULL and CLUSTER.
> There's an efficiency benefit to skipping this even on normal
> relations in cases where it isn't necessary, and it shouldn't be
> necessary if we're rewriting the rows unchanged.
Something like the attached?
> Also, you need to start adding these patches to the CF app.
Done for all.
*** a/src/backend/catalog/index.c
--- b/src/backend/catalog/index.c
***************
*** 2543,2558 **** reindex_index(Oid indexId, bool skip_constraint_checks)
* do CCI after having collected the index list. (This way we can still use
* catalog indexes while collecting the list.)
*
! * We also skip rechecking uniqueness/exclusion constraint properties if
! * heap_rebuilt is true. This avoids likely deadlock conditions when doing
! * VACUUM FULL or CLUSTER on system catalogs. REINDEX should be used to
! * rebuild an index if constraint inconsistency is suspected.
*
* Returns true if any indexes were rebuilt. Note that a
* CommandCounterIncrement will occur after each index rebuild.
*/
bool
! reindex_relation(Oid relid, bool toast_too, bool heap_rebuilt)
{
Relation rel;
Oid toast_relid;
--- 2543,2559 ----
* do CCI after having collected the index list. (This way we can still use
* catalog indexes while collecting the list.)
*
! * If we rebuilt a heap without changing tuple values, we also skip rechecking
! * uniqueness/exclusion constraint properties. This avoids likely deadlock
! * conditions when doing VACUUM FULL or CLUSTER on system catalogs. REINDEX
! * should be used to rebuild an index if constraint inconsistency is
suspected.
*
* Returns true if any indexes were rebuilt. Note that a
* CommandCounterIncrement will occur after each index rebuild.
*/
bool
! reindex_relation(Oid relid, bool toast_too, bool heap_rebuilt,
! bool tuples_changed)
{
Relation rel;
Oid toast_relid;
***************
*** 2629,2635 **** reindex_relation(Oid relid, bool toast_too, bool
heap_rebuilt)
if (is_pg_class)
RelationSetIndexList(rel, doneIndexes,
InvalidOid);
! reindex_index(indexOid, heap_rebuilt);
CommandCounterIncrement();
--- 2630,2636 ----
if (is_pg_class)
RelationSetIndexList(rel, doneIndexes,
InvalidOid);
! reindex_index(indexOid, heap_rebuilt &&
!tuples_changed);
CommandCounterIncrement();
***************
*** 2661,2670 **** reindex_relation(Oid relid, bool toast_too, bool
heap_rebuilt)
/*
* If the relation has a secondary toast rel, reindex that too while we
! * still hold the lock on the master table.
*/
if (toast_too && OidIsValid(toast_relid))
! result |= reindex_relation(toast_relid, false, false);
return result;
}
--- 2662,2674 ----
/*
* If the relation has a secondary toast rel, reindex that too while we
! * still hold the lock on the master table. There's never a reason to
! * reindex the toast table if heap_rebuilt; it will always be fresh.
*/
+ Assert(!(toast_too && heap_rebuilt));
if (toast_too && OidIsValid(toast_relid))
! result |= reindex_relation(toast_relid, false, heap_rebuilt,
!
tuples_changed);
return result;
}
*** a/src/backend/commands/cluster.c
--- b/src/backend/commands/cluster.c
***************
*** 565,571 **** rebuild_relation(Relation OldHeap, Oid indexOid,
* rebuild the target's indexes and throw away the transient table.
*/
finish_heap_swap(tableOid, OIDNewHeap, is_system_catalog,
! swap_toast_by_content, frozenXid);
}
--- 565,571 ----
* rebuild the target's indexes and throw away the transient table.
*/
finish_heap_swap(tableOid, OIDNewHeap, is_system_catalog,
! swap_toast_by_content, false,
frozenXid);
}
***************
*** 1362,1367 **** void
--- 1362,1368 ----
finish_heap_swap(Oid OIDOldHeap, Oid OIDNewHeap,
bool is_system_catalog,
bool swap_toast_by_content,
+ bool tuples_changed,
TransactionId frozenXid)
{
ObjectAddress object;
***************
*** 1395,1401 **** finish_heap_swap(Oid OIDOldHeap, Oid OIDNewHeap,
* so no chance to reclaim disk space before commit. We do not need a
* final CommandCounterIncrement() because reindex_relation does it.
*/
! reindex_relation(OIDOldHeap, false, true);
/* Destroy new heap with old filenode */
object.classId = RelationRelationId;
--- 1396,1402 ----
* so no chance to reclaim disk space before commit. We do not need a
* final CommandCounterIncrement() because reindex_relation does it.
*/
! reindex_relation(OIDOldHeap, false, true, tuples_changed);
/* Destroy new heap with old filenode */
object.classId = RelationRelationId;
*** a/src/backend/commands/indexcmds.c
--- b/src/backend/commands/indexcmds.c
***************
*** 1629,1635 **** ReindexTable(RangeVar *relation)
ReleaseSysCache(tuple);
! if (!reindex_relation(heapOid, true, false))
ereport(NOTICE,
(errmsg("table \"%s\" has no indexes",
relation->relname)));
--- 1629,1635 ----
ReleaseSysCache(tuple);
! if (!reindex_relation(heapOid, true, false, false))
ereport(NOTICE,
(errmsg("table \"%s\" has no indexes",
relation->relname)));
***************
*** 1742,1748 **** ReindexDatabase(const char *databaseName, bool do_system,
bool do_user)
StartTransactionCommand();
/* functions in indexes may want a snapshot set */
PushActiveSnapshot(GetTransactionSnapshot());
! if (reindex_relation(relid, true, false))
ereport(NOTICE,
(errmsg("table \"%s.%s\" was reindexed",
get_namespace_name(get_rel_namespace(relid)),
--- 1742,1748 ----
StartTransactionCommand();
/* functions in indexes may want a snapshot set */
PushActiveSnapshot(GetTransactionSnapshot());
! if (reindex_relation(relid, true, false, false))
ereport(NOTICE,
(errmsg("table \"%s.%s\" was reindexed",
get_namespace_name(get_rel_namespace(relid)),
*** a/src/backend/commands/tablecmds.c
--- b/src/backend/commands/tablecmds.c
***************
*** 1076,1082 **** ExecuteTruncate(TruncateStmt *stmt)
/*
* Reconstruct the indexes to match, and we're done.
*/
! reindex_relation(heap_relid, true, false);
}
}
--- 1076,1082 ----
/*
* Reconstruct the indexes to match, and we're done.
*/
! reindex_relation(heap_relid, true, false, false);
}
}
***************
*** 3236,3248 **** ATRewriteTables(List **wqueue, LOCKMODE lockmode)
/*
* Swap the physical files of the old and new heaps,
then rebuild
! * indexes and discard the new heap. We can use
RecentXmin for
* the table's new relfrozenxid because we rewrote all
the tuples
* in ATRewriteTable, so no older Xid remains in the
table. Also,
* we never try to swap toast tables by content, since
we have no
* interest in letting this code work on system
catalogs.
*/
! finish_heap_swap(tab->relid, OIDNewHeap, false, false,
RecentXmin);
}
else
{
--- 3236,3249 ----
/*
* Swap the physical files of the old and new heaps,
then rebuild
! * indexes and discard the old heap. We can use
RecentXmin for
* the table's new relfrozenxid because we rewrote all
the tuples
* in ATRewriteTable, so no older Xid remains in the
table. Also,
* we never try to swap toast tables by content, since
we have no
* interest in letting this code work on system
catalogs.
*/
! finish_heap_swap(tab->relid, OIDNewHeap,
! false, false, true,
RecentXmin);
}
else
{
*** a/src/include/catalog/index.h
--- b/src/include/catalog/index.h
***************
*** 71,77 **** extern double IndexBuildHeapScan(Relation heapRelation,
extern void validate_index(Oid heapId, Oid indexId, Snapshot snapshot);
extern void reindex_index(Oid indexId, bool skip_constraint_checks);
! extern bool reindex_relation(Oid relid, bool toast_too, bool heap_rebuilt);
extern bool ReindexIsProcessingHeap(Oid heapOid);
extern bool ReindexIsProcessingIndex(Oid indexOid);
--- 71,78 ----
extern void validate_index(Oid heapId, Oid indexId, Snapshot snapshot);
extern void reindex_index(Oid indexId, bool skip_constraint_checks);
! extern bool reindex_relation(Oid relid, bool toast_too, bool heap_rebuilt,
! bool tuples_changed);
extern bool ReindexIsProcessingHeap(Oid heapOid);
extern bool ReindexIsProcessingIndex(Oid indexOid);
*** a/src/include/commands/cluster.h
--- b/src/include/commands/cluster.h
***************
*** 29,34 **** extern Oid make_new_heap(Oid OIDOldHeap, Oid
NewTableSpace);
--- 29,35 ----
extern void finish_heap_swap(Oid OIDOldHeap, Oid OIDNewHeap,
bool is_system_catalog,
bool swap_toast_by_content,
+ bool tuples_changed,
TransactionId frozenXid);
#endif /* CLUSTER_H */
*** a/src/test/regress/expected/alter_table.out
--- b/src/test/regress/expected/alter_table.out
***************
*** 1774,1779 **** DEBUG: Rebuilding index "t_strarr_idx"
--- 1774,1781 ----
DEBUG: Rebuilding index "t_square_idx"
DEBUG: Rebuilding index "t_expr_idx"
DEBUG: Rebuilding index "t_touchy_f_idx"
+ ERROR: could not create unique index "t_touchy_f_idx"
+ DETAIL: Key (touchy_f(constraint1))=(100) is duplicated.
ALTER TABLE t ALTER constraint2 TYPE trickint;
-- noop-e
DEBUG: Rewriting table "t"
DEBUG: Rebuilding index "t_constraint4_key"
***************
*** 1792,1816 **** DEBUG: Rebuilding index "t_strarr_idx"
DEBUG: Rebuilding index "t_square_idx"
DEBUG: Rebuilding index "t_touchy_f_idx"
DEBUG: Rebuilding index "t_expr_idx"
! -- Temporary fixup until behavior of the previous two improves.
! ALTER TABLE t ALTER constraint1 TYPE int, ALTER constraint2 TYPE int;
! DEBUG: Rewriting table "t"
! DEBUG: Rebuilding index "t_constraint4_key"
! DEBUG: Rebuilding index "t_integral_key"
! DEBUG: Rebuilding index "t_rational_key"
! DEBUG: Rebuilding index "t_daytimetz_key"
! DEBUG: Rebuilding index "t_daytime_key"
! DEBUG: Rebuilding index "t_stamptz_key"
! DEBUG: Rebuilding index "t_stamp_key"
! DEBUG: Rebuilding index "t_timegap_key"
! DEBUG: Rebuilding index "t_bits_key"
! DEBUG: Rebuilding index "t_network_key"
! DEBUG: Rebuilding index "t_string_idx"
! DEBUG: Rebuilding index "t_string_idx1"
! DEBUG: Rebuilding index "t_strarr_idx"
! DEBUG: Rebuilding index "t_square_idx"
! DEBUG: Rebuilding index "t_touchy_f_idx"
! DEBUG: Rebuilding index "t_expr_idx"
-- Change a column with an outgoing foreign key constraint.
ALTER TABLE t ALTER constraint3 TYPE numeric(8,1); -- rewrite, FK error
DEBUG: Rewriting table "t"
--- 1794,1801 ----
DEBUG: Rebuilding index "t_square_idx"
DEBUG: Rebuilding index "t_touchy_f_idx"
DEBUG: Rebuilding index "t_expr_idx"
! ERROR: could not create unique index "t_expr_idx"
! DETAIL: Key ((1))=(1) is duplicated.
-- Change a column with an outgoing foreign key constraint.
ALTER TABLE t ALTER constraint3 TYPE numeric(8,1); -- rewrite, FK error
DEBUG: Rewriting table "t"
*** a/src/test/regress/sql/alter_table.sql
--- b/src/test/regress/sql/alter_table.sql
***************
*** 1246,1253 **** FROM pg_trigger WHERE tgrelid = 't'::regclass ORDER BY
tgname;
ALTER TABLE t ALTER constraint0 TYPE trickint;
-- verify-e
ALTER TABLE t ALTER constraint1 TYPE trickint;
-- noop-e
ALTER TABLE t ALTER constraint2 TYPE trickint;
-- noop-e
- -- Temporary fixup until behavior of the previous two improves.
- ALTER TABLE t ALTER constraint1 TYPE int, ALTER constraint2 TYPE int;
-- Change a column with an outgoing foreign key constraint.
ALTER TABLE t ALTER constraint3 TYPE numeric(8,1); -- rewrite, FK error
--- 1246,1251 ----
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
