Avoid index rebuilds for no-rewrite ALTER TABLE ALTER TYPE
Previous version (at7-alt-index-opfamily.patch):
http://archives.postgresql.org/message-id/20110113230124.GA18733@tornado.gateway.2wire.net
Design:
http://archives.postgresql.org/message-id/20110524104029.GB18831@tornado.gateway.2wire.net
Patches committed in 2011CF1 allow ALTER TABLE ... ALTER ... SET DATA TYPE to
avoid rewriting the table in certain cases. We still rebuild all indexes and
recheck all foreign key constraints dependent on the changing column. This
patch avoids the rebuild for some indexes, using the operator family facility
to identify when that is safe.
Changes since last version:
* Instead of noting in comments that the operator family contracts do not make
the guarantees I desire and opining that the code is fine in practice anyway,
I amend those contracts in the documentation. See the aforementioned design
explanation, but note that its third and fifth sentences are incorrect. I
settled on requiring identical behavior among operators in the family after
implicit casts (of any castmethod) or binary coercion casts (of any
castcontext) on operands. I really only need the requirement for binary
coercion casts, but implicit casts seem like a obvious extension.
* Update for per-column collations. Index collations need to match.
* We don't assign meaning to GIN or GiST operator family groupings. For
access methods other than btree and hash, require an index rebuild unless the
operator classes match exactly. Even if we could do otherwise, it would
currently be academic; GIN "array_ops" is the only such family with more than
one constituent operator class. The only practical case I've observed to be
helped here is a conversion like varchar(2)[] -> varchar(4)[] with a GIN index
on the column; operator class comparison covers that fine.
* Remove support for avoiding foreign key constraint revalidation. This is
orthogonal to the index case, though they share many principles. If we get
this committed, I will submit a small patch for foreign keys at a later date.
* Original patch was submitted in two forms for comment on the relative
merits. The first form had tablecmds.c updating catalog entries pertaining to
an old index until it matched how the index would be recreated with the new
column data type. The second form actually dropped and recreated the index
using the usual facilities, then reattached the old RelFileNode to the "new"
index. Nobody commented, but I've come around to thinking the second approach
is clearly better. Therefore, I'm submitting only that.
* Change the division of labor between tablecmds.c and indexcmds.c.
* Test cases removed per 2011CF1 discussion.
* Sync with variable names changed since last submission.
This patch is fully independent of the "Eliding no-op varchar length
coercions" patch I've posted recently, though they do have synergy.
Thanks,
nm
Attachments:
at-index-opfamily-v2.patchtext/plain; charset=us-asciiDownload
diff --git a/doc/src/sgml/xindex.sgml b/doc/src/sgml/xindex.sgml
index a90b4e2..b6fe908 100644
*** a/doc/src/sgml/xindex.sgml
--- b/doc/src/sgml/xindex.sgml
***************
*** 834,846 **** ALTER OPERATOR FAMILY integer_ops USING btree ADD
<para>
In a B-tree operator family, all the operators in the family must sort
compatibly, meaning that the transitive laws hold across all the data types
! supported by the family: <quote>if A = B and B = C, then A =
! C</>, and <quote>if A < B and B < C, then A < C</>. For each
! operator in the family there must be a support function having the same
! two input data types as the operator. It is recommended that a family be
! complete, i.e., for each combination of data types, all operators are
! included. Each operator class should include just the non-cross-type
! operators and support function for its data type.
</para>
<para>
--- 834,848 ----
<para>
In a B-tree operator family, all the operators in the family must sort
compatibly, meaning that the transitive laws hold across all the data types
! supported by the family: <quote>if A = B and B = C, then A = C</>,
! and <quote>if A < B and B < C, then A < C</>. Subjecting operands
! to any number of implicit casts or binary coercion casts involving only
! types represented in the family must not change the result other than to
! require a similar cast thereon. For each operator in the family there must
! be a support function having the same two input data types as the operator.
! It is recommended that a family be complete, i.e., for each combination of
! data types, all operators are included. Each operator class should include
! just the non-cross-type operators and support function for its data type.
</para>
<para>
***************
*** 851,861 **** ALTER OPERATOR FAMILY integer_ops USING btree ADD
by the family's equality operators, even when the values are of different
types. This is usually difficult to accomplish when the types have
different physical representations, but it can be done in some cases.
! Notice that there is only one support function per data type, not one
! per equality operator. It is recommended that a family be complete, i.e.,
! provide an equality operator for each combination of data types.
! Each operator class should include just the non-cross-type equality
! operator and the support function for its data type.
</para>
<para>
--- 853,865 ----
by the family's equality operators, even when the values are of different
types. This is usually difficult to accomplish when the types have
different physical representations, but it can be done in some cases.
! Implicit casts and binary coercion casts among types represented in the
! family must preserve this invariant. Notice that there is only one support
! function per data type, not one per equality operator. It is recommended
! that a family be complete, i.e., provide an equality operator for each
! combination of data types. Each operator class should include just the
! non-cross-type equality operator and the support function for its data
! type.
</para>
<para>
diff --git a/src/backend/cataloindex 39ba486..6fc234b 100644
diff --git a/src/backend/catalog/sindex 57987be..682bd89 100644
*** a/src/backend/catalog/storage.c
--- b/src/backend/catalog/storage.c
***************
*** 221,229 **** RelationPreserveStorage(RelFileNode rnode)
next = pending->next;
if (RelFileNodeEquals(rnode, pending->relnode))
{
- /* we should only find delete-on-abort entries, else trouble */
- if (pending->atCommit)
- elog(ERROR, "cannot preserve a delete-on-commit relation");
/* unlink and delete list entry */
if (prev)
prev->next = next;
--- 221,226 ----
diff --git a/src/backend/commands/inindex b7c021d..073a3fa 100644
*** a/src/backend/commands/indexcmds.c
--- b/src/backend/commands/indexcmds.c
***************
*** 73,78 **** static char *ChooseIndexNameAddition(List *colnames);
--- 73,264 ----
/*
+ * CheckIndexCompatible
+ * Determine whether an existing index definition is compatible with a
+ * prospective index definition, such that the existing index storage
+ * could become the storage of the new index, avoiding a rebuild.
+ *
+ * 'heapRelation': the relation the index would apply to.
+ * 'accessMethodName': name of the AM to use.
+ * 'attributeList': a list of IndexElem specifying columns and expressions
+ * to index on.
+ * 'exclusionOpNames': list of names of exclusion-constraint operators,
+ * or NIL if not an exclusion constraint.
+ *
+ * This is tailored to the needs of ALTER TABLE ALTER TYPE, which recreates
+ * any indexes that depended on a changing column from their pg_get_indexdef
+ * or pg_get_constraintdef definitions. We omit some of the sanity checks of
+ * DefineIndex. We assume that the old and new indexes have the same number
+ * of columns and that if one has an expression column or predicate, both do.
+ * Errors arising from the attribute list still apply.
+ *
+ * Most column type changes that can skip a table rewrite will not invalidate
+ * indexes. For btree and hash indexes, we assume continued validity when
+ * each column of an index would have the same operator family before and
+ * after the change. Since we do not document a contract for GIN or GiST
+ * operator families, we require an exact operator class match for them and
+ * for any other access methods.
+ *
+ * DefineIndex always verifies that each exclusion operator shares an operator
+ * family with its corresponding index operator class. For access methods
+ * having no operator family contract, confirm that the old and new indexes
+ * use the exact same exclusion operator. For btree and hash, there's nothing
+ * more to check.
+ *
+ * We do not yet implement a test to verify compatibility of expression
+ * columns or predicates, so assume any such index is incompatible.
+ */
+ bool
+ CheckIndexCompatible(Oid oldId,
+ RangeVar *heapRelation,
+ char *accessMethodName,
+ List *attributeList,
+ List *exclusionOpNames)
+ {
+ bool isconstraint;
+ Oid *collationObjectId;
+ Oid *classObjectId;
+ Oid accessMethodId;
+ Oid relationId;
+ HeapTuple tuple;
+ Form_pg_am accessMethodForm;
+ bool amcanorder;
+ RegProcedure amoptions;
+ int16 *coloptions;
+ IndexInfo *indexInfo;
+ int numberOfAttributes;
+ int old_natts;
+ bool isnull;
+ bool family_am;
+ bool ret = true;
+ oidvector *old_indclass;
+ oidvector *old_indcollation;
+ int i;
+ Datum d;
+
+ /* Caller should already have the relation locked in some way. */
+ relationId = RangeVarGetRelid(heapRelation, false);
+ /*
+ * We can pretend isconstraint = false unconditionally. It only serves to
+ * decide the text of an error message that should never happen for us.
+ */
+ isconstraint = false;
+
+ numberOfAttributes = list_length(attributeList);
+ Assert(numberOfAttributes > 0);
+ Assert(numberOfAttributes <= INDEX_MAX_KEYS);
+
+ /* look up the access method */
+ tuple = SearchSysCache1(AMNAME, PointerGetDatum(accessMethodName));
+ if (!HeapTupleIsValid(tuple))
+ ereport(ERROR,
+ (errcode(ERRCODE_UNDEFINED_OBJECT),
+ errmsg("access method \"%s\" does not exist",
+ accessMethodName)));
+ accessMethodId = HeapTupleGetOid(tuple);
+ accessMethodForm = (Form_pg_am) GETSTRUCT(tuple);
+ amcanorder = accessMethodForm->amcanorder;
+ amoptions = accessMethodForm->amoptions;
+ ReleaseSysCache(tuple);
+
+ indexInfo = makeNode(IndexInfo);
+ indexInfo->ii_Expressions = NIL;
+ indexInfo->ii_ExpressionsState = NIL;
+ indexInfo->ii_PredicateState = NIL;
+ indexInfo->ii_ExclusionOps = NULL;
+ indexInfo->ii_ExclusionProcs = NULL;
+ indexInfo->ii_ExclusionStrats = NULL;
+ collationObjectId = (Oid *) palloc(numberOfAttributes * sizeof(Oid));
+ classObjectId = (Oid *) palloc(numberOfAttributes * sizeof(Oid));
+ coloptions = (int16 *) palloc(numberOfAttributes * sizeof(int16));
+
+ ComputeIndexAttrs(indexInfo, collationObjectId, classObjectId,
+ coloptions, attributeList,
+ exclusionOpNames, relationId,
+ accessMethodName, accessMethodId,
+ amcanorder, isconstraint);
+
+
+ /* Get the soon-obsolete pg_index tuple. */
+ tuple = SearchSysCache1(INDEXRELID, ObjectIdGetDatum(oldId));
+ if (!HeapTupleIsValid(tuple))
+ elog(ERROR, "cache lookup failed for index %u", oldId);
+
+ /* We don't assess expressions or predicates; assume incompatibility. */
+ if (!(heap_attisnull(tuple, Anum_pg_index_indpred) &&
+ heap_attisnull(tuple, Anum_pg_index_indexprs)))
+ {
+ ReleaseSysCache(tuple);
+ return false;
+ }
+
+ /*
+ * If the old and new operator class of any index column differ in
+ * operator family or collation, regard the old index as incompatible.
+ * For access methods other than btree and hash, a family match has no
+ * defined meaning; require an exact operator class match.
+ */
+ old_natts = ((Form_pg_index) GETSTRUCT(tuple))->indnatts;
+ Assert(old_natts == numberOfAttributes);
+
+ d = SysCacheGetAttr(INDEXRELID, tuple, Anum_pg_index_indcollation, &isnull);
+ Assert(!isnull);
+ old_indcollation = (oidvector *) DatumGetPointer(d);
+
+ d = SysCacheGetAttr(INDEXRELID, tuple, Anum_pg_index_indclass, &isnull);
+ Assert(!isnull);
+ old_indclass = (oidvector *) DatumGetPointer(d);
+
+ family_am = accessMethodId == BTREE_AM_OID || accessMethodId == HASH_AM_OID;
+
+ for (i = 0; i < old_natts; i++)
+ {
+ Oid old_class = old_indclass->values[i];
+ Oid new_class = classObjectId[i];
+
+ if (!(old_indcollation->values[i] == collationObjectId[i]
+ && (old_class == new_class
+ || (family_am && (get_opclass_family(old_class)
+ == get_opclass_family(new_class))))))
+ {
+ ret = false;
+ break;
+ }
+ }
+
+ ReleaseSysCache(tuple);
+
+ /*
+ * For btree and hash, exclusion operators need only fall in the same
+ * operator family; ComputeIndexAttrs already verified that much. If we
+ * get this far, we know that the index operator family has not changed,
+ * and we're done. For other access methods, require exact matches for
+ * all exclusion operators.
+ */
+ if (ret && !family_am && indexInfo->ii_ExclusionOps != NULL)
+ {
+ Relation irel;
+ Oid *old_operators, *old_procs;
+ uint16 *old_strats;
+
+ /* Caller probably already holds a stronger lock. */
+ irel = index_open(oldId, AccessShareLock);
+ RelationGetExclusionInfo(irel, &old_operators, &old_procs, &old_strats);
+
+ for (i = 0; i < old_natts; i++)
+ if (old_operators[i] != indexInfo->ii_ExclusionOps[i])
+ {
+ ret = false;
+ break;
+ }
+
+ index_close(irel, NoLock);
+ }
+
+ return ret;
+ }
+
+ /*
* DefineIndex
* Creates a new index.
*
***************
*** 103,110 **** static char *ChooseIndexNameAddition(List *colnames);
* it will be filled later.
* 'quiet': suppress the NOTICE chatter ordinarily provided for constraints.
* 'concurrent': avoid blocking writers to the table while building.
*/
! void
DefineIndex(RangeVar *heapRelation,
char *indexRelationName,
Oid indexRelationId,
--- 289,298 ----
* it will be filled later.
* 'quiet': suppress the NOTICE chatter ordinarily provided for constraints.
* 'concurrent': avoid blocking writers to the table while building.
+ *
+ * Returns the OID of the new index relation.
*/
! Oid
DefineIndex(RangeVar *heapRelation,
char *indexRelationName,
Oid indexRelationId,
***************
*** 421,427 **** DefineIndex(RangeVar *heapRelation,
{
/* Close the heap and we're done, in the non-concurrent case */
heap_close(rel, NoLock);
! return;
}
/* save lockrelid and locktag for below, then close rel */
--- 609,615 ----
{
/* Close the heap and we're done, in the non-concurrent case */
heap_close(rel, NoLock);
! return indexRelationId;
}
/* save lockrelid and locktag for below, then close rel */
***************
*** 709,714 **** DefineIndex(RangeVar *heapRelation,
--- 897,904 ----
* Last thing to do is release the session-level lock on the parent table.
*/
UnlockRelationIdForSession(&heaprelid, ShareUpdateExclusiveLock);
+
+ return indexRelationId;
}
diff --git a/src/backend/commands/tableindex 2c9f855..3860e61 100644
*** a/src/backend/commands/tablecmds.c
--- b/src/backend/commands/tablecmds.c
***************
*** 344,350 **** static bool ATColumnChangeRequiresRewrite(Node *expr, AttrNumber varattno);
static void ATExecAlterColumnType(AlteredTableInfo *tab, Relation rel,
AlterTableCmd *cmd, LOCKMODE lockmode);
static void ATPostAlterTypeCleanup(List **wqueue, AlteredTableInfo *tab, LOCKMODE lockmode);
! static void ATPostAlterTypeParse(char *cmd, List **wqueue, LOCKMODE lockmode);
static void change_owner_recurse_to_sequences(Oid relationOid,
Oid newOwnerId, LOCKMODE lockmode);
static void ATExecClusterOn(Relation rel, const char *indexName, LOCKMODE lockmode);
--- 344,352 ----
static void ATExecAlterColumnType(AlteredTableInfo *tab, Relation rel,
AlterTableCmd *cmd, LOCKMODE lockmode);
static void ATPostAlterTypeCleanup(List **wqueue, AlteredTableInfo *tab, LOCKMODE lockmode);
! static void ATPostAlterTypeParse(Oid oldId, char *cmd,
! List **wqueue, LOCKMODE lockmode, bool rewrite);
! static void TryReuseIndex(Oid oldId, IndexStmt *stmt);
static void change_owner_recurse_to_sequences(Oid relationOid,
Oid newOwnerId, LOCKMODE lockmode);
static void ATExecClusterOn(Relation rel, const char *indexName, LOCKMODE lockmode);
***************
*** 5179,5215 **** ATExecAddIndex(AlteredTableInfo *tab, Relation rel,
bool check_rights;
bool skip_build;
bool quiet;
Assert(IsA(stmt, IndexStmt));
/* suppress schema rights check when rebuilding existing index */
check_rights = !is_rebuild;
! /* skip index build if phase 3 will have to rewrite table anyway */
! skip_build = tab->rewrite;
/* suppress notices when rebuilding existing index */
quiet = is_rebuild;
/* The IndexStmt has already been through transformIndexStmt */
! DefineIndex(stmt->relation, /* relation */
! stmt->idxname, /* index name */
! InvalidOid, /* no predefined OID */
! stmt->accessMethod, /* am name */
! stmt->tableSpace,
! stmt->indexParams, /* parameters */
! (Expr *) stmt->whereClause,
! stmt->options,
! stmt->excludeOpNames,
! stmt->unique,
! stmt->primary,
! stmt->isconstraint,
! stmt->deferrable,
! stmt->initdeferred,
! true, /* is_alter_table */
! check_rights,
! skip_build,
! quiet,
! false);
}
/*
--- 5181,5255 ----
bool check_rights;
bool skip_build;
bool quiet;
+ Oid new_index;
Assert(IsA(stmt, IndexStmt));
/* suppress schema rights check when rebuilding existing index */
check_rights = !is_rebuild;
! /* skip index build if phase 3 will do it or we're reusing an old one */
! skip_build = tab->rewrite || OidIsValid(stmt->oldNode);
/* suppress notices when rebuilding existing index */
quiet = is_rebuild;
/* The IndexStmt has already been through transformIndexStmt */
! new_index = DefineIndex(stmt->relation, /* relation */
! stmt->idxname, /* index name */
! InvalidOid, /* no predefined OID */
! stmt->accessMethod, /* am name */
! stmt->tableSpace,
! stmt->indexParams, /* parameters */
! (Expr *) stmt->whereClause,
! stmt->options,
! stmt->excludeOpNames,
! stmt->unique,
! stmt->primary,
! stmt->isconstraint,
! stmt->deferrable,
! stmt->initdeferred,
! true, /* is_alter_table */
! check_rights,
! skip_build,
! quiet,
! false);
!
! /*
! * If TryReuseIndex() stashed a relfilenode for us, replace the fresh one
! * with that. This vaguely follows swap_relation_files(), but we need not
! * consider the case of a system table. Indexes inherit relfrozenxid and
! * never have TOAST tables.
! */
! if (OidIsValid(stmt->oldNode))
! {
! Relation pg_class;
! Relation irel;
! HeapTuple tup;
!
! /* Drop the empty storage just created under DefineIndex. */
! irel = index_open(new_index, NoLock);
! RelationDropStorage(irel);
!
! /* Update pg_class.relfilenode. */
! pg_class = heap_open(RelationRelationId, RowExclusiveLock);
! tup = SearchSysCacheCopy1(RELOID, ObjectIdGetDatum(new_index));
! if (!HeapTupleIsValid(tup)) /* shouldn't happen */
! elog(ERROR, "cache lookup failed for relation %u", new_index);
!
! ((Form_pg_class) GETSTRUCT(tup))->relfilenode = stmt->oldNode;
! simple_heap_update(pg_class, &tup->t_self, tup);
! CatalogUpdateIndexes(pg_class, tup);
! heap_close(pg_class, RowExclusiveLock);
!
! CommandCounterIncrement();
!
! /*
! * The reassigned storage is probably scheduled for deletion, having
! * been attached to a just-DROPped index. Revive it.
! */
! RelationPreserveStorage(irel->rd_node);
! index_close(irel, NoLock);
! }
}
/*
***************
*** 7171,7177 **** static void
ATPostAlterTypeCleanup(List **wqueue, AlteredTableInfo *tab, LOCKMODE lockmode)
{
ObjectAddress obj;
! ListCell *l;
/*
* Re-parse the index and constraint definitions, and attach them to the
--- 7211,7218 ----
ATPostAlterTypeCleanup(List **wqueue, AlteredTableInfo *tab, LOCKMODE lockmode)
{
ObjectAddress obj;
! ListCell *def;
! ListCell *oid;
/*
* Re-parse the index and constraint definitions, and attach them to the
***************
*** 7181,7190 **** ATPostAlterTypeCleanup(List **wqueue, AlteredTableInfo *tab, LOCKMODE lockmode)
* that before dropping. It's safe because the parser won't actually look
* at the catalogs to detect the existing entry.
*/
! foreach(l, tab->changedIndexDefs)
! ATPostAlterTypeParse((char *) lfirst(l), wqueue, lockmode);
! foreach(l, tab->changedConstraintDefs)
! ATPostAlterTypeParse((char *) lfirst(l), wqueue, lockmode);
/*
* Now we can drop the existing constraints and indexes --- constraints
--- 7222,7233 ----
* that before dropping. It's safe because the parser won't actually look
* at the catalogs to detect the existing entry.
*/
! forboth(oid, tab->changedConstraintOids, def, tab->changedConstraintDefs)
! ATPostAlterTypeParse(lfirst_oid(oid), (char *) lfirst(def),
! wqueue, lockmode, tab->rewrite);
! forboth(oid, tab->changedIndexOids, def, tab->changedIndexDefs)
! ATPostAlterTypeParse(lfirst_oid(oid), (char *) lfirst(def),
! wqueue, lockmode, tab->rewrite);
/*
* Now we can drop the existing constraints and indexes --- constraints
***************
*** 7194,7211 **** ATPostAlterTypeCleanup(List **wqueue, AlteredTableInfo *tab, LOCKMODE lockmode)
* should be okay to use DROP_RESTRICT here, since nothing else should be
* depending on these objects.
*/
! foreach(l, tab->changedConstraintOids)
{
obj.classId = ConstraintRelationId;
! obj.objectId = lfirst_oid(l);
obj.objectSubId = 0;
performDeletion(&obj, DROP_RESTRICT);
}
! foreach(l, tab->changedIndexOids)
{
obj.classId = RelationRelationId;
! obj.objectId = lfirst_oid(l);
obj.objectSubId = 0;
performDeletion(&obj, DROP_RESTRICT);
}
--- 7237,7254 ----
* should be okay to use DROP_RESTRICT here, since nothing else should be
* depending on these objects.
*/
! foreach(oid, tab->changedConstraintOids)
{
obj.classId = ConstraintRelationId;
! obj.objectId = lfirst_oid(oid);
obj.objectSubId = 0;
performDeletion(&obj, DROP_RESTRICT);
}
! foreach(oid, tab->changedIndexOids)
{
obj.classId = RelationRelationId;
! obj.objectId = lfirst_oid(oid);
obj.objectSubId = 0;
performDeletion(&obj, DROP_RESTRICT);
}
***************
*** 7217,7223 **** ATPostAlterTypeCleanup(List **wqueue, AlteredTableInfo *tab, LOCKMODE lockmode)
}
static void
! ATPostAlterTypeParse(char *cmd, List **wqueue, LOCKMODE lockmode)
{
List *raw_parsetree_list;
List *querytree_list;
--- 7260,7267 ----
}
static void
! ATPostAlterTypeParse(Oid oldId, char *cmd,
! List **wqueue, LOCKMODE lockmode, bool rewrite)
{
List *raw_parsetree_list;
List *querytree_list;
***************
*** 7264,7269 **** ATPostAlterTypeParse(char *cmd, List **wqueue, LOCKMODE lockmode)
--- 7308,7316 ----
IndexStmt *stmt = (IndexStmt *) stm;
AlterTableCmd *newcmd;
+ if (!rewrite)
+ TryReuseIndex(oldId, stmt);
+
rel = relation_openrv(stmt->relation, lockmode);
tab = ATGetQueueEntry(wqueue, rel);
newcmd = makeNode(AlterTableCmd);
***************
*** 7288,7293 **** ATPostAlterTypeParse(char *cmd, List **wqueue, LOCKMODE lockmode)
--- 7335,7344 ----
switch (cmd->subtype)
{
case AT_AddIndex:
+ Assert(IsA(cmd->def, IndexStmt));
+ if (!rewrite)
+ TryReuseIndex(get_constraint_index(oldId),
+ (IndexStmt *) cmd->def);
cmd->subtype = AT_ReAddIndex;
tab->subcmds[AT_PASS_OLD_INDEX] =
lappend(tab->subcmds[AT_PASS_OLD_INDEX], cmd);
***************
*** 7311,7316 **** ATPostAlterTypeParse(char *cmd, List **wqueue, LOCKMODE lockmode)
--- 7362,7387 ----
}
}
+ /*
+ * Subroutine for ATPostAlterTypeParse(). Calls out to CheckIndexCompatible()
+ * for the real analysis, then mutates the IndexStmt based on that verdict.
+ */
+ static void
+ TryReuseIndex(Oid oldId, IndexStmt *stmt)
+ {
+
+ if (CheckIndexCompatible(oldId,
+ stmt->relation,
+ stmt->accessMethod,
+ stmt->indexParams,
+ stmt->excludeOpNames))
+ {
+ Relation irel = index_open(oldId, NoLock);
+ stmt->oldNode = irel->rd_node.relNode;
+ index_close(irel, NoLock);
+ }
+ }
+
/*
* ALTER TABLE OWNER
diff --git a/src/backend/nodes/copyfuncindex c9133dd..c18d670 100644
*** a/src/backend/nodes/copyfuncs.c
--- b/src/backend/nodes/copyfuncs.c
***************
*** 2813,2818 **** _copyIndexStmt(IndexStmt *from)
--- 2813,2819 ----
COPY_SCALAR_FIELD(deferrable);
COPY_SCALAR_FIELD(initdeferred);
COPY_SCALAR_FIELD(concurrent);
+ COPY_SCALAR_FIELD(oldNode);
return newnode;
}
diff --git a/src/backend/nodes/equalindex 3a0267c..61fed33 100644
*** a/src/backend/nodes/equalfuncs.c
--- b/src/backend/nodes/equalfuncs.c
***************
*** 1251,1256 **** _equalIndexStmt(IndexStmt *a, IndexStmt *b)
--- 1251,1257 ----
COMPARE_SCALAR_FIELD(deferrable);
COMPARE_SCALAR_FIELD(initdeferred);
COMPARE_SCALAR_FIELD(concurrent);
+ COMPARE_SCALAR_FIELD(oldNode);
return true;
}
diff --git a/src/backend/nodes/outfunindex 681f5f8..0dfa5f4 100644
*** a/src/backend/nodes/outfuncs.c
--- b/src/backend/nodes/outfuncs.c
***************
*** 1982,1987 **** _outIndexStmt(StringInfo str, IndexStmt *node)
--- 1982,1988 ----
WRITE_BOOL_FIELD(deferrable);
WRITE_BOOL_FIELD(initdeferred);
WRITE_BOOL_FIELD(concurrent);
+ WRITE_OID_FIELD(oldNode);
}
static void
diff --git a/src/include/commands/dindex bbc024f..cbb5f1f 100644
*** a/src/include/commands/defrem.h
--- b/src/include/commands/defrem.h
***************
*** 18,24 ****
/* commands/indexcmds.c */
! extern void DefineIndex(RangeVar *heapRelation,
char *indexRelationName,
Oid indexRelationId,
char *accessMethodName,
--- 18,24 ----
/* commands/indexcmds.c */
! extern Oid DefineIndex(RangeVar *heapRelation,
char *indexRelationName,
Oid indexRelationId,
char *accessMethodName,
***************
*** 49,54 **** extern char *ChooseIndexName(const char *tabname, Oid namespaceId,
--- 49,59 ----
List *colnames, List *exclusionOpNames,
bool primary, bool isconstraint);
extern List *ChooseIndexColumnNames(List *indexElems);
+ extern bool CheckIndexCompatible(Oid oldId,
+ RangeVar *heapRelation,
+ char *accessMethodName,
+ List *attributeList,
+ List *exclusionOpNames);
extern Oid GetDefaultOpClass(Oid type_id, Oid am_id);
/* commands/functioncmds.c */
diff --git a/src/include/nodes/parseindex 14937d4..2d226a9 100644
*** a/src/include/nodes/parsenodes.h
--- b/src/include/nodes/parsenodes.h
***************
*** 2065,2070 **** typedef struct IndexStmt
--- 2065,2071 ----
bool deferrable; /* is the constraint DEFERRABLE? */
bool initdeferred; /* is the constraint INITIALLY DEFERRED? */
bool concurrent; /* should this be a concurrent index build? */
+ Oid oldNode; /* relfilenode of my former self */
} IndexStmt;
/* ----------------------
diff --git a/src/test/regress/GNUmakeindex 90aea6c..949f82b 100644
diff --git a/src/test/regress/expecnew file mode 100644
index 0000000..10dbcc7
diff --git a/src/test/regress/parallel_schedule b/srindex 376f28d..e3b515e 100644
diff --git a/src/test/regress/serial_scheindex bb654f9..a7e6dc6 100644
diff --git a/src/test/regress/sql/big_anew file mode 100644
index 0000000..6ae9007
On Wed, Jun 15, 2011 at 1:03 AM, Noah Misch <noah@leadboat.com> wrote:
[patch to avoid index rebuilds]
With respect to the documentation hunks, it seems to me that the first
hunk might be made clearer by leaving the paragraph of which it is a
part as-is, and adding another paragraph afterwards beginning with the
words "In addition". I am not sure whether the second hunk is
necessary at all. Doesn't the existing language cover the same
territory as what you've added?
I think that the variables in ATPostAlterTypeCleanup() could be better
named. They appear to be values, when in fact they are ListCells.
Honestly I'd probably just use l1 and l2, but if you want to insist on
some more mnemonic naming it should probably be something that sounds
vaguely list-ish.
As you no doubt expected, my eyes was immediately drawn to the
index-resurrection hack. Reviewing the thread, I see that you asked
about that in January and never got feedback. I have to say that what
you've done here looks like a pretty vile hack, but it's hard to say
for sure without knowing what to compare it against. You made
reference to this being smaller and simpler than updating the index
definition in place - can you give a sketch of what would need to be
done if we went that route instead?
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
On Mon, Jun 27, 2011 at 03:45:43PM -0400, Robert Haas wrote:
On Wed, Jun 15, 2011 at 1:03 AM, Noah Misch <noah@leadboat.com> wrote:
[patch to avoid index rebuilds]
With respect to the documentation hunks, it seems to me that the first
hunk might be made clearer by leaving the paragraph of which it is a
part as-is, and adding another paragraph afterwards beginning with the
words "In addition".
The added restriction elaborates on the transitivity requirement, so I wanted to
keep the new language adjacent to that.
I am not sure whether the second hunk is
necessary at all. Doesn't the existing language cover the same
territory as what you've added?
The first hunk updates the contract for btree families, and the second updates
the contract for hash families. I kept the second instance a bit terse since it
follows soon after the similar text for B-tree.
I think that the variables in ATPostAlterTypeCleanup() could be better
named. They appear to be values, when in fact they are ListCells.
Honestly I'd probably just use l1 and l2, but if you want to insist on
some more mnemonic naming it should probably be something that sounds
vaguely list-ish.
Okay; I'll do that in the next version. Either l1/l2 or maybe oid_item/def_item
like we use in postgres.c.
As you no doubt expected, my eyes was immediately drawn to the
index-resurrection hack. Reviewing the thread, I see that you asked
about that in January and never got feedback. I have to say that what
you've done here looks like a pretty vile hack, but it's hard to say
for sure without knowing what to compare it against. You made
reference to this being smaller and simpler than updating the index
definition in place - can you give a sketch of what would need to be
done if we went that route instead?
In "at7-index-opfamily.patch" attached to
http://archives.postgresql.org/message-id/20110113230124.GA18733@tornado.gateway.2wire.net
check out the code following the comment "/* The old index is compatible.
Update catalogs. */" until the end of the function. That code would need
updates for per-column collations, and it incorrectly reuses
values/nulls/replace arrays. It probably does not belong in tablecmds.c,
either. However, it gives the right general outline.
It would be valuable to avoid introducing a second chunk of code that knows
everything about the catalog entries behind an index. That's what led me to the
put forward the most recent version as best. What do you find vile about that
approach? I wasn't comfortable with it at first, because I suspected the checks
in RelationPreserveStorage() might be important for correctness. Having studied
it some more, though, I think they just reflect the narrower needs of its
current sole user.
Thanks,
nm
On Mon, Jun 27, 2011 at 10:43 PM, Noah Misch <noah@leadboat.com> wrote:
On Mon, Jun 27, 2011 at 03:45:43PM -0400, Robert Haas wrote:
On Wed, Jun 15, 2011 at 1:03 AM, Noah Misch <noah@leadboat.com> wrote:
[patch to avoid index rebuilds]
With respect to the documentation hunks, it seems to me that the first
hunk might be made clearer by leaving the paragraph of which it is a
part as-is, and adding another paragraph afterwards beginning with the
words "In addition".The added restriction elaborates on the transitivity requirement, so I wanted to
keep the new language adjacent to that.
That makes sense, but it reads a bit awkwardly to me. Maybe it's just
that the sentence itself is so complicated that I have difficulty
understanding it. I guess it's the same problem as with the text you
added about hash indexes: without thinking about it, it's hard to
understand what territory is covered by the new sentence that is not
covered by what's already there. In the case of the hash indexes, I
think I have it figured out: we already know that we must get
compatible hash values out of logically equal values of different
datatypes. But it's possible that the inter-type cast changes the
value in some arbitrary way and then compensates for it by defining
the hash function in such a way as to compensate. Similarly, for
btrees, we need the relative ordering of A and B to remain the same
after casting within the opfamily, not to be rearranged somehow.
Maybe the text you've got is OK to explain this, but I wonder if
there's a way to do it more simply.
As you no doubt expected, my eyes was immediately drawn to the
index-resurrection hack. Reviewing the thread, I see that you asked
about that in January and never got feedback. I have to say that what
you've done here looks like a pretty vile hack, but it's hard to say
for sure without knowing what to compare it against. You made
reference to this being smaller and simpler than updating the index
definition in place - can you give a sketch of what would need to be
done if we went that route instead?In "at7-index-opfamily.patch" attached to
http://archives.postgresql.org/message-id/20110113230124.GA18733@tornado.gateway.2wire.net
check out the code following the comment "/* The old index is compatible.
Update catalogs. */" until the end of the function. That code would need
updates for per-column collations, and it incorrectly reuses
values/nulls/replace arrays. It probably does not belong in tablecmds.c,
either. However, it gives the right general outline.It would be valuable to avoid introducing a second chunk of code that knows
everything about the catalog entries behind an index. That's what led me to the
put forward the most recent version as best. What do you find vile about that
approach? I wasn't comfortable with it at first, because I suspected the checks
in RelationPreserveStorage() might be important for correctness. Having studied
it some more, though, I think they just reflect the narrower needs of its
current sole user.
Maybe vile is a strong word, but it seems like a modularity violation:
we're basically letting DefineIndex() do some stuff we don't really
want done, and then backing it out parts of it that we don't really
want done after all. It seems like it would be better to provide
DefineIndex with enough information not to do the wrong thing in the
first place. Could we maybe pass stmt->oldNode to DefineIndex(), and
let it work out the details?
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
On Tue, Jun 28, 2011 at 02:11:11PM -0400, Robert Haas wrote:
On Mon, Jun 27, 2011 at 10:43 PM, Noah Misch <noah@leadboat.com> wrote:
On Mon, Jun 27, 2011 at 03:45:43PM -0400, Robert Haas wrote:
On Wed, Jun 15, 2011 at 1:03 AM, Noah Misch <noah@leadboat.com> wrote:
[patch to avoid index rebuilds]
With respect to the documentation hunks, it seems to me that the first
hunk might be made clearer by leaving the paragraph of which it is a
part as-is, and adding another paragraph afterwards beginning with the
words "In addition".The added restriction elaborates on the transitivity requirement, so I wanted to
keep the new language adjacent to that.That makes sense, but it reads a bit awkwardly to me. Maybe it's just
that the sentence itself is so complicated that I have difficulty
understanding it. I guess it's the same problem as with the text you
added about hash indexes: without thinking about it, it's hard to
understand what territory is covered by the new sentence that is not
covered by what's already there. In the case of the hash indexes, I
think I have it figured out: we already know that we must get
compatible hash values out of logically equal values of different
datatypes. But it's possible that the inter-type cast changes the
value in some arbitrary way and then compensates for it by defining
the hash function in such a way as to compensate. Similarly, for
btrees, we need the relative ordering of A and B to remain the same
after casting within the opfamily, not to be rearranged somehow.
Maybe the text you've got is OK to explain this, but I wonder if
there's a way to do it more simply.
That's basically right, I think. Presently, there is no connection between
casts and operator family notions of equality. For example, a cast can change
the hash value. In general, that's not wrong. However, I wish to forbid it
when some hash operator family covers both the source and destination types of
the cast. Note that, I don't especially care whether the stored bits changed or
not. I just want casts to preserve equality when an operator family defines
equality across the types involved in the cast. The specific way of
articulating that is probably vulnerable to improvement.
It would be valuable to avoid introducing a second chunk of code that knows
everything about the catalog entries behind an index. ?That's what led me to the
put forward the most recent version as best. ?What do you find vile about that
approach? ?I wasn't comfortable with it at first, because I suspected the checks
in RelationPreserveStorage() might be important for correctness. ?Having studied
it some more, though, I think they just reflect the narrower needs of its
current sole user.Maybe vile is a strong word, but it seems like a modularity violation:
we're basically letting DefineIndex() do some stuff we don't really
want done, and then backing it out parts of it that we don't really
want done after all. It seems like it would be better to provide
DefineIndex with enough information not to do the wrong thing in the
first place. Could we maybe pass stmt->oldNode to DefineIndex(), and
let it work out the details?
True. I initially shied away from that, because we assume somewhat deep into
the stack that the new relation will have pg_class.oid = pg_class.relfilenode.
Here's the call stack in question:
RelationBuildLocalRelation
heap_create
index_create
DefineIndex
ATExecAddIndex
Looking at it again, it wouldn't bring the end of the world to add a relfilenode
argument to each. None of those have more than four callers. ATExecAddIndex()
would then call RelationPreserveStorage() before calling DefineIndex(), which
would in turn put things in a correct state from the start. Does that seem
appropriate? Offhand, I do like it better than what I had.
Thanks,
nm
On Tue, Jun 28, 2011 at 3:40 PM, Noah Misch <noah@leadboat.com> wrote:
On Tue, Jun 28, 2011 at 02:11:11PM -0400, Robert Haas wrote:
On Mon, Jun 27, 2011 at 10:43 PM, Noah Misch <noah@leadboat.com> wrote:
On Mon, Jun 27, 2011 at 03:45:43PM -0400, Robert Haas wrote:
On Wed, Jun 15, 2011 at 1:03 AM, Noah Misch <noah@leadboat.com> wrote:
[patch to avoid index rebuilds]
With respect to the documentation hunks, it seems to me that the first
hunk might be made clearer by leaving the paragraph of which it is a
part as-is, and adding another paragraph afterwards beginning with the
words "In addition".The added restriction elaborates on the transitivity requirement, so I wanted to
keep the new language adjacent to that.That makes sense, but it reads a bit awkwardly to me. Maybe it's just
that the sentence itself is so complicated that I have difficulty
understanding it. I guess it's the same problem as with the text you
added about hash indexes: without thinking about it, it's hard to
understand what territory is covered by the new sentence that is not
covered by what's already there. In the case of the hash indexes, I
think I have it figured out: we already know that we must get
compatible hash values out of logically equal values of different
datatypes. But it's possible that the inter-type cast changes the
value in some arbitrary way and then compensates for it by defining
the hash function in such a way as to compensate. Similarly, for
btrees, we need the relative ordering of A and B to remain the same
after casting within the opfamily, not to be rearranged somehow.
Maybe the text you've got is OK to explain this, but I wonder if
there's a way to do it more simply.That's basically right, I think. Presently, there is no connection between
casts and operator family notions of equality. For example, a cast can change
the hash value. In general, that's not wrong. However, I wish to forbid it
when some hash operator family covers both the source and destination types of
the cast. Note that, I don't especially care whether the stored bits changed or
not. I just want casts to preserve equality when an operator family defines
equality across the types involved in the cast. The specific way of
articulating that is probably vulnerable to improvement.It would be valuable to avoid introducing a second chunk of code that knows
everything about the catalog entries behind an index. ?That's what led me to the
put forward the most recent version as best. ?What do you find vile about that
approach? ?I wasn't comfortable with it at first, because I suspected the checks
in RelationPreserveStorage() might be important for correctness. ?Having studied
it some more, though, I think they just reflect the narrower needs of its
current sole user.Maybe vile is a strong word, but it seems like a modularity violation:
we're basically letting DefineIndex() do some stuff we don't really
want done, and then backing it out parts of it that we don't really
want done after all. It seems like it would be better to provide
DefineIndex with enough information not to do the wrong thing in the
first place. Could we maybe pass stmt->oldNode to DefineIndex(), and
let it work out the details?True. I initially shied away from that, because we assume somewhat deep into
the stack that the new relation will have pg_class.oid = pg_class.relfilenode.
Here's the call stack in question:RelationBuildLocalRelation
heap_create
index_create
DefineIndex
ATExecAddIndexLooking at it again, it wouldn't bring the end of the world to add a relfilenode
argument to each. None of those have more than four callers.
Yeah. Those functions take an awful lot of arguments, which suggests
that some refactoring might be in order, but I still think it's
cleaner to add another argument than to change the state around
after-the-fact.
ATExecAddIndex()
would then call RelationPreserveStorage() before calling DefineIndex(), which
would in turn put things in a correct state from the start. Does that seem
appropriate? Offhand, I do like it better than what I had.
I wish we could avoid the whole death-and-resurrection thing
altogether, but off-hand I'm not seeing a real clean way to do that.
At the very least we had better comment it to death.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
On Wed, Jun 29, 2011 at 09:42:06AM -0400, Robert Haas wrote:
On Tue, Jun 28, 2011 at 3:40 PM, Noah Misch <noah@leadboat.com> wrote:
Here's the call stack in question:
? ? ? ?RelationBuildLocalRelation
? ? ? ?heap_create
? ? ? ?index_create
? ? ? ?DefineIndex
? ? ? ?ATExecAddIndexLooking at it again, it wouldn't bring the end of the world to add a relfilenode
argument to each. None of those have more than four callers.Yeah. Those functions take an awful lot of arguments, which suggests
that some refactoring might be in order, but I still think it's
cleaner to add another argument than to change the state around
after-the-fact.ATExecAddIndex()
would then call RelationPreserveStorage() before calling DefineIndex(), which
would in turn put things in a correct state from the start. ?Does that seem
appropriate? ?Offhand, I do like it better than what I had.I wish we could avoid the whole death-and-resurrection thing
altogether, but off-hand I'm not seeing a real clean way to do that.
At the very least we had better comment it to death.
I couldn't think of a massive amount to say about that, but see what you think
of this level of commentary.
Looking at this again turned up a live bug in the previous version: if the old
index file were created in the current transaction, we would wrongly remove its
delete-at-abort entry as well as its delete-at-commit entry. This leaked the
disk file. Fixed by adding an argument to RelationPreserveStorage() indicating
which kind to remove. Test case:
BEGIN;
CREATE TABLE t AS SELECT * FROM generate_series(1,100000) t(n);
CREATE INDEX ti ON t(n);
SELECT pg_relation_filepath('ti');
ALTER TABLE t ALTER n TYPE int;
ROLLBACK;
CHECKPOINT;
-- file named above should be gone
I also updated the ATPostAlterTypeCleanup() variable names per discussion and
moved IndexStmt.oldNode to a more-natural position in the structure.
Thanks,
nm
Attachments:
at-index-opfamily-v3.patchtext/plain; charset=us-asciiDownload
diff --git a/doc/src/sgml/xindex.sgml b/doc/src/sgml/xindex.sgml
index a90b4e2..b6fe908 100644
*** a/doc/src/sgml/xindex.sgml
--- b/doc/src/sgml/xindex.sgml
***************
*** 834,846 **** ALTER OPERATOR FAMILY integer_ops USING btree ADD
<para>
In a B-tree operator family, all the operators in the family must sort
compatibly, meaning that the transitive laws hold across all the data types
! supported by the family: <quote>if A = B and B = C, then A =
! C</>, and <quote>if A < B and B < C, then A < C</>. For each
! operator in the family there must be a support function having the same
! two input data types as the operator. It is recommended that a family be
! complete, i.e., for each combination of data types, all operators are
! included. Each operator class should include just the non-cross-type
! operators and support function for its data type.
</para>
<para>
--- 834,848 ----
<para>
In a B-tree operator family, all the operators in the family must sort
compatibly, meaning that the transitive laws hold across all the data types
! supported by the family: <quote>if A = B and B = C, then A = C</>,
! and <quote>if A < B and B < C, then A < C</>. Subjecting operands
! to any number of implicit casts or binary coercion casts involving only
! types represented in the family must not change the result other than to
! require a similar cast thereon. For each operator in the family there must
! be a support function having the same two input data types as the operator.
! It is recommended that a family be complete, i.e., for each combination of
! data types, all operators are included. Each operator class should include
! just the non-cross-type operators and support function for its data type.
</para>
<para>
***************
*** 851,861 **** ALTER OPERATOR FAMILY integer_ops USING btree ADD
by the family's equality operators, even when the values are of different
types. This is usually difficult to accomplish when the types have
different physical representations, but it can be done in some cases.
! Notice that there is only one support function per data type, not one
! per equality operator. It is recommended that a family be complete, i.e.,
! provide an equality operator for each combination of data types.
! Each operator class should include just the non-cross-type equality
! operator and the support function for its data type.
</para>
<para>
--- 853,865 ----
by the family's equality operators, even when the values are of different
types. This is usually difficult to accomplish when the types have
different physical representations, but it can be done in some cases.
! Implicit casts and binary coercion casts among types represented in the
! family must preserve this invariant. Notice that there is only one support
! function per data type, not one per equality operator. It is recommended
! that a family be complete, i.e., provide an equality operator for each
! combination of data types. Each operator class should include just the
! non-cross-type equality operator and the support function for its data
! type.
</para>
<para>
diff --git a/src/backend/bootstindex f3e85aa..d0a0e92 100644
*** a/src/backend/bootstrap/bootparse.y
--- b/src/backend/bootstrap/bootparse.y
***************
*** 217,222 **** Boot_CreateStmt:
--- 217,223 ----
PG_CATALOG_NAMESPACE,
shared_relation ? GLOBALTABLESPACE_OID : 0,
$3,
+ InvalidOid,
tupdesc,
RELKIND_RELATION,
RELPERSISTENCE_PERMANENT,
***************
*** 284,289 **** Boot_DeclareIndexStmt:
--- 285,291 ----
DefineIndex(makeRangeVar(NULL, $6, -1),
$3,
$4,
+ InvalidOid,
$8,
NULL,
$10,
***************
*** 302,307 **** Boot_DeclareUniqueIndexStmt:
--- 304,310 ----
DefineIndex(makeRangeVar(NULL, $7, -1),
$4,
$5,
+ InvalidOid,
$9,
NULL,
$11,
diff --git a/src/backend/catalog/heap.c index e606ac2..d50bbd3 100644
*** a/src/backend/catalog/heap.c
--- b/src/backend/catalog/heap.c
***************
*** 229,235 **** SystemAttributeByName(const char *attname, bool relhasoids)
* heap_create - Create an uncataloged heap relation
*
* Note API change: the caller must now always provide the OID
! * to use for the relation.
*
* rel->rd_rel is initialized by RelationBuildLocalRelation,
* and is mostly zeroes at return.
--- 229,236 ----
* heap_create - Create an uncataloged heap relation
*
* Note API change: the caller must now always provide the OID
! * to use for the relation. The relfilenode may (and, normally,
! * should) be left unspecified.
*
* rel->rd_rel is initialized by RelationBuildLocalRelation,
* and is mostly zeroes at return.
***************
*** 240,245 **** heap_create(const char *relname,
--- 241,247 ----
Oid relnamespace,
Oid reltablespace,
Oid relid,
+ Oid relfilenode,
TupleDesc tupDesc,
char relkind,
char relpersistence,
***************
*** 297,302 **** heap_create(const char *relname,
--- 299,314 ----
}
/*
+ * Unless otherwise requested, the physical ID (relfilenode) is initially
+ * the same as the logical ID (OID). When the caller did specify a
+ * relfilenode, it already exists; do not attempt to create it.
+ */
+ if (OidIsValid(relfilenode))
+ create_storage = false;
+ else
+ relfilenode = relid;
+
+ /*
* Never allow a pg_class entry to explicitly specify the database's
* default tablespace in reltablespace; force it to zero instead. This
* ensures that if the database is cloned with a different default
***************
*** 315,320 **** heap_create(const char *relname,
--- 327,333 ----
relnamespace,
tupDesc,
relid,
+ relfilenode,
reltablespace,
shared_relation,
mapped_relation,
***************
*** 1103,1108 **** heap_create_with_catalog(const char *relname,
--- 1116,1122 ----
relnamespace,
reltablespace,
relid,
+ InvalidOid,
tupdesc,
relkind,
relpersistence,
diff --git a/src/backend/catalog/index 39ba486..e32639e 100644
diff --git a/src/backend/catalog/sindex 57987be..d027361 100644
*** a/src/backend/catalog/storage.c
--- b/src/backend/catalog/storage.c
***************
*** 206,215 **** RelationDropStorage(Relation rel)
* The relation mapper fixes this by telling us to not delete such relations
* after all as part of its commit.
*
* No-op if the relation is not among those scheduled for deletion.
*/
void
! RelationPreserveStorage(RelFileNode rnode)
{
PendingRelDelete *pending;
PendingRelDelete *prev;
--- 206,218 ----
* The relation mapper fixes this by telling us to not delete such relations
* after all as part of its commit.
*
+ * We also use this to reuse an old build of an index during ALTER TABLE, this
+ * time removing the delete-at-commit entry.
+ *
* No-op if the relation is not among those scheduled for deletion.
*/
void
! RelationPreserveStorage(RelFileNode rnode, bool atCommit)
{
PendingRelDelete *pending;
PendingRelDelete *prev;
***************
*** 219,229 **** RelationPreserveStorage(RelFileNode rnode)
for (pending = pendingDeletes; pending != NULL; pending = next)
{
next = pending->next;
! if (RelFileNodeEquals(rnode, pending->relnode))
{
- /* we should only find delete-on-abort entries, else trouble */
- if (pending->atCommit)
- elog(ERROR, "cannot preserve a delete-on-commit relation");
/* unlink and delete list entry */
if (prev)
prev->next = next;
--- 222,230 ----
for (pending = pendingDeletes; pending != NULL; pending = next)
{
next = pending->next;
! if (RelFileNodeEquals(rnode, pending->relnode)
! && pending->atCommit == atCommit)
{
/* unlink and delete list entry */
if (prev)
prev->next = next;
diff --git a/src/backend/catalog/toaindex b059f9d..d4b1666 100644
*** a/src/backend/catalog/toasting.c
--- b/src/backend/catalog/toasting.c
***************
*** 273,279 **** create_toast_table(Relation rel, Oid toastOid, Oid toastIndexOid, Datum reloptio
coloptions[0] = 0;
coloptions[1] = 0;
! index_create(toast_rel, toast_idxname, toastIndexOid,
indexInfo,
list_make2("chunk_id", "chunk_seq"),
BTREE_AM_OID,
--- 273,279 ----
coloptions[0] = 0;
coloptions[1] = 0;
! index_create(toast_rel, toast_idxname, toastIndexOid, InvalidOid,
indexInfo,
list_make2("chunk_id", "chunk_seq"),
BTREE_AM_OID,
diff --git a/src/backend/commands/indindex b7c021d..ff4d1df 100644
*** a/src/backend/commands/indexcmds.c
--- b/src/backend/commands/indexcmds.c
***************
*** 73,78 **** static char *ChooseIndexNameAddition(List *colnames);
--- 73,264 ----
/*
+ * CheckIndexCompatible
+ * Determine whether an existing index definition is compatible with a
+ * prospective index definition, such that the existing index storage
+ * could become the storage of the new index, avoiding a rebuild.
+ *
+ * 'heapRelation': the relation the index would apply to.
+ * 'accessMethodName': name of the AM to use.
+ * 'attributeList': a list of IndexElem specifying columns and expressions
+ * to index on.
+ * 'exclusionOpNames': list of names of exclusion-constraint operators,
+ * or NIL if not an exclusion constraint.
+ *
+ * This is tailored to the needs of ALTER TABLE ALTER TYPE, which recreates
+ * any indexes that depended on a changing column from their pg_get_indexdef
+ * or pg_get_constraintdef definitions. We omit some of the sanity checks of
+ * DefineIndex. We assume that the old and new indexes have the same number
+ * of columns and that if one has an expression column or predicate, both do.
+ * Errors arising from the attribute list still apply.
+ *
+ * Most column type changes that can skip a table rewrite will not invalidate
+ * indexes. For btree and hash indexes, we assume continued validity when
+ * each column of an index would have the same operator family before and
+ * after the change. Since we do not document a contract for GIN or GiST
+ * operator families, we require an exact operator class match for them and
+ * for any other access methods.
+ *
+ * DefineIndex always verifies that each exclusion operator shares an operator
+ * family with its corresponding index operator class. For access methods
+ * having no operator family contract, confirm that the old and new indexes
+ * use the exact same exclusion operator. For btree and hash, there's nothing
+ * more to check.
+ *
+ * We do not yet implement a test to verify compatibility of expression
+ * columns or predicates, so assume any such index is incompatible.
+ */
+ bool
+ CheckIndexCompatible(Oid oldId,
+ RangeVar *heapRelation,
+ char *accessMethodName,
+ List *attributeList,
+ List *exclusionOpNames)
+ {
+ bool isconstraint;
+ Oid *collationObjectId;
+ Oid *classObjectId;
+ Oid accessMethodId;
+ Oid relationId;
+ HeapTuple tuple;
+ Form_pg_am accessMethodForm;
+ bool amcanorder;
+ RegProcedure amoptions;
+ int16 *coloptions;
+ IndexInfo *indexInfo;
+ int numberOfAttributes;
+ int old_natts;
+ bool isnull;
+ bool family_am;
+ bool ret = true;
+ oidvector *old_indclass;
+ oidvector *old_indcollation;
+ int i;
+ Datum d;
+
+ /* Caller should already have the relation locked in some way. */
+ relationId = RangeVarGetRelid(heapRelation, false);
+ /*
+ * We can pretend isconstraint = false unconditionally. It only serves to
+ * decide the text of an error message that should never happen for us.
+ */
+ isconstraint = false;
+
+ numberOfAttributes = list_length(attributeList);
+ Assert(numberOfAttributes > 0);
+ Assert(numberOfAttributes <= INDEX_MAX_KEYS);
+
+ /* look up the access method */
+ tuple = SearchSysCache1(AMNAME, PointerGetDatum(accessMethodName));
+ if (!HeapTupleIsValid(tuple))
+ ereport(ERROR,
+ (errcode(ERRCODE_UNDEFINED_OBJECT),
+ errmsg("access method \"%s\" does not exist",
+ accessMethodName)));
+ accessMethodId = HeapTupleGetOid(tuple);
+ accessMethodForm = (Form_pg_am) GETSTRUCT(tuple);
+ amcanorder = accessMethodForm->amcanorder;
+ amoptions = accessMethodForm->amoptions;
+ ReleaseSysCache(tuple);
+
+ indexInfo = makeNode(IndexInfo);
+ indexInfo->ii_Expressions = NIL;
+ indexInfo->ii_ExpressionsState = NIL;
+ indexInfo->ii_PredicateState = NIL;
+ indexInfo->ii_ExclusionOps = NULL;
+ indexInfo->ii_ExclusionProcs = NULL;
+ indexInfo->ii_ExclusionStrats = NULL;
+ collationObjectId = (Oid *) palloc(numberOfAttributes * sizeof(Oid));
+ classObjectId = (Oid *) palloc(numberOfAttributes * sizeof(Oid));
+ coloptions = (int16 *) palloc(numberOfAttributes * sizeof(int16));
+
+ ComputeIndexAttrs(indexInfo, collationObjectId, classObjectId,
+ coloptions, attributeList,
+ exclusionOpNames, relationId,
+ accessMethodName, accessMethodId,
+ amcanorder, isconstraint);
+
+
+ /* Get the soon-obsolete pg_index tuple. */
+ tuple = SearchSysCache1(INDEXRELID, ObjectIdGetDatum(oldId));
+ if (!HeapTupleIsValid(tuple))
+ elog(ERROR, "cache lookup failed for index %u", oldId);
+
+ /* We don't assess expressions or predicates; assume incompatibility. */
+ if (!(heap_attisnull(tuple, Anum_pg_index_indpred) &&
+ heap_attisnull(tuple, Anum_pg_index_indexprs)))
+ {
+ ReleaseSysCache(tuple);
+ return false;
+ }
+
+ /*
+ * If the old and new operator class of any index column differ in
+ * operator family or collation, regard the old index as incompatible.
+ * For access methods other than btree and hash, a family match has no
+ * defined meaning; require an exact operator class match.
+ */
+ old_natts = ((Form_pg_index) GETSTRUCT(tuple))->indnatts;
+ Assert(old_natts == numberOfAttributes);
+
+ d = SysCacheGetAttr(INDEXRELID, tuple, Anum_pg_index_indcollation, &isnull);
+ Assert(!isnull);
+ old_indcollation = (oidvector *) DatumGetPointer(d);
+
+ d = SysCacheGetAttr(INDEXRELID, tuple, Anum_pg_index_indclass, &isnull);
+ Assert(!isnull);
+ old_indclass = (oidvector *) DatumGetPointer(d);
+
+ family_am = accessMethodId == BTREE_AM_OID || accessMethodId == HASH_AM_OID;
+
+ for (i = 0; i < old_natts; i++)
+ {
+ Oid old_class = old_indclass->values[i];
+ Oid new_class = classObjectId[i];
+
+ if (!(old_indcollation->values[i] == collationObjectId[i]
+ && (old_class == new_class
+ || (family_am && (get_opclass_family(old_class)
+ == get_opclass_family(new_class))))))
+ {
+ ret = false;
+ break;
+ }
+ }
+
+ ReleaseSysCache(tuple);
+
+ /*
+ * For btree and hash, exclusion operators need only fall in the same
+ * operator family; ComputeIndexAttrs already verified that much. If we
+ * get this far, we know that the index operator family has not changed,
+ * and we're done. For other access methods, require exact matches for
+ * all exclusion operators.
+ */
+ if (ret && !family_am && indexInfo->ii_ExclusionOps != NULL)
+ {
+ Relation irel;
+ Oid *old_operators, *old_procs;
+ uint16 *old_strats;
+
+ /* Caller probably already holds a stronger lock. */
+ irel = index_open(oldId, AccessShareLock);
+ RelationGetExclusionInfo(irel, &old_operators, &old_procs, &old_strats);
+
+ for (i = 0; i < old_natts; i++)
+ if (old_operators[i] != indexInfo->ii_ExclusionOps[i])
+ {
+ ret = false;
+ break;
+ }
+
+ index_close(irel, NoLock);
+ }
+
+ return ret;
+ }
+
+ /*
* DefineIndex
* Creates a new index.
*
***************
*** 81,86 **** static char *ChooseIndexNameAddition(List *colnames);
--- 267,274 ----
* that a nonconflicting default name should be picked.
* 'indexRelationId': normally InvalidOid, but during bootstrap can be
* nonzero to specify a preselected OID for the index.
+ * 'relFileNode': normally InvalidOid, but can be nonzero to specify existing
+ * storage constituting a valid build of this index.
* 'accessMethodName': name of the AM to use.
* 'tableSpaceName': name of the tablespace to create the index in.
* NULL specifies using the appropriate default.
***************
*** 103,113 **** static char *ChooseIndexNameAddition(List *colnames);
* it will be filled later.
* 'quiet': suppress the NOTICE chatter ordinarily provided for constraints.
* 'concurrent': avoid blocking writers to the table while building.
*/
! void
DefineIndex(RangeVar *heapRelation,
char *indexRelationName,
Oid indexRelationId,
char *accessMethodName,
char *tableSpaceName,
List *attributeList,
--- 291,304 ----
* it will be filled later.
* 'quiet': suppress the NOTICE chatter ordinarily provided for constraints.
* 'concurrent': avoid blocking writers to the table while building.
+ *
+ * Returns the OID of the created index.
*/
! Oid
DefineIndex(RangeVar *heapRelation,
char *indexRelationName,
Oid indexRelationId,
+ Oid relFileNode,
char *accessMethodName,
char *tableSpaceName,
List *attributeList,
***************
*** 403,413 **** DefineIndex(RangeVar *heapRelation,
}
/*
* Make the catalog entries for the index, including constraints. Then, if
* not skip_build || concurrent, actually build the index.
*/
indexRelationId =
! index_create(rel, indexRelationName, indexRelationId,
indexInfo, indexColNames,
accessMethodId, tablespaceId,
collationObjectId, classObjectId,
--- 594,610 ----
}
/*
+ * A valid relFileNode implies that we already have a built form of the
+ * index. The caller should also decline any index build.
+ */
+ Assert(!OidIsValid(relFileNode) || (skip_build && !concurrent));
+
+ /*
* Make the catalog entries for the index, including constraints. Then, if
* not skip_build || concurrent, actually build the index.
*/
indexRelationId =
! index_create(rel, indexRelationName, indexRelationId, relFileNode,
indexInfo, indexColNames,
accessMethodId, tablespaceId,
collationObjectId, classObjectId,
***************
*** 421,427 **** DefineIndex(RangeVar *heapRelation,
{
/* Close the heap and we're done, in the non-concurrent case */
heap_close(rel, NoLock);
! return;
}
/* save lockrelid and locktag for below, then close rel */
--- 618,624 ----
{
/* Close the heap and we're done, in the non-concurrent case */
heap_close(rel, NoLock);
! return indexRelationId;
}
/* save lockrelid and locktag for below, then close rel */
***************
*** 709,714 **** DefineIndex(RangeVar *heapRelation,
--- 906,913 ----
* Last thing to do is release the session-level lock on the parent table.
*/
UnlockRelationIdForSession(&heaprelid, ShareUpdateExclusiveLock);
+
+ return indexRelationId;
}
diff --git a/src/backend/commands/tableindex b2ba11c..540055f 100644
*** a/src/backend/commands/tablecmds.c
--- b/src/backend/commands/tablecmds.c
***************
*** 345,351 **** static bool ATColumnChangeRequiresRewrite(Node *expr, AttrNumber varattno);
static void ATExecAlterColumnType(AlteredTableInfo *tab, Relation rel,
AlterTableCmd *cmd, LOCKMODE lockmode);
static void ATPostAlterTypeCleanup(List **wqueue, AlteredTableInfo *tab, LOCKMODE lockmode);
! static void ATPostAlterTypeParse(char *cmd, List **wqueue, LOCKMODE lockmode);
static void change_owner_recurse_to_sequences(Oid relationOid,
Oid newOwnerId, LOCKMODE lockmode);
static void ATExecClusterOn(Relation rel, const char *indexName, LOCKMODE lockmode);
--- 345,353 ----
static void ATExecAlterColumnType(AlteredTableInfo *tab, Relation rel,
AlterTableCmd *cmd, LOCKMODE lockmode);
static void ATPostAlterTypeCleanup(List **wqueue, AlteredTableInfo *tab, LOCKMODE lockmode);
! static void ATPostAlterTypeParse(Oid oldId, char *cmd,
! List **wqueue, LOCKMODE lockmode, bool rewrite);
! static void TryReuseIndex(Oid oldId, IndexStmt *stmt);
static void change_owner_recurse_to_sequences(Oid relationOid,
Oid newOwnerId, LOCKMODE lockmode);
static void ATExecClusterOn(Relation rel, const char *indexName, LOCKMODE lockmode);
***************
*** 5181,5217 **** ATExecAddIndex(AlteredTableInfo *tab, Relation rel,
bool check_rights;
bool skip_build;
bool quiet;
Assert(IsA(stmt, IndexStmt));
/* suppress schema rights check when rebuilding existing index */
check_rights = !is_rebuild;
! /* skip index build if phase 3 will have to rewrite table anyway */
! skip_build = tab->rewrite;
/* suppress notices when rebuilding existing index */
quiet = is_rebuild;
/* The IndexStmt has already been through transformIndexStmt */
! DefineIndex(stmt->relation, /* relation */
! stmt->idxname, /* index name */
! InvalidOid, /* no predefined OID */
! stmt->accessMethod, /* am name */
! stmt->tableSpace,
! stmt->indexParams, /* parameters */
! (Expr *) stmt->whereClause,
! stmt->options,
! stmt->excludeOpNames,
! stmt->unique,
! stmt->primary,
! stmt->isconstraint,
! stmt->deferrable,
! stmt->initdeferred,
! true, /* is_alter_table */
! check_rights,
! skip_build,
! quiet,
! false);
}
/*
--- 5183,5234 ----
bool check_rights;
bool skip_build;
bool quiet;
+ Oid new_index;
Assert(IsA(stmt, IndexStmt));
/* suppress schema rights check when rebuilding existing index */
check_rights = !is_rebuild;
! /* skip index build if phase 3 will do it or we're reusing an old one */
! skip_build = tab->rewrite || OidIsValid(stmt->oldNode);
/* suppress notices when rebuilding existing index */
quiet = is_rebuild;
/* The IndexStmt has already been through transformIndexStmt */
! new_index = DefineIndex(stmt->relation, /* relation */
! stmt->idxname, /* index name */
! InvalidOid, /* no predefined OID */
! stmt->oldNode,
! stmt->accessMethod, /* am name */
! stmt->tableSpace,
! stmt->indexParams, /* parameters */
! (Expr *) stmt->whereClause,
! stmt->options,
! stmt->excludeOpNames,
! stmt->unique,
! stmt->primary,
! stmt->isconstraint,
! stmt->deferrable,
! stmt->initdeferred,
! true, /* is_alter_table */
! check_rights,
! skip_build,
! quiet,
! false);
!
! /*
! * If TryReuseIndex() stashed a relfilenode for us, we used it for the new
! * index instead of building from scratch. The DROP of the old edition of
! * this index will have scheduled the storage for deletion at commit, so
! * cancel that pending deletion.
! */
! if (OidIsValid(stmt->oldNode))
! {
! Relation irel = index_open(new_index, NoLock);
! RelationPreserveStorage(irel->rd_node, true);
! index_close(irel, NoLock);
! }
}
/*
***************
*** 7177,7183 **** static void
ATPostAlterTypeCleanup(List **wqueue, AlteredTableInfo *tab, LOCKMODE lockmode)
{
ObjectAddress obj;
! ListCell *l;
/*
* Re-parse the index and constraint definitions, and attach them to the
--- 7194,7201 ----
ATPostAlterTypeCleanup(List **wqueue, AlteredTableInfo *tab, LOCKMODE lockmode)
{
ObjectAddress obj;
! ListCell *def_item;
! ListCell *oid_item;
/*
* Re-parse the index and constraint definitions, and attach them to the
***************
*** 7187,7196 **** ATPostAlterTypeCleanup(List **wqueue, AlteredTableInfo *tab, LOCKMODE lockmode)
* that before dropping. It's safe because the parser won't actually look
* at the catalogs to detect the existing entry.
*/
! foreach(l, tab->changedIndexDefs)
! ATPostAlterTypeParse((char *) lfirst(l), wqueue, lockmode);
! foreach(l, tab->changedConstraintDefs)
! ATPostAlterTypeParse((char *) lfirst(l), wqueue, lockmode);
/*
* Now we can drop the existing constraints and indexes --- constraints
--- 7205,7218 ----
* that before dropping. It's safe because the parser won't actually look
* at the catalogs to detect the existing entry.
*/
! forboth(oid_item, tab->changedConstraintOids,
! def_item, tab->changedConstraintDefs)
! ATPostAlterTypeParse(lfirst_oid(oid_item), (char *) lfirst(def_item),
! wqueue, lockmode, tab->rewrite);
! forboth(oid_item, tab->changedIndexOids,
! def_item, tab->changedIndexDefs)
! ATPostAlterTypeParse(lfirst_oid(oid_item), (char *) lfirst(def_item),
! wqueue, lockmode, tab->rewrite);
/*
* Now we can drop the existing constraints and indexes --- constraints
***************
*** 7200,7217 **** ATPostAlterTypeCleanup(List **wqueue, AlteredTableInfo *tab, LOCKMODE lockmode)
* should be okay to use DROP_RESTRICT here, since nothing else should be
* depending on these objects.
*/
! foreach(l, tab->changedConstraintOids)
{
obj.classId = ConstraintRelationId;
! obj.objectId = lfirst_oid(l);
obj.objectSubId = 0;
performDeletion(&obj, DROP_RESTRICT);
}
! foreach(l, tab->changedIndexOids)
{
obj.classId = RelationRelationId;
! obj.objectId = lfirst_oid(l);
obj.objectSubId = 0;
performDeletion(&obj, DROP_RESTRICT);
}
--- 7222,7239 ----
* should be okay to use DROP_RESTRICT here, since nothing else should be
* depending on these objects.
*/
! foreach(oid_item, tab->changedConstraintOids)
{
obj.classId = ConstraintRelationId;
! obj.objectId = lfirst_oid(oid_item);
obj.objectSubId = 0;
performDeletion(&obj, DROP_RESTRICT);
}
! foreach(oid_item, tab->changedIndexOids)
{
obj.classId = RelationRelationId;
! obj.objectId = lfirst_oid(oid_item);
obj.objectSubId = 0;
performDeletion(&obj, DROP_RESTRICT);
}
***************
*** 7223,7229 **** ATPostAlterTypeCleanup(List **wqueue, AlteredTableInfo *tab, LOCKMODE lockmode)
}
static void
! ATPostAlterTypeParse(char *cmd, List **wqueue, LOCKMODE lockmode)
{
List *raw_parsetree_list;
List *querytree_list;
--- 7245,7252 ----
}
static void
! ATPostAlterTypeParse(Oid oldId, char *cmd,
! List **wqueue, LOCKMODE lockmode, bool rewrite)
{
List *raw_parsetree_list;
List *querytree_list;
***************
*** 7270,7275 **** ATPostAlterTypeParse(char *cmd, List **wqueue, LOCKMODE lockmode)
--- 7293,7301 ----
IndexStmt *stmt = (IndexStmt *) stm;
AlterTableCmd *newcmd;
+ if (!rewrite)
+ TryReuseIndex(oldId, stmt);
+
rel = relation_openrv(stmt->relation, lockmode);
tab = ATGetQueueEntry(wqueue, rel);
newcmd = makeNode(AlterTableCmd);
***************
*** 7294,7299 **** ATPostAlterTypeParse(char *cmd, List **wqueue, LOCKMODE lockmode)
--- 7320,7329 ----
switch (cmd->subtype)
{
case AT_AddIndex:
+ Assert(IsA(cmd->def, IndexStmt));
+ if (!rewrite)
+ TryReuseIndex(get_constraint_index(oldId),
+ (IndexStmt *) cmd->def);
cmd->subtype = AT_ReAddIndex;
tab->subcmds[AT_PASS_OLD_INDEX] =
lappend(tab->subcmds[AT_PASS_OLD_INDEX], cmd);
***************
*** 7317,7322 **** ATPostAlterTypeParse(char *cmd, List **wqueue, LOCKMODE lockmode)
--- 7347,7372 ----
}
}
+ /*
+ * Subroutine for ATPostAlterTypeParse(). Calls out to CheckIndexCompatible()
+ * for the real analysis, then mutates the IndexStmt based on that verdict.
+ */
+ static void
+ TryReuseIndex(Oid oldId, IndexStmt *stmt)
+ {
+
+ if (CheckIndexCompatible(oldId,
+ stmt->relation,
+ stmt->accessMethod,
+ stmt->indexParams,
+ stmt->excludeOpNames))
+ {
+ Relation irel = index_open(oldId, NoLock);
+ stmt->oldNode = irel->rd_node.relNode;
+ index_close(irel, NoLock);
+ }
+ }
+
/*
* ALTER TABLE OWNER
diff --git a/src/backend/nodes/copyfuncindex c9133dd..7a51456 100644
*** a/src/backend/nodes/copyfuncs.c
--- b/src/backend/nodes/copyfuncs.c
***************
*** 2807,2812 **** _copyIndexStmt(IndexStmt *from)
--- 2807,2813 ----
COPY_NODE_FIELD(whereClause);
COPY_NODE_FIELD(excludeOpNames);
COPY_SCALAR_FIELD(indexOid);
+ COPY_SCALAR_FIELD(oldNode);
COPY_SCALAR_FIELD(unique);
COPY_SCALAR_FIELD(primary);
COPY_SCALAR_FIELD(isconstraint);
diff --git a/src/backend/nodes/equalindex 3a0267c..4052a9a 100644
*** a/src/backend/nodes/equalfuncs.c
--- b/src/backend/nodes/equalfuncs.c
***************
*** 1245,1250 **** _equalIndexStmt(IndexStmt *a, IndexStmt *b)
--- 1245,1251 ----
COMPARE_NODE_FIELD(whereClause);
COMPARE_NODE_FIELD(excludeOpNames);
COMPARE_SCALAR_FIELD(indexOid);
+ COMPARE_SCALAR_FIELD(oldNode);
COMPARE_SCALAR_FIELD(unique);
COMPARE_SCALAR_FIELD(primary);
COMPARE_SCALAR_FIELD(isconstraint);
diff --git a/src/backend/nodes/outfunindex 681f5f8..b5be09a 100644
*** a/src/backend/nodes/outfuncs.c
--- b/src/backend/nodes/outfuncs.c
***************
*** 1976,1981 **** _outIndexStmt(StringInfo str, IndexStmt *node)
--- 1976,1982 ----
WRITE_NODE_FIELD(whereClause);
WRITE_NODE_FIELD(excludeOpNames);
WRITE_OID_FIELD(indexOid);
+ WRITE_OID_FIELD(oldNode);
WRITE_BOOL_FIELD(unique);
WRITE_BOOL_FIELD(primary);
WRITE_BOOL_FIELD(isconstraint);
diff --git a/src/backend/tcop/utiliindex 0559998..6a06764 100644
*** a/src/backend/tcop/utility.c
--- b/src/backend/tcop/utility.c
***************
*** 941,946 **** standard_ProcessUtility(Node *parsetree,
--- 941,947 ----
DefineIndex(stmt->relation, /* relation */
stmt->idxname, /* index name */
InvalidOid, /* no predefined OID */
+ InvalidOid, /* no previous storage */
stmt->accessMethod, /* am name */
stmt->tableSpace,
stmt->indexParams, /* parameters */
diff --git a/src/backend/utils/caindex 1a400a0..87a6356 100644
*** a/src/backend/utils/cache/relcache.c
--- b/src/backend/utils/cache/relcache.c
***************
*** 2395,2400 **** RelationBuildLocalRelation(const char *relname,
--- 2395,2401 ----
Oid relnamespace,
TupleDesc tupDesc,
Oid relid,
+ Oid relfilenode,
Oid reltablespace,
bool shared_relation,
bool mapped_relation,
***************
*** 2529,2538 **** RelationBuildLocalRelation(const char *relname,
/*
* Insert relation physical and logical identifiers (OIDs) into the right
! * places. Note that the physical ID (relfilenode) is initially the same
! * as the logical ID (OID); except that for a mapped relation, we set
! * relfilenode to zero and rely on RelationInitPhysicalAddr to consult the
! * map.
*/
rel->rd_rel->relisshared = shared_relation;
--- 2530,2537 ----
/*
* Insert relation physical and logical identifiers (OIDs) into the right
! * places. For a mapped relation, we set relfilenode to zero and rely on
! * RelationInitPhysicalAddr to consult the map.
*/
rel->rd_rel->relisshared = shared_relation;
***************
*** 2547,2556 **** RelationBuildLocalRelation(const char *relname,
{
rel->rd_rel->relfilenode = InvalidOid;
/* Add it to the active mapping information */
! RelationMapUpdateMap(relid, relid, shared_relation, true);
}
else
! rel->rd_rel->relfilenode = relid;
RelationInitLockInfo(rel); /* see lmgr.c */
--- 2546,2555 ----
{
rel->rd_rel->relfilenode = InvalidOid;
/* Add it to the active mapping information */
! RelationMapUpdateMap(relid, relfilenode, shared_relation, true);
}
else
! rel->rd_rel->relfilenode = relfilenode;
RelationInitLockInfo(rel); /* see lmgr.c */
diff --git a/src/backend/utils/cache/relmindex a19ee28..b04dc9e 100644
*** a/src/backend/utils/cache/relmapper.c
--- b/src/backend/utils/cache/relmapper.c
***************
*** 792,798 **** write_relmap_file(bool shared, RelMapFile *newmap,
rnode.spcNode = tsid;
rnode.dbNode = dbid;
rnode.relNode = newmap->mappings[i].mapfilenode;
! RelationPreserveStorage(rnode);
}
}
--- 792,798 ----
rnode.spcNode = tsid;
rnode.dbNode = dbid;
rnode.relNode = newmap->mappings[i].mapfilenode;
! RelationPreserveStorage(rnode, false);
}
}
diff --git a/src/include/catalog/heap.h b/index c95e913..a7d58ea 100644
*** a/src/include/catalog/heap.h
--- b/src/include/catalog/heap.h
***************
*** 38,43 **** extern Relation heap_create(const char *relname,
--- 38,44 ----
Oid relnamespace,
Oid reltablespace,
Oid relid,
+ Oid relfilenode,
TupleDesc tupDesc,
char relkind,
char relpersistence,
diff --git a/src/include/catalog/index 071db7f..8b78b05 100644
*** a/src/include/catalog/index.h
--- b/src/include/catalog/index.h
***************
*** 35,40 **** extern void index_check_primary_key(Relation heapRel,
--- 35,41 ----
extern Oid index_create(Relation heapRelation,
const char *indexRelationName,
Oid indexRelationId,
+ Oid relFileNode,
IndexInfo *indexInfo,
List *indexColNames,
Oid accessMethodObjectId,
diff --git a/src/include/catalog/sindex 8dee8cf..6907d83 100644
*** a/src/include/catalog/storage.h
--- b/src/include/catalog/storage.h
***************
*** 22,28 ****
extern void RelationCreateStorage(RelFileNode rnode, char relpersistence);
extern void RelationDropStorage(Relation rel);
! extern void RelationPreserveStorage(RelFileNode rnode);
extern void RelationTruncate(Relation rel, BlockNumber nblocks);
/*
--- 22,28 ----
extern void RelationCreateStorage(RelFileNode rnode, char relpersistence);
extern void RelationDropStorage(Relation rel);
! extern void RelationPreserveStorage(RelFileNode rnode, bool atCommit);
extern void RelationTruncate(Relation rel, BlockNumber nblocks);
/*
diff --git a/src/include/commands/deindex bbc024f..81c515e 100644
*** a/src/include/commands/defrem.h
--- b/src/include/commands/defrem.h
***************
*** 18,26 ****
/* commands/indexcmds.c */
! extern void DefineIndex(RangeVar *heapRelation,
char *indexRelationName,
Oid indexRelationId,
char *accessMethodName,
char *tableSpaceName,
List *attributeList,
--- 18,27 ----
/* commands/indexcmds.c */
! extern Oid DefineIndex(RangeVar *heapRelation,
char *indexRelationName,
Oid indexRelationId,
+ Oid relFileNode,
char *accessMethodName,
char *tableSpaceName,
List *attributeList,
***************
*** 49,54 **** extern char *ChooseIndexName(const char *tabname, Oid namespaceId,
--- 50,60 ----
List *colnames, List *exclusionOpNames,
bool primary, bool isconstraint);
extern List *ChooseIndexColumnNames(List *indexElems);
+ extern bool CheckIndexCompatible(Oid oldId,
+ RangeVar *heapRelation,
+ char *accessMethodName,
+ List *attributeList,
+ List *exclusionOpNames);
extern Oid GetDefaultOpClass(Oid type_id, Oid am_id);
/* commands/functioncmds.c */
diff --git a/src/include/nodes/parseindex c65e3cd..c09efc0 100644
*** a/src/include/nodes/parsenodes.h
--- b/src/include/nodes/parsenodes.h
***************
*** 2059,2064 **** typedef struct IndexStmt
--- 2059,2065 ----
Node *whereClause; /* qualification (partial-index predicate) */
List *excludeOpNames; /* exclusion operator names, or NIL if none */
Oid indexOid; /* OID of an existing index, if any */
+ Oid oldNode; /* relfilenode of my former self */
bool unique; /* is index unique? */
bool primary; /* is index on primary key? */
bool isconstraint; /* is it from a CONSTRAINT clause? */
diff --git a/src/include/utils/relcacindex 1f4def5..ef1692c 100644
*** a/src/include/utils/relcache.h
--- b/src/include/utils/relcache.h
***************
*** 67,72 **** extern Relation RelationBuildLocalRelation(const char *relname,
--- 67,73 ----
Oid relnamespace,
TupleDesc tupDesc,
Oid relid,
+ Oid relfilenode,
Oid reltablespace,
bool shared_relation,
bool mapped_relation,
diff --git a/src/test/regress/GNUmaindex 90aea6c..949f82b 100644
diff --git a/src/test/regress/expecnew file mode 100644
index 0000000..44df2ea
diff --git a/src/test/regress/parallel_schedule b/srindex 376f28d..e3b515e 100644
diff --git a/src/test/regress/serial_scheindex bb654f9..a7e6dc6 100644
diff --git a/src/test/regress/sql/big_anew file mode 100644
index 0000000..6ae9007
On Thu, Jun 30, 2011 at 11:55 AM, Noah Misch <noah@leadboat.com> wrote:
On Wed, Jun 29, 2011 at 09:42:06AM -0400, Robert Haas wrote:
On Tue, Jun 28, 2011 at 3:40 PM, Noah Misch <noah@leadboat.com> wrote:
Here's the call stack in question:
? ? ? ?RelationBuildLocalRelation
? ? ? ?heap_create
? ? ? ?index_create
? ? ? ?DefineIndex
? ? ? ?ATExecAddIndexLooking at it again, it wouldn't bring the end of the world to add a relfilenode
argument to each. None of those have more than four callers.Yeah. Those functions take an awful lot of arguments, which suggests
that some refactoring might be in order, but I still think it's
cleaner to add another argument than to change the state around
after-the-fact.ATExecAddIndex()
would then call RelationPreserveStorage() before calling DefineIndex(), which
would in turn put things in a correct state from the start. ?Does that seem
appropriate? ?Offhand, I do like it better than what I had.I wish we could avoid the whole death-and-resurrection thing
altogether, but off-hand I'm not seeing a real clean way to do that.
At the very least we had better comment it to death.I couldn't think of a massive amount to say about that, but see what you think
of this level of commentary.Looking at this again turned up a live bug in the previous version: if the old
index file were created in the current transaction, we would wrongly remove its
delete-at-abort entry as well as its delete-at-commit entry. This leaked the
disk file. Fixed by adding an argument to RelationPreserveStorage() indicating
which kind to remove. Test case:BEGIN;
CREATE TABLE t AS SELECT * FROM generate_series(1,100000) t(n);
CREATE INDEX ti ON t(n);
SELECT pg_relation_filepath('ti');
ALTER TABLE t ALTER n TYPE int;
ROLLBACK;
CHECKPOINT;
-- file named above should be goneI also updated the ATPostAlterTypeCleanup() variable names per discussion and
moved IndexStmt.oldNode to a more-natural position in the structure.
On first blush, that looks a whole lot cleaner. I'll try to find some
time for a more detailed review soon.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
On Thu, Jun 30, 2011 at 1:02 PM, Robert Haas <robertmhaas@gmail.com> wrote:
On first blush, that looks a whole lot cleaner. I'll try to find some
time for a more detailed review soon.
This seems not to compile for me:
gcc -O2 -Wall -Wmissing-prototypes -Wpointer-arith
-Wdeclaration-after-statement -Wendif-labels -Wformat-security
-fno-strict-aliasing -fwrapv -g -Werror -I../../../src/include
-I/opt/local/include -c -o index.o index.c -MMD -MP -MF
.deps/index.Po
index.c:692: error: conflicting types for ‘index_create’
../../../src/include/catalog/index.h:53: error: previous declaration
of ‘index_create’ was here
cc1: warnings being treated as errors
index.c: In function ‘index_create’:
index.c:821: warning: passing argument 5 of ‘heap_create’ makes
integer from pointer without a cast
index.c:821: warning: passing argument 6 of ‘heap_create’ makes
pointer from integer without a cast
index.c:821: error: too few arguments to function ‘heap_create’
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
On Wed, Jul 06, 2011 at 09:55:01AM -0400, Robert Haas wrote:
On Thu, Jun 30, 2011 at 1:02 PM, Robert Haas <robertmhaas@gmail.com> wrote:
On first blush, that looks a whole lot cleaner. ?I'll try to find some
time for a more detailed review soon.This seems not to compile for me:
gcc -O2 -Wall -Wmissing-prototypes -Wpointer-arith
-Wdeclaration-after-statement -Wendif-labels -Wformat-security
-fno-strict-aliasing -fwrapv -g -Werror -I../../../src/include
-I/opt/local/include -c -o index.o index.c -MMD -MP -MF
.deps/index.Po
index.c:692: error: conflicting types for ?index_create?
../../../src/include/catalog/index.h:53: error: previous declaration
of ?index_create? was here
cc1: warnings being treated as errors
index.c: In function ?index_create?:
index.c:821: warning: passing argument 5 of ?heap_create? makes
integer from pointer without a cast
index.c:821: warning: passing argument 6 of ?heap_create? makes
pointer from integer without a cast
index.c:821: error: too few arguments to function ?heap_create?
Drat; fixed in this version. My local branches contain a large test battery
that I filter out of the diff before posting. This time, that filter also
removed an essential part of the patch.
Thanks,
nm
Attachments:
at-index-opfamily-v4.patchtext/plain; charset=us-asciiDownload
diff --git a/doc/src/sgml/xindex.sgml b/doc/src/sgml/xindex.sgml
index a90b4e2..b6fe908 100644
*** a/doc/src/sgml/xindex.sgml
--- b/doc/src/sgml/xindex.sgml
***************
*** 834,846 **** ALTER OPERATOR FAMILY integer_ops USING btree ADD
<para>
In a B-tree operator family, all the operators in the family must sort
compatibly, meaning that the transitive laws hold across all the data types
! supported by the family: <quote>if A = B and B = C, then A =
! C</>, and <quote>if A < B and B < C, then A < C</>. For each
! operator in the family there must be a support function having the same
! two input data types as the operator. It is recommended that a family be
! complete, i.e., for each combination of data types, all operators are
! included. Each operator class should include just the non-cross-type
! operators and support function for its data type.
</para>
<para>
--- 834,848 ----
<para>
In a B-tree operator family, all the operators in the family must sort
compatibly, meaning that the transitive laws hold across all the data types
! supported by the family: <quote>if A = B and B = C, then A = C</>,
! and <quote>if A < B and B < C, then A < C</>. Subjecting operands
! to any number of implicit casts or binary coercion casts involving only
! types represented in the family must not change the result other than to
! require a similar cast thereon. For each operator in the family there must
! be a support function having the same two input data types as the operator.
! It is recommended that a family be complete, i.e., for each combination of
! data types, all operators are included. Each operator class should include
! just the non-cross-type operators and support function for its data type.
</para>
<para>
***************
*** 851,861 **** ALTER OPERATOR FAMILY integer_ops USING btree ADD
by the family's equality operators, even when the values are of different
types. This is usually difficult to accomplish when the types have
different physical representations, but it can be done in some cases.
! Notice that there is only one support function per data type, not one
! per equality operator. It is recommended that a family be complete, i.e.,
! provide an equality operator for each combination of data types.
! Each operator class should include just the non-cross-type equality
! operator and the support function for its data type.
</para>
<para>
--- 853,865 ----
by the family's equality operators, even when the values are of different
types. This is usually difficult to accomplish when the types have
different physical representations, but it can be done in some cases.
! Implicit casts and binary coercion casts among types represented in the
! family must preserve this invariant. Notice that there is only one support
! function per data type, not one per equality operator. It is recommended
! that a family be complete, i.e., provide an equality operator for each
! combination of data types. Each operator class should include just the
! non-cross-type equality operator and the support function for its data
! type.
</para>
<para>
diff --git a/src/backend/bootstindex f3e85aa..d0a0e92 100644
*** a/src/backend/bootstrap/bootparse.y
--- b/src/backend/bootstrap/bootparse.y
***************
*** 217,222 **** Boot_CreateStmt:
--- 217,223 ----
PG_CATALOG_NAMESPACE,
shared_relation ? GLOBALTABLESPACE_OID : 0,
$3,
+ InvalidOid,
tupdesc,
RELKIND_RELATION,
RELPERSISTENCE_PERMANENT,
***************
*** 284,289 **** Boot_DeclareIndexStmt:
--- 285,291 ----
DefineIndex(makeRangeVar(NULL, $6, -1),
$3,
$4,
+ InvalidOid,
$8,
NULL,
$10,
***************
*** 302,307 **** Boot_DeclareUniqueIndexStmt:
--- 304,310 ----
DefineIndex(makeRangeVar(NULL, $7, -1),
$4,
$5,
+ InvalidOid,
$9,
NULL,
$11,
diff --git a/src/backend/catalog/heap.c index a8c2700..45ccb04 100644
*** a/src/backend/catalog/heap.c
--- b/src/backend/catalog/heap.c
***************
*** 229,235 **** SystemAttributeByName(const char *attname, bool relhasoids)
* heap_create - Create an uncataloged heap relation
*
* Note API change: the caller must now always provide the OID
! * to use for the relation.
*
* rel->rd_rel is initialized by RelationBuildLocalRelation,
* and is mostly zeroes at return.
--- 229,236 ----
* heap_create - Create an uncataloged heap relation
*
* Note API change: the caller must now always provide the OID
! * to use for the relation. The relfilenode may (and, normally,
! * should) be left unspecified.
*
* rel->rd_rel is initialized by RelationBuildLocalRelation,
* and is mostly zeroes at return.
***************
*** 240,245 **** heap_create(const char *relname,
--- 241,247 ----
Oid relnamespace,
Oid reltablespace,
Oid relid,
+ Oid relfilenode,
TupleDesc tupDesc,
char relkind,
char relpersistence,
***************
*** 297,302 **** heap_create(const char *relname,
--- 299,314 ----
}
/*
+ * Unless otherwise requested, the physical ID (relfilenode) is initially
+ * the same as the logical ID (OID). When the caller did specify a
+ * relfilenode, it already exists; do not attempt to create it.
+ */
+ if (OidIsValid(relfilenode))
+ create_storage = false;
+ else
+ relfilenode = relid;
+
+ /*
* Never allow a pg_class entry to explicitly specify the database's
* default tablespace in reltablespace; force it to zero instead. This
* ensures that if the database is cloned with a different default
***************
*** 315,320 **** heap_create(const char *relname,
--- 327,333 ----
relnamespace,
tupDesc,
relid,
+ relfilenode,
reltablespace,
shared_relation,
mapped_relation,
***************
*** 1103,1108 **** heap_create_with_catalog(const char *relname,
--- 1116,1122 ----
relnamespace,
reltablespace,
relid,
+ InvalidOid,
tupdesc,
relkind,
relpersistence,
diff --git a/src/backend/catalog/index 6fc234b..e32639e 100644
*** a/src/backend/catalog/index.c
--- b/src/backend/catalog/index.c
***************
*** 649,654 **** UpdateIndexRelation(Oid indexoid,
--- 649,656 ----
* indexRelationId: normally, pass InvalidOid to let this routine
* generate an OID for the index. During bootstrap this may be
* nonzero to specify a preselected OID.
+ * relFileNode: normally, pass InvalidOid to get new storage. May be
+ * nonzero to attach an existing valid build.
* indexInfo: same info executor uses to insert into the index
* indexColNames: column names to use for index (List of char *)
* accessMethodObjectId: OID of index AM to use
***************
*** 674,679 **** Oid
--- 676,682 ----
index_create(Relation heapRelation,
const char *indexRelationName,
Oid indexRelationId,
+ Oid relFileNode,
IndexInfo *indexInfo,
List *indexColNames,
Oid accessMethodObjectId,
***************
*** 813,818 **** index_create(Relation heapRelation,
--- 816,822 ----
namespaceId,
tableSpaceId,
indexRelationId,
+ relFileNode,
indexTupDesc,
RELKIND_INDEX,
relpersistence,
diff --git a/src/backend/catalog/sindex 57987be..d027361 100644
*** a/src/backend/catalog/storage.c
--- b/src/backend/catalog/storage.c
***************
*** 206,215 **** RelationDropStorage(Relation rel)
* The relation mapper fixes this by telling us to not delete such relations
* after all as part of its commit.
*
* No-op if the relation is not among those scheduled for deletion.
*/
void
! RelationPreserveStorage(RelFileNode rnode)
{
PendingRelDelete *pending;
PendingRelDelete *prev;
--- 206,218 ----
* The relation mapper fixes this by telling us to not delete such relations
* after all as part of its commit.
*
+ * We also use this to reuse an old build of an index during ALTER TABLE, this
+ * time removing the delete-at-commit entry.
+ *
* No-op if the relation is not among those scheduled for deletion.
*/
void
! RelationPreserveStorage(RelFileNode rnode, bool atCommit)
{
PendingRelDelete *pending;
PendingRelDelete *prev;
***************
*** 219,229 **** RelationPreserveStorage(RelFileNode rnode)
for (pending = pendingDeletes; pending != NULL; pending = next)
{
next = pending->next;
! if (RelFileNodeEquals(rnode, pending->relnode))
{
- /* we should only find delete-on-abort entries, else trouble */
- if (pending->atCommit)
- elog(ERROR, "cannot preserve a delete-on-commit relation");
/* unlink and delete list entry */
if (prev)
prev->next = next;
--- 222,230 ----
for (pending = pendingDeletes; pending != NULL; pending = next)
{
next = pending->next;
! if (RelFileNodeEquals(rnode, pending->relnode)
! && pending->atCommit == atCommit)
{
/* unlink and delete list entry */
if (prev)
prev->next = next;
diff --git a/src/backend/catalog/toaindex ce082fd..a09a3ad 100644
*** a/src/backend/catalog/toasting.c
--- b/src/backend/catalog/toasting.c
***************
*** 274,280 **** create_toast_table(Relation rel, Oid toastOid, Oid toastIndexOid, Datum reloptio
coloptions[0] = 0;
coloptions[1] = 0;
! index_create(toast_rel, toast_idxname, toastIndexOid,
indexInfo,
list_make2("chunk_id", "chunk_seq"),
BTREE_AM_OID,
--- 274,280 ----
coloptions[0] = 0;
coloptions[1] = 0;
! index_create(toast_rel, toast_idxname, toastIndexOid, InvalidOid,
indexInfo,
list_make2("chunk_id", "chunk_seq"),
BTREE_AM_OID,
diff --git a/src/backend/commands/indindex b7c021d..ff4d1df 100644
*** a/src/backend/commands/indexcmds.c
--- b/src/backend/commands/indexcmds.c
***************
*** 73,78 **** static char *ChooseIndexNameAddition(List *colnames);
--- 73,264 ----
/*
+ * CheckIndexCompatible
+ * Determine whether an existing index definition is compatible with a
+ * prospective index definition, such that the existing index storage
+ * could become the storage of the new index, avoiding a rebuild.
+ *
+ * 'heapRelation': the relation the index would apply to.
+ * 'accessMethodName': name of the AM to use.
+ * 'attributeList': a list of IndexElem specifying columns and expressions
+ * to index on.
+ * 'exclusionOpNames': list of names of exclusion-constraint operators,
+ * or NIL if not an exclusion constraint.
+ *
+ * This is tailored to the needs of ALTER TABLE ALTER TYPE, which recreates
+ * any indexes that depended on a changing column from their pg_get_indexdef
+ * or pg_get_constraintdef definitions. We omit some of the sanity checks of
+ * DefineIndex. We assume that the old and new indexes have the same number
+ * of columns and that if one has an expression column or predicate, both do.
+ * Errors arising from the attribute list still apply.
+ *
+ * Most column type changes that can skip a table rewrite will not invalidate
+ * indexes. For btree and hash indexes, we assume continued validity when
+ * each column of an index would have the same operator family before and
+ * after the change. Since we do not document a contract for GIN or GiST
+ * operator families, we require an exact operator class match for them and
+ * for any other access methods.
+ *
+ * DefineIndex always verifies that each exclusion operator shares an operator
+ * family with its corresponding index operator class. For access methods
+ * having no operator family contract, confirm that the old and new indexes
+ * use the exact same exclusion operator. For btree and hash, there's nothing
+ * more to check.
+ *
+ * We do not yet implement a test to verify compatibility of expression
+ * columns or predicates, so assume any such index is incompatible.
+ */
+ bool
+ CheckIndexCompatible(Oid oldId,
+ RangeVar *heapRelation,
+ char *accessMethodName,
+ List *attributeList,
+ List *exclusionOpNames)
+ {
+ bool isconstraint;
+ Oid *collationObjectId;
+ Oid *classObjectId;
+ Oid accessMethodId;
+ Oid relationId;
+ HeapTuple tuple;
+ Form_pg_am accessMethodForm;
+ bool amcanorder;
+ RegProcedure amoptions;
+ int16 *coloptions;
+ IndexInfo *indexInfo;
+ int numberOfAttributes;
+ int old_natts;
+ bool isnull;
+ bool family_am;
+ bool ret = true;
+ oidvector *old_indclass;
+ oidvector *old_indcollation;
+ int i;
+ Datum d;
+
+ /* Caller should already have the relation locked in some way. */
+ relationId = RangeVarGetRelid(heapRelation, false);
+ /*
+ * We can pretend isconstraint = false unconditionally. It only serves to
+ * decide the text of an error message that should never happen for us.
+ */
+ isconstraint = false;
+
+ numberOfAttributes = list_length(attributeList);
+ Assert(numberOfAttributes > 0);
+ Assert(numberOfAttributes <= INDEX_MAX_KEYS);
+
+ /* look up the access method */
+ tuple = SearchSysCache1(AMNAME, PointerGetDatum(accessMethodName));
+ if (!HeapTupleIsValid(tuple))
+ ereport(ERROR,
+ (errcode(ERRCODE_UNDEFINED_OBJECT),
+ errmsg("access method \"%s\" does not exist",
+ accessMethodName)));
+ accessMethodId = HeapTupleGetOid(tuple);
+ accessMethodForm = (Form_pg_am) GETSTRUCT(tuple);
+ amcanorder = accessMethodForm->amcanorder;
+ amoptions = accessMethodForm->amoptions;
+ ReleaseSysCache(tuple);
+
+ indexInfo = makeNode(IndexInfo);
+ indexInfo->ii_Expressions = NIL;
+ indexInfo->ii_ExpressionsState = NIL;
+ indexInfo->ii_PredicateState = NIL;
+ indexInfo->ii_ExclusionOps = NULL;
+ indexInfo->ii_ExclusionProcs = NULL;
+ indexInfo->ii_ExclusionStrats = NULL;
+ collationObjectId = (Oid *) palloc(numberOfAttributes * sizeof(Oid));
+ classObjectId = (Oid *) palloc(numberOfAttributes * sizeof(Oid));
+ coloptions = (int16 *) palloc(numberOfAttributes * sizeof(int16));
+
+ ComputeIndexAttrs(indexInfo, collationObjectId, classObjectId,
+ coloptions, attributeList,
+ exclusionOpNames, relationId,
+ accessMethodName, accessMethodId,
+ amcanorder, isconstraint);
+
+
+ /* Get the soon-obsolete pg_index tuple. */
+ tuple = SearchSysCache1(INDEXRELID, ObjectIdGetDatum(oldId));
+ if (!HeapTupleIsValid(tuple))
+ elog(ERROR, "cache lookup failed for index %u", oldId);
+
+ /* We don't assess expressions or predicates; assume incompatibility. */
+ if (!(heap_attisnull(tuple, Anum_pg_index_indpred) &&
+ heap_attisnull(tuple, Anum_pg_index_indexprs)))
+ {
+ ReleaseSysCache(tuple);
+ return false;
+ }
+
+ /*
+ * If the old and new operator class of any index column differ in
+ * operator family or collation, regard the old index as incompatible.
+ * For access methods other than btree and hash, a family match has no
+ * defined meaning; require an exact operator class match.
+ */
+ old_natts = ((Form_pg_index) GETSTRUCT(tuple))->indnatts;
+ Assert(old_natts == numberOfAttributes);
+
+ d = SysCacheGetAttr(INDEXRELID, tuple, Anum_pg_index_indcollation, &isnull);
+ Assert(!isnull);
+ old_indcollation = (oidvector *) DatumGetPointer(d);
+
+ d = SysCacheGetAttr(INDEXRELID, tuple, Anum_pg_index_indclass, &isnull);
+ Assert(!isnull);
+ old_indclass = (oidvector *) DatumGetPointer(d);
+
+ family_am = accessMethodId == BTREE_AM_OID || accessMethodId == HASH_AM_OID;
+
+ for (i = 0; i < old_natts; i++)
+ {
+ Oid old_class = old_indclass->values[i];
+ Oid new_class = classObjectId[i];
+
+ if (!(old_indcollation->values[i] == collationObjectId[i]
+ && (old_class == new_class
+ || (family_am && (get_opclass_family(old_class)
+ == get_opclass_family(new_class))))))
+ {
+ ret = false;
+ break;
+ }
+ }
+
+ ReleaseSysCache(tuple);
+
+ /*
+ * For btree and hash, exclusion operators need only fall in the same
+ * operator family; ComputeIndexAttrs already verified that much. If we
+ * get this far, we know that the index operator family has not changed,
+ * and we're done. For other access methods, require exact matches for
+ * all exclusion operators.
+ */
+ if (ret && !family_am && indexInfo->ii_ExclusionOps != NULL)
+ {
+ Relation irel;
+ Oid *old_operators, *old_procs;
+ uint16 *old_strats;
+
+ /* Caller probably already holds a stronger lock. */
+ irel = index_open(oldId, AccessShareLock);
+ RelationGetExclusionInfo(irel, &old_operators, &old_procs, &old_strats);
+
+ for (i = 0; i < old_natts; i++)
+ if (old_operators[i] != indexInfo->ii_ExclusionOps[i])
+ {
+ ret = false;
+ break;
+ }
+
+ index_close(irel, NoLock);
+ }
+
+ return ret;
+ }
+
+ /*
* DefineIndex
* Creates a new index.
*
***************
*** 81,86 **** static char *ChooseIndexNameAddition(List *colnames);
--- 267,274 ----
* that a nonconflicting default name should be picked.
* 'indexRelationId': normally InvalidOid, but during bootstrap can be
* nonzero to specify a preselected OID for the index.
+ * 'relFileNode': normally InvalidOid, but can be nonzero to specify existing
+ * storage constituting a valid build of this index.
* 'accessMethodName': name of the AM to use.
* 'tableSpaceName': name of the tablespace to create the index in.
* NULL specifies using the appropriate default.
***************
*** 103,113 **** static char *ChooseIndexNameAddition(List *colnames);
* it will be filled later.
* 'quiet': suppress the NOTICE chatter ordinarily provided for constraints.
* 'concurrent': avoid blocking writers to the table while building.
*/
! void
DefineIndex(RangeVar *heapRelation,
char *indexRelationName,
Oid indexRelationId,
char *accessMethodName,
char *tableSpaceName,
List *attributeList,
--- 291,304 ----
* it will be filled later.
* 'quiet': suppress the NOTICE chatter ordinarily provided for constraints.
* 'concurrent': avoid blocking writers to the table while building.
+ *
+ * Returns the OID of the created index.
*/
! Oid
DefineIndex(RangeVar *heapRelation,
char *indexRelationName,
Oid indexRelationId,
+ Oid relFileNode,
char *accessMethodName,
char *tableSpaceName,
List *attributeList,
***************
*** 403,413 **** DefineIndex(RangeVar *heapRelation,
}
/*
* Make the catalog entries for the index, including constraints. Then, if
* not skip_build || concurrent, actually build the index.
*/
indexRelationId =
! index_create(rel, indexRelationName, indexRelationId,
indexInfo, indexColNames,
accessMethodId, tablespaceId,
collationObjectId, classObjectId,
--- 594,610 ----
}
/*
+ * A valid relFileNode implies that we already have a built form of the
+ * index. The caller should also decline any index build.
+ */
+ Assert(!OidIsValid(relFileNode) || (skip_build && !concurrent));
+
+ /*
* Make the catalog entries for the index, including constraints. Then, if
* not skip_build || concurrent, actually build the index.
*/
indexRelationId =
! index_create(rel, indexRelationName, indexRelationId, relFileNode,
indexInfo, indexColNames,
accessMethodId, tablespaceId,
collationObjectId, classObjectId,
***************
*** 421,427 **** DefineIndex(RangeVar *heapRelation,
{
/* Close the heap and we're done, in the non-concurrent case */
heap_close(rel, NoLock);
! return;
}
/* save lockrelid and locktag for below, then close rel */
--- 618,624 ----
{
/* Close the heap and we're done, in the non-concurrent case */
heap_close(rel, NoLock);
! return indexRelationId;
}
/* save lockrelid and locktag for below, then close rel */
***************
*** 709,714 **** DefineIndex(RangeVar *heapRelation,
--- 906,913 ----
* Last thing to do is release the session-level lock on the parent table.
*/
UnlockRelationIdForSession(&heaprelid, ShareUpdateExclusiveLock);
+
+ return indexRelationId;
}
diff --git a/src/backend/commands/tableindex 3bc350a..a4430e2 100644
*** a/src/backend/commands/tablecmds.c
--- b/src/backend/commands/tablecmds.c
***************
*** 347,353 **** static bool ATColumnChangeRequiresRewrite(Node *expr, AttrNumber varattno);
static void ATExecAlterColumnType(AlteredTableInfo *tab, Relation rel,
AlterTableCmd *cmd, LOCKMODE lockmode);
static void ATPostAlterTypeCleanup(List **wqueue, AlteredTableInfo *tab, LOCKMODE lockmode);
! static void ATPostAlterTypeParse(char *cmd, List **wqueue, LOCKMODE lockmode);
static void change_owner_recurse_to_sequences(Oid relationOid,
Oid newOwnerId, LOCKMODE lockmode);
static void ATExecClusterOn(Relation rel, const char *indexName, LOCKMODE lockmode);
--- 347,355 ----
static void ATExecAlterColumnType(AlteredTableInfo *tab, Relation rel,
AlterTableCmd *cmd, LOCKMODE lockmode);
static void ATPostAlterTypeCleanup(List **wqueue, AlteredTableInfo *tab, LOCKMODE lockmode);
! static void ATPostAlterTypeParse(Oid oldId, char *cmd,
! List **wqueue, LOCKMODE lockmode, bool rewrite);
! static void TryReuseIndex(Oid oldId, IndexStmt *stmt);
static void change_owner_recurse_to_sequences(Oid relationOid,
Oid newOwnerId, LOCKMODE lockmode);
static void ATExecClusterOn(Relation rel, const char *indexName, LOCKMODE lockmode);
***************
*** 5225,5261 **** ATExecAddIndex(AlteredTableInfo *tab, Relation rel,
bool check_rights;
bool skip_build;
bool quiet;
Assert(IsA(stmt, IndexStmt));
/* suppress schema rights check when rebuilding existing index */
check_rights = !is_rebuild;
! /* skip index build if phase 3 will have to rewrite table anyway */
! skip_build = tab->rewrite;
/* suppress notices when rebuilding existing index */
quiet = is_rebuild;
/* The IndexStmt has already been through transformIndexStmt */
! DefineIndex(stmt->relation, /* relation */
! stmt->idxname, /* index name */
! InvalidOid, /* no predefined OID */
! stmt->accessMethod, /* am name */
! stmt->tableSpace,
! stmt->indexParams, /* parameters */
! (Expr *) stmt->whereClause,
! stmt->options,
! stmt->excludeOpNames,
! stmt->unique,
! stmt->primary,
! stmt->isconstraint,
! stmt->deferrable,
! stmt->initdeferred,
! true, /* is_alter_table */
! check_rights,
! skip_build,
! quiet,
! false);
}
/*
--- 5227,5278 ----
bool check_rights;
bool skip_build;
bool quiet;
+ Oid new_index;
Assert(IsA(stmt, IndexStmt));
/* suppress schema rights check when rebuilding existing index */
check_rights = !is_rebuild;
! /* skip index build if phase 3 will do it or we're reusing an old one */
! skip_build = tab->rewrite || OidIsValid(stmt->oldNode);
/* suppress notices when rebuilding existing index */
quiet = is_rebuild;
/* The IndexStmt has already been through transformIndexStmt */
! new_index = DefineIndex(stmt->relation, /* relation */
! stmt->idxname, /* index name */
! InvalidOid, /* no predefined OID */
! stmt->oldNode,
! stmt->accessMethod, /* am name */
! stmt->tableSpace,
! stmt->indexParams, /* parameters */
! (Expr *) stmt->whereClause,
! stmt->options,
! stmt->excludeOpNames,
! stmt->unique,
! stmt->primary,
! stmt->isconstraint,
! stmt->deferrable,
! stmt->initdeferred,
! true, /* is_alter_table */
! check_rights,
! skip_build,
! quiet,
! false);
!
! /*
! * If TryReuseIndex() stashed a relfilenode for us, we used it for the new
! * index instead of building from scratch. The DROP of the old edition of
! * this index will have scheduled the storage for deletion at commit, so
! * cancel that pending deletion.
! */
! if (OidIsValid(stmt->oldNode))
! {
! Relation irel = index_open(new_index, NoLock);
! RelationPreserveStorage(irel->rd_node, true);
! index_close(irel, NoLock);
! }
}
/*
***************
*** 7375,7381 **** static void
ATPostAlterTypeCleanup(List **wqueue, AlteredTableInfo *tab, LOCKMODE lockmode)
{
ObjectAddress obj;
! ListCell *l;
/*
* Re-parse the index and constraint definitions, and attach them to the
--- 7392,7399 ----
ATPostAlterTypeCleanup(List **wqueue, AlteredTableInfo *tab, LOCKMODE lockmode)
{
ObjectAddress obj;
! ListCell *def_item;
! ListCell *oid_item;
/*
* Re-parse the index and constraint definitions, and attach them to the
***************
*** 7385,7394 **** ATPostAlterTypeCleanup(List **wqueue, AlteredTableInfo *tab, LOCKMODE lockmode)
* that before dropping. It's safe because the parser won't actually look
* at the catalogs to detect the existing entry.
*/
! foreach(l, tab->changedIndexDefs)
! ATPostAlterTypeParse((char *) lfirst(l), wqueue, lockmode);
! foreach(l, tab->changedConstraintDefs)
! ATPostAlterTypeParse((char *) lfirst(l), wqueue, lockmode);
/*
* Now we can drop the existing constraints and indexes --- constraints
--- 7403,7416 ----
* that before dropping. It's safe because the parser won't actually look
* at the catalogs to detect the existing entry.
*/
! forboth(oid_item, tab->changedConstraintOids,
! def_item, tab->changedConstraintDefs)
! ATPostAlterTypeParse(lfirst_oid(oid_item), (char *) lfirst(def_item),
! wqueue, lockmode, tab->rewrite);
! forboth(oid_item, tab->changedIndexOids,
! def_item, tab->changedIndexDefs)
! ATPostAlterTypeParse(lfirst_oid(oid_item), (char *) lfirst(def_item),
! wqueue, lockmode, tab->rewrite);
/*
* Now we can drop the existing constraints and indexes --- constraints
***************
*** 7398,7415 **** ATPostAlterTypeCleanup(List **wqueue, AlteredTableInfo *tab, LOCKMODE lockmode)
* should be okay to use DROP_RESTRICT here, since nothing else should be
* depending on these objects.
*/
! foreach(l, tab->changedConstraintOids)
{
obj.classId = ConstraintRelationId;
! obj.objectId = lfirst_oid(l);
obj.objectSubId = 0;
performDeletion(&obj, DROP_RESTRICT);
}
! foreach(l, tab->changedIndexOids)
{
obj.classId = RelationRelationId;
! obj.objectId = lfirst_oid(l);
obj.objectSubId = 0;
performDeletion(&obj, DROP_RESTRICT);
}
--- 7420,7437 ----
* should be okay to use DROP_RESTRICT here, since nothing else should be
* depending on these objects.
*/
! foreach(oid_item, tab->changedConstraintOids)
{
obj.classId = ConstraintRelationId;
! obj.objectId = lfirst_oid(oid_item);
obj.objectSubId = 0;
performDeletion(&obj, DROP_RESTRICT);
}
! foreach(oid_item, tab->changedIndexOids)
{
obj.classId = RelationRelationId;
! obj.objectId = lfirst_oid(oid_item);
obj.objectSubId = 0;
performDeletion(&obj, DROP_RESTRICT);
}
***************
*** 7421,7427 **** ATPostAlterTypeCleanup(List **wqueue, AlteredTableInfo *tab, LOCKMODE lockmode)
}
static void
! ATPostAlterTypeParse(char *cmd, List **wqueue, LOCKMODE lockmode)
{
List *raw_parsetree_list;
List *querytree_list;
--- 7443,7450 ----
}
static void
! ATPostAlterTypeParse(Oid oldId, char *cmd,
! List **wqueue, LOCKMODE lockmode, bool rewrite)
{
List *raw_parsetree_list;
List *querytree_list;
***************
*** 7468,7473 **** ATPostAlterTypeParse(char *cmd, List **wqueue, LOCKMODE lockmode)
--- 7491,7499 ----
IndexStmt *stmt = (IndexStmt *) stm;
AlterTableCmd *newcmd;
+ if (!rewrite)
+ TryReuseIndex(oldId, stmt);
+
rel = relation_openrv(stmt->relation, lockmode);
tab = ATGetQueueEntry(wqueue, rel);
newcmd = makeNode(AlterTableCmd);
***************
*** 7492,7497 **** ATPostAlterTypeParse(char *cmd, List **wqueue, LOCKMODE lockmode)
--- 7518,7527 ----
switch (cmd->subtype)
{
case AT_AddIndex:
+ Assert(IsA(cmd->def, IndexStmt));
+ if (!rewrite)
+ TryReuseIndex(get_constraint_index(oldId),
+ (IndexStmt *) cmd->def);
cmd->subtype = AT_ReAddIndex;
tab->subcmds[AT_PASS_OLD_INDEX] =
lappend(tab->subcmds[AT_PASS_OLD_INDEX], cmd);
***************
*** 7515,7520 **** ATPostAlterTypeParse(char *cmd, List **wqueue, LOCKMODE lockmode)
--- 7545,7570 ----
}
}
+ /*
+ * Subroutine for ATPostAlterTypeParse(). Calls out to CheckIndexCompatible()
+ * for the real analysis, then mutates the IndexStmt based on that verdict.
+ */
+ static void
+ TryReuseIndex(Oid oldId, IndexStmt *stmt)
+ {
+
+ if (CheckIndexCompatible(oldId,
+ stmt->relation,
+ stmt->accessMethod,
+ stmt->indexParams,
+ stmt->excludeOpNames))
+ {
+ Relation irel = index_open(oldId, NoLock);
+ stmt->oldNode = irel->rd_node.relNode;
+ index_close(irel, NoLock);
+ }
+ }
+
/*
* ALTER TABLE OWNER
diff --git a/src/backend/nodes/copyfuncindex c9133dd..7a51456 100644
*** a/src/backend/nodes/copyfuncs.c
--- b/src/backend/nodes/copyfuncs.c
***************
*** 2807,2812 **** _copyIndexStmt(IndexStmt *from)
--- 2807,2813 ----
COPY_NODE_FIELD(whereClause);
COPY_NODE_FIELD(excludeOpNames);
COPY_SCALAR_FIELD(indexOid);
+ COPY_SCALAR_FIELD(oldNode);
COPY_SCALAR_FIELD(unique);
COPY_SCALAR_FIELD(primary);
COPY_SCALAR_FIELD(isconstraint);
diff --git a/src/backend/nodes/equalindex 3a0267c..4052a9a 100644
*** a/src/backend/nodes/equalfuncs.c
--- b/src/backend/nodes/equalfuncs.c
***************
*** 1245,1250 **** _equalIndexStmt(IndexStmt *a, IndexStmt *b)
--- 1245,1251 ----
COMPARE_NODE_FIELD(whereClause);
COMPARE_NODE_FIELD(excludeOpNames);
COMPARE_SCALAR_FIELD(indexOid);
+ COMPARE_SCALAR_FIELD(oldNode);
COMPARE_SCALAR_FIELD(unique);
COMPARE_SCALAR_FIELD(primary);
COMPARE_SCALAR_FIELD(isconstraint);
diff --git a/src/backend/nodes/outfunindex 681f5f8..b5be09a 100644
*** a/src/backend/nodes/outfuncs.c
--- b/src/backend/nodes/outfuncs.c
***************
*** 1976,1981 **** _outIndexStmt(StringInfo str, IndexStmt *node)
--- 1976,1982 ----
WRITE_NODE_FIELD(whereClause);
WRITE_NODE_FIELD(excludeOpNames);
WRITE_OID_FIELD(indexOid);
+ WRITE_OID_FIELD(oldNode);
WRITE_BOOL_FIELD(unique);
WRITE_BOOL_FIELD(primary);
WRITE_BOOL_FIELD(isconstraint);
diff --git a/src/backend/tcop/utiliindex 224e1f3..d13a56e 100644
*** a/src/backend/tcop/utility.c
--- b/src/backend/tcop/utility.c
***************
*** 945,950 **** standard_ProcessUtility(Node *parsetree,
--- 945,951 ----
DefineIndex(stmt->relation, /* relation */
stmt->idxname, /* index name */
InvalidOid, /* no predefined OID */
+ InvalidOid, /* no previous storage */
stmt->accessMethod, /* am name */
stmt->tableSpace,
stmt->indexParams, /* parameters */
diff --git a/src/backend/utils/caindex 0b9d77a..809222b 100644
*** a/src/backend/utils/cache/relcache.c
--- b/src/backend/utils/cache/relcache.c
***************
*** 2395,2400 **** RelationBuildLocalRelation(const char *relname,
--- 2395,2401 ----
Oid relnamespace,
TupleDesc tupDesc,
Oid relid,
+ Oid relfilenode,
Oid reltablespace,
bool shared_relation,
bool mapped_relation,
***************
*** 2529,2538 **** RelationBuildLocalRelation(const char *relname,
/*
* Insert relation physical and logical identifiers (OIDs) into the right
! * places. Note that the physical ID (relfilenode) is initially the same
! * as the logical ID (OID); except that for a mapped relation, we set
! * relfilenode to zero and rely on RelationInitPhysicalAddr to consult the
! * map.
*/
rel->rd_rel->relisshared = shared_relation;
--- 2530,2537 ----
/*
* Insert relation physical and logical identifiers (OIDs) into the right
! * places. For a mapped relation, we set relfilenode to zero and rely on
! * RelationInitPhysicalAddr to consult the map.
*/
rel->rd_rel->relisshared = shared_relation;
***************
*** 2547,2556 **** RelationBuildLocalRelation(const char *relname,
{
rel->rd_rel->relfilenode = InvalidOid;
/* Add it to the active mapping information */
! RelationMapUpdateMap(relid, relid, shared_relation, true);
}
else
! rel->rd_rel->relfilenode = relid;
RelationInitLockInfo(rel); /* see lmgr.c */
--- 2546,2555 ----
{
rel->rd_rel->relfilenode = InvalidOid;
/* Add it to the active mapping information */
! RelationMapUpdateMap(relid, relfilenode, shared_relation, true);
}
else
! rel->rd_rel->relfilenode = relfilenode;
RelationInitLockInfo(rel); /* see lmgr.c */
diff --git a/src/backend/utils/cache/relmindex a19ee28..b04dc9e 100644
*** a/src/backend/utils/cache/relmapper.c
--- b/src/backend/utils/cache/relmapper.c
***************
*** 792,798 **** write_relmap_file(bool shared, RelMapFile *newmap,
rnode.spcNode = tsid;
rnode.dbNode = dbid;
rnode.relNode = newmap->mappings[i].mapfilenode;
! RelationPreserveStorage(rnode);
}
}
--- 792,798 ----
rnode.spcNode = tsid;
rnode.dbNode = dbid;
rnode.relNode = newmap->mappings[i].mapfilenode;
! RelationPreserveStorage(rnode, false);
}
}
diff --git a/src/include/catalog/heap.h b/index 0b7b190..aee2d88 100644
*** a/src/include/catalog/heap.h
--- b/src/include/catalog/heap.h
***************
*** 39,44 **** extern Relation heap_create(const char *relname,
--- 39,45 ----
Oid relnamespace,
Oid reltablespace,
Oid relid,
+ Oid relfilenode,
TupleDesc tupDesc,
char relkind,
char relpersistence,
diff --git a/src/include/catalog/index 071db7f..8b78b05 100644
*** a/src/include/catalog/index.h
--- b/src/include/catalog/index.h
***************
*** 35,40 **** extern void index_check_primary_key(Relation heapRel,
--- 35,41 ----
extern Oid index_create(Relation heapRelation,
const char *indexRelationName,
Oid indexRelationId,
+ Oid relFileNode,
IndexInfo *indexInfo,
List *indexColNames,
Oid accessMethodObjectId,
diff --git a/src/include/catalog/sindex 8dee8cf..6907d83 100644
*** a/src/include/catalog/storage.h
--- b/src/include/catalog/storage.h
***************
*** 22,28 ****
extern void RelationCreateStorage(RelFileNode rnode, char relpersistence);
extern void RelationDropStorage(Relation rel);
! extern void RelationPreserveStorage(RelFileNode rnode);
extern void RelationTruncate(Relation rel, BlockNumber nblocks);
/*
--- 22,28 ----
extern void RelationCreateStorage(RelFileNode rnode, char relpersistence);
extern void RelationDropStorage(Relation rel);
! extern void RelationPreserveStorage(RelFileNode rnode, bool atCommit);
extern void RelationTruncate(Relation rel, BlockNumber nblocks);
/*
diff --git a/src/include/commands/deindex bbc024f..81c515e 100644
*** a/src/include/commands/defrem.h
--- b/src/include/commands/defrem.h
***************
*** 18,26 ****
/* commands/indexcmds.c */
! extern void DefineIndex(RangeVar *heapRelation,
char *indexRelationName,
Oid indexRelationId,
char *accessMethodName,
char *tableSpaceName,
List *attributeList,
--- 18,27 ----
/* commands/indexcmds.c */
! extern Oid DefineIndex(RangeVar *heapRelation,
char *indexRelationName,
Oid indexRelationId,
+ Oid relFileNode,
char *accessMethodName,
char *tableSpaceName,
List *attributeList,
***************
*** 49,54 **** extern char *ChooseIndexName(const char *tabname, Oid namespaceId,
--- 50,60 ----
List *colnames, List *exclusionOpNames,
bool primary, bool isconstraint);
extern List *ChooseIndexColumnNames(List *indexElems);
+ extern bool CheckIndexCompatible(Oid oldId,
+ RangeVar *heapRelation,
+ char *accessMethodName,
+ List *attributeList,
+ List *exclusionOpNames);
extern Oid GetDefaultOpClass(Oid type_id, Oid am_id);
/* commands/functioncmds.c */
diff --git a/src/include/nodes/parseindex 00c1269..92e40d3 100644
*** a/src/include/nodes/parsenodes.h
--- b/src/include/nodes/parsenodes.h
***************
*** 2062,2067 **** typedef struct IndexStmt
--- 2062,2068 ----
Node *whereClause; /* qualification (partial-index predicate) */
List *excludeOpNames; /* exclusion operator names, or NIL if none */
Oid indexOid; /* OID of an existing index, if any */
+ Oid oldNode; /* relfilenode of my former self */
bool unique; /* is index unique? */
bool primary; /* is index on primary key? */
bool isconstraint; /* is it from a CONSTRAINT clause? */
diff --git a/src/include/utils/relcacindex 1f4def5..ef1692c 100644
*** a/src/include/utils/relcache.h
--- b/src/include/utils/relcache.h
***************
*** 67,72 **** extern Relation RelationBuildLocalRelation(const char *relname,
--- 67,73 ----
Oid relnamespace,
TupleDesc tupDesc,
Oid relid,
+ Oid relfilenode,
Oid reltablespace,
bool shared_relation,
bool mapped_relation,
diff --git a/src/test/regress/expecindex 8e75b14..44df2ea 100644
diff --git a/src/test/regress/sql/big_alter_table.sqindex b568302..6ae9007 100644
On Wed, Jul 6, 2011 at 2:50 PM, Noah Misch <noah@2ndquadrant.com> wrote:
Drat; fixed in this version. My local branches contain a large test battery
that I filter out of the diff before posting. This time, that filter also
removed an essential part of the patch.
OK, I'm pretty happy with this version, with the following minor caveats:
1. I still think the documentation changes could use some
word-smithing. If I end up being the one who commits this, I'll take
a look at that as part of the commit.
2. I think it would be helpful to add a comment explaining why
CheckIndexCompatible() is calling
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
On Thu, Jul 7, 2011 at 2:43 PM, Robert Haas <robertmhaas@gmail.com> wrote:
On Wed, Jul 6, 2011 at 2:50 PM, Noah Misch <noah@2ndquadrant.com> wrote:
Drat; fixed in this version. My local branches contain a large test battery
that I filter out of the diff before posting. This time, that filter also
removed an essential part of the patch.OK, I'm pretty happy with this version, with the following minor caveats:
1. I still think the documentation changes could use some
word-smithing. If I end up being the one who commits this, I'll take
a look at that as part of the commit.2. I think it would be helpful to add a comment explaining why
CheckIndexCompatible() is calling
Woops, sorry. Hit send too soon.
...why CheckIndexCompatible() is calling ComputeIndexAttrs().
Rather than committing this immediately, I'm going to mark this "Ready
for Committer", just in case Tom or someone else wants to look this
over and weigh in.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
On Thu, Jul 07, 2011 at 02:44:49PM -0400, Robert Haas wrote:
On Thu, Jul 7, 2011 at 2:43 PM, Robert Haas <robertmhaas@gmail.com> wrote:
On Wed, Jul 6, 2011 at 2:50 PM, Noah Misch <noah@2ndquadrant.com> wrote:
Drat; fixed in this version. �My local branches contain a large test battery
that I filter out of the diff before posting. �This time, that filter also
removed an essential part of the patch.OK, I'm pretty happy with this version, with the following minor caveats:
1. I still think the documentation changes could use some
word-smithing. �If I end up being the one who commits this, I'll take
a look at that as part of the commit.2. I think it would be helpful to add a comment explaining why
CheckIndexCompatible() is callingWoops, sorry. Hit send too soon.
...why CheckIndexCompatible() is calling ComputeIndexAttrs().
CheckIndexCompatible() calls ComputeIndexAttrs() to resolve the new operator
classes, collations and exclusion operators for each index column. It then
checks those against the existing values for the same. I figured that was
obvious enough, but do you want a new version noting that?
Rather than committing this immediately, I'm going to mark this "Ready
for Committer", just in case Tom or someone else wants to look this
over and weigh in.
Great. Thanks for reviewing.
On Thu, Jul 7, 2011 at 2:55 PM, Noah Misch <noah@2ndquadrant.com> wrote:
CheckIndexCompatible() calls ComputeIndexAttrs() to resolve the new operator
classes, collations and exclusion operators for each index column. It then
checks those against the existing values for the same. I figured that was
obvious enough, but do you want a new version noting that?
I guess one question I had was... are we depending on the fact that
ComputeIndexAttrs() performs a bunch of internal sanity checks? Or
are we just expecting those to always pass, and we're going to examine
the outputs after the fact?
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
On Thu, Jul 07, 2011 at 03:06:46PM -0400, Robert Haas wrote:
On Thu, Jul 7, 2011 at 2:55 PM, Noah Misch <noah@2ndquadrant.com> wrote:
CheckIndexCompatible() calls ComputeIndexAttrs() to resolve the new operator
classes, collations and exclusion operators for each index column. �It then
checks those against the existing values for the same. �I figured that was
obvious enough, but do you want a new version noting that?I guess one question I had was... are we depending on the fact that
ComputeIndexAttrs() performs a bunch of internal sanity checks? Or
are we just expecting those to always pass, and we're going to examine
the outputs after the fact?
Those checks can fail; consider an explicit operator class or collation that
does not support the destination type. At that stage, we neither rely on those
checks nor mind if they do fire. If we somehow miss the problem at that stage,
DefineIndex() will detect it later. Likewise, if we hit an error in
CheckIndexCompatible(), we would also hit it later in DefineIndex().
On Thu, Jul 7, 2011 at 3:21 PM, Noah Misch <noah@2ndquadrant.com> wrote:
On Thu, Jul 07, 2011 at 03:06:46PM -0400, Robert Haas wrote:
On Thu, Jul 7, 2011 at 2:55 PM, Noah Misch <noah@2ndquadrant.com> wrote:
CheckIndexCompatible() calls ComputeIndexAttrs() to resolve the new operator
classes, collations and exclusion operators for each index column. It then
checks those against the existing values for the same. I figured that was
obvious enough, but do you want a new version noting that?I guess one question I had was... are we depending on the fact that
ComputeIndexAttrs() performs a bunch of internal sanity checks? Or
are we just expecting those to always pass, and we're going to examine
the outputs after the fact?Those checks can fail; consider an explicit operator class or collation that
does not support the destination type. At that stage, we neither rely on those
checks nor mind if they do fire. If we somehow miss the problem at that stage,
DefineIndex() will detect it later. Likewise, if we hit an error in
CheckIndexCompatible(), we would also hit it later in DefineIndex().
OK.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
On Thu, Jul 7, 2011 at 3:25 PM, Robert Haas <robertmhaas@gmail.com> wrote:
On Thu, Jul 7, 2011 at 3:21 PM, Noah Misch <noah@2ndquadrant.com> wrote:
On Thu, Jul 07, 2011 at 03:06:46PM -0400, Robert Haas wrote:
On Thu, Jul 7, 2011 at 2:55 PM, Noah Misch <noah@2ndquadrant.com> wrote:
CheckIndexCompatible() calls ComputeIndexAttrs() to resolve the new operator
classes, collations and exclusion operators for each index column. It then
checks those against the existing values for the same. I figured that was
obvious enough, but do you want a new version noting that?I guess one question I had was... are we depending on the fact that
ComputeIndexAttrs() performs a bunch of internal sanity checks? Or
are we just expecting those to always pass, and we're going to examine
the outputs after the fact?Those checks can fail; consider an explicit operator class or collation that
does not support the destination type. At that stage, we neither rely on those
checks nor mind if they do fire. If we somehow miss the problem at that stage,
DefineIndex() will detect it later. Likewise, if we hit an error in
CheckIndexCompatible(), we would also hit it later in DefineIndex().OK.
Committed with minor comment and documentation changes.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
On mån, 2011-07-18 at 11:05 -0400, Robert Haas wrote:
On Thu, Jul 7, 2011 at 3:25 PM, Robert Haas <robertmhaas@gmail.com> wrote:
On Thu, Jul 7, 2011 at 3:21 PM, Noah Misch <noah@2ndquadrant.com> wrote:
On Thu, Jul 07, 2011 at 03:06:46PM -0400, Robert Haas wrote:
On Thu, Jul 7, 2011 at 2:55 PM, Noah Misch <noah@2ndquadrant.com> wrote:
CheckIndexCompatible() calls ComputeIndexAttrs() to resolve the new operator
classes, collations and exclusion operators for each index column. It then
checks those against the existing values for the same. I figured that was
obvious enough, but do you want a new version noting that?I guess one question I had was... are we depending on the fact that
ComputeIndexAttrs() performs a bunch of internal sanity checks? Or
are we just expecting those to always pass, and we're going to examine
the outputs after the fact?Those checks can fail; consider an explicit operator class or collation that
does not support the destination type. At that stage, we neither rely on those
checks nor mind if they do fire. If we somehow miss the problem at that stage,
DefineIndex() will detect it later. Likewise, if we hit an error in
CheckIndexCompatible(), we would also hit it later in DefineIndex().OK.
Committed with minor comment and documentation changes.
Please review and fix this compiler warning:
indexcmds.c: In function ‘CheckIndexCompatible’:
indexcmds.c:126:15: warning: variable ‘amoptions’ set but not used [-Wunused-but-set-variable]
On Tue, Jul 19, 2011 at 12:24 AM, Peter Eisentraut <peter_e@gmx.net> wrote:
Please review and fix this compiler warning:
indexcmds.c: In function ‘CheckIndexCompatible’:
indexcmds.c:126:15: warning: variable ‘amoptions’ set but not used [-Wunused-but-set-variable]
I have removed the offending variable.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company