On Thu, Jan 20, 2011 at 09:26:29AM -0500, Robert Haas wrote:
> My main beef with the Boolean flags is that this kind of thing is not too
> clear:
>
> reindex_relation(myrel, false, false, true, true, false, true,
> false, false, true);
>
> Unless you have an excellent memory, you can't tell what the heck
> that's doing without flipping back and forth between the function
> definition and the call site. With a bit-field, it's a lot easier to
> glance at the call site and have a clue what's going on. We're of
> course not quite to the point of that exaggerated example yet.
Agreed.
> > However, suppose we inverted both flags, say REINDEX_SKIP_CONSTRAINT_CHECKS
> > and
> > REINDEX_ALLOW_OLD_INDEX_USE. ?Then, flags = 0 can hurt performance but not
> > correctness. ?That's looking like a win.
>
> I prefer the positive sense for those flags because I think it's more
> clear. There aren't so many call sites or so many people using this
> that we have to worry about what people are going to do in new calling
> locations; getting it right in any new code shouldn't be a
> consideration.
Okay. I've attached a new patch version based on that strategy.
diff --git a/src/backend/catalog/index.c b/src/backend/catalog/index.c
index 1c9df98..75e7055 100644
*** a/src/backend/catalog/index.c
--- b/src/backend/catalog/index.c
***************
*** 2533,2558 **** reindex_index(Oid indexId, bool skip_constraint_checks)
* reindex_relation - This routine is used to recreate all indexes
* of a relation (and optionally its toast relation too, if any).
*
! * If heap_rebuilt is true, then the relation was just completely rebuilt by
! * an operation such as VACUUM FULL or CLUSTER, and therefore its indexes are
! * inconsistent with it. This makes things tricky if the relation is a system
! * catalog that we might consult during the reindexing. To deal with that
! * case, we mark all of the indexes as pending rebuild so that they won't be
! * trusted until rebuilt. The caller is required to call us *without* having
! * made the rebuilt versions visible by doing CommandCounterIncrement; we'll
! * 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;
--- 2533,2561 ----
* reindex_relation - This routine is used to recreate all indexes
* of a relation (and optionally its toast relation too, if any).
*
! * "flags" can include REINDEX_SUPPRESS_INDEX_USE and
REINDEX_CHECK_CONSTRAINTS.
*
! * If flags has REINDEX_SUPPRESS_INDEX_USE, the relation was just completely
! * rebuilt by an operation such as VACUUM FULL or CLUSTER, and therefore its
! * indexes are inconsistent with it. This makes things tricky if the relation
! * is a system catalog that we might consult during the reindexing. To deal
! * with that case, we mark all of the indexes as pending rebuild so that they
! * won't be trusted until rebuilt. The caller is required to call us
*without*
! * having made the rebuilt versions visible by doing CommandCounterIncrement;
! * we'll do CCI after having collected the index list. (This way we can still
! * use catalog indexes while collecting the list.)
! *
! * To avoid deadlocks, VACUUM FULL or CLUSTER on a system catalog must omit
the
! * REINDEX_CHECK_CONSTRAINTS flag. REINDEX should be used to rebuild an index
! * if constraint inconsistency is suspected. For optimal performance, other
! * callers should include the flag only after transforming the data in a
manner
! * that risks a change in constraint validity.
*
* 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, int flags)
{
Relation rel;
Oid toast_relid;
***************
*** 2608,2614 **** reindex_relation(Oid relid, bool toast_too, bool
heap_rebuilt)
List *doneIndexes;
ListCell *indexId;
! if (heap_rebuilt)
{
/* Suppress use of all the indexes until they are
rebuilt */
SetReindexPending(indexIds);
--- 2611,2617 ----
List *doneIndexes;
ListCell *indexId;
! if (flags & REINDEX_SUPPRESS_INDEX_USE)
{
/* Suppress use of all the indexes until they are
rebuilt */
SetReindexPending(indexIds);
***************
*** 2629,2639 **** reindex_relation(Oid relid, bool toast_too, bool
heap_rebuilt)
if (is_pg_class)
RelationSetIndexList(rel, doneIndexes,
InvalidOid);
! reindex_index(indexOid, heap_rebuilt);
CommandCounterIncrement();
! if (heap_rebuilt)
RemoveReindexPending(indexOid);
if (is_pg_class)
--- 2632,2642 ----
if (is_pg_class)
RelationSetIndexList(rel, doneIndexes,
InvalidOid);
! reindex_index(indexOid, !(flags &
REINDEX_CHECK_CONSTRAINTS));
CommandCounterIncrement();
! if (flags & REINDEX_SUPPRESS_INDEX_USE)
RemoveReindexPending(indexOid);
if (is_pg_class)
***************
*** 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;
}
--- 2664,2675 ----
/*
* 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 right after rebuilding the heap.
*/
+ Assert(!(toast_too && (flags & REINDEX_SUPPRESS_INDEX_USE)));
if (toast_too && OidIsValid(toast_relid))
! result |= reindex_relation(toast_relid, false, flags);
return result;
}
diff --git a/src/backend/commands/index 19c3cf9..59a4394 100644
*** 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,1371 **** void
--- 1362,1373 ----
finish_heap_swap(Oid OIDOldHeap, Oid OIDNewHeap,
bool is_system_catalog,
bool swap_toast_by_content,
+ bool check_constraints,
TransactionId frozenXid)
{
ObjectAddress object;
Oid mapped_tables[4];
+ int reindex_flags;
int i;
/* Zero out possible results from swapped_relation_files */
***************
*** 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;
--- 1397,1406 ----
* so no chance to reclaim disk space before commit. We do not need a
* final CommandCounterIncrement() because reindex_relation does it.
*/
! reindex_flags = REINDEX_SUPPRESS_INDEX_USE;
! if (check_constraints)
! reindex_flags |= REINDEX_CHECK_CONSTRAINTS;
! reindex_relation(OIDOldHeap, false, reindex_flags);
/* Destroy new heap with old filenode */
object.classId = RelationRelationId;
diff --git a/src/backend/commands/indindex f809a26..7a6a4c3 100644
*** 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, 0))
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, 0))
ereport(NOTICE,
(errmsg("table \"%s.%s\" was reindexed",
get_namespace_name(get_rel_namespace(relid)),
diff --git a/src/backend/commands/tableindex 487d0ac..77ea9df 100644
*** 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, 0);
}
}
***************
*** 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
{
diff --git a/src/include/catalog/index.index 2804c63..29c4717 100644
*** 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,80 ----
extern void validate_index(Oid heapId, Oid indexId, Snapshot snapshot);
extern void reindex_index(Oid indexId, bool skip_constraint_checks);
!
! #define REINDEX_CHECK_CONSTRAINTS 0x1
! #define REINDEX_SUPPRESS_INDEX_USE 0x2
! extern bool reindex_relation(Oid relid, bool toast_too, int flags);
extern bool ReindexIsProcessingHeap(Oid heapOid);
extern bool ReindexIsProcessingIndex(Oid indexOid);
diff --git a/src/include/commands/index 427d802..518e896 100644
*** 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 check_constraints,
TransactionId frozenXid);
#endif /* CLUSTER_H */
diff --git a/src/test/regress/expecteindex 387aeaf..955b578 100644
*** 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"
diff --git a/src/test/regress/sql/alter_table.sqindex 18d60e1..fd12181 100644
*** 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
