Thread: Getting rid of pre-assignment of index names in CREATE TABLE LIKE
In bug #6734 we have a complaint about a longstanding misfeature of
CREATE TABLE LIKE. Ordinarily, this command doesn't select names for
copied indexes, but leaves that to be done at runtime by DefineIndex.
But if it's copying comments, and an index of the source table has a
comment, it's forced to preassign a name to the new index so that it can
build a CommentStmt that can apply the comment after the index is made.
Apart from being something of a modularity violation, this isn't very safe
because of the danger of name collision with earlier indexes for the new
table. And that's exactly what's happening in bug #6734.
I suggested that we could dodge the problem by allowing IndexStmt to
carry a comment to be attached to the new index, and thereby avoid
needing an explicit COMMENT command. Attached is a patch that fixes it
that way.
While I was at it, it seemed like DefineIndex's parameter list had grown
well past any sane bound, so I refactored it to pass the IndexStmt
struct as-is rather than passing all the fields individually.
With or without that choice, though, this approach means a change in
DefineIndex's API, as well as the contents of struct IndexStmt. That
means it's probably unsafe to back-patch, since it seems plausible that
there might be third-party code out there that creates indexes and would
use these interfaces.
I would like to sneak this fix into 9.2, though. Does anyone think
it's already too late to be touching these APIs for 9.2?
regards, tom lane
diff --git a/src/backend/bootstrap/bootparse.y b/src/backend/bootstrap/bootparse.y
index 18f0add852e7832739e3877811e385abcb540fab..ec634f1660e0be883b451abbb380d6dc30e69b93 100644
*** a/src/backend/bootstrap/bootparse.y
--- b/src/backend/bootstrap/bootparse.y
*************** Boot_InsertStmt:
*** 279,296 ****
Boot_DeclareIndexStmt:
XDECLARE INDEX boot_ident oidspec ON boot_ident USING boot_ident LPAREN boot_index_params RPAREN
{
do_start();
! DefineIndex(makeRangeVar(NULL, $6, -1),
! $3,
$4,
! InvalidOid,
! $8,
! NULL,
! $10,
! NULL, NIL, NIL,
! false, false, false, false, false,
! false, false, true, false, false);
do_end();
}
;
--- 279,312 ----
Boot_DeclareIndexStmt:
XDECLARE INDEX boot_ident oidspec ON boot_ident USING boot_ident LPAREN boot_index_params RPAREN
{
+ IndexStmt *stmt = makeNode(IndexStmt);
+
do_start();
! stmt->idxname = $3;
! stmt->relation = makeRangeVar(NULL, $6, -1);
! stmt->accessMethod = $8;
! stmt->tableSpace = NULL;
! stmt->indexParams = $10;
! stmt->options = NIL;
! stmt->whereClause = NULL;
! stmt->excludeOpNames = NIL;
! stmt->idxcomment = NULL;
! stmt->indexOid = InvalidOid;
! stmt->oldNode = InvalidOid;
! stmt->unique = false;
! stmt->primary = false;
! stmt->isconstraint = false;
! stmt->deferrable = false;
! stmt->initdeferred = false;
! stmt->concurrent = false;
!
! DefineIndex(stmt,
$4,
! false,
! false,
! true, /* skip_build */
! false);
do_end();
}
;
*************** Boot_DeclareIndexStmt:
*** 298,315 ****
Boot_DeclareUniqueIndexStmt:
XDECLARE UNIQUE INDEX boot_ident oidspec ON boot_ident USING boot_ident LPAREN boot_index_params RPAREN
{
do_start();
! DefineIndex(makeRangeVar(NULL, $7, -1),
! $4,
$5,
! InvalidOid,
! $9,
! NULL,
! $11,
! NULL, NIL, NIL,
! true, false, false, false, false,
! false, false, true, false, false);
do_end();
}
;
--- 314,347 ----
Boot_DeclareUniqueIndexStmt:
XDECLARE UNIQUE INDEX boot_ident oidspec ON boot_ident USING boot_ident LPAREN boot_index_params RPAREN
{
+ IndexStmt *stmt = makeNode(IndexStmt);
+
do_start();
! stmt->idxname = $4;
! stmt->relation = makeRangeVar(NULL, $7, -1);
! stmt->accessMethod = $9;
! stmt->tableSpace = NULL;
! stmt->indexParams = $11;
! stmt->options = NIL;
! stmt->whereClause = NULL;
! stmt->excludeOpNames = NIL;
! stmt->idxcomment = NULL;
! stmt->indexOid = InvalidOid;
! stmt->oldNode = InvalidOid;
! stmt->unique = true;
! stmt->primary = false;
! stmt->isconstraint = false;
! stmt->deferrable = false;
! stmt->initdeferred = false;
! stmt->concurrent = false;
!
! DefineIndex(stmt,
$5,
! false,
! false,
! true, /* skip_build */
! false);
do_end();
}
;
diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c
index e5d1d8b699ad6d37cd3dd9e19b1d76ba97c01eaa..f315ab60154d6c6aa3c96f05babb8e79945f70b8 100644
*** a/src/backend/commands/indexcmds.c
--- b/src/backend/commands/indexcmds.c
***************
*** 24,29 ****
--- 24,30 ----
#include "catalog/pg_opfamily.h"
#include "catalog/pg_tablespace.h"
#include "catalog/pg_type.h"
+ #include "commands/comment.h"
#include "commands/dbcommands.h"
#include "commands/defrem.h"
#include "commands/tablecmds.h"
*************** static void ComputeIndexAttrs(IndexInfo
*** 65,71 ****
--- 66,76 ----
bool isconstraint);
static Oid GetIndexOpClass(List *opclass, Oid attrType,
char *accessMethodName, Oid accessMethodId);
+ static char *ChooseIndexName(const char *tabname, Oid namespaceId,
+ List *colnames, List *exclusionOpNames,
+ bool primary, bool isconstraint);
static char *ChooseIndexNameAddition(List *colnames);
+ static List *ChooseIndexColumnNames(List *indexElems);
static void RangeVarCallbackForReindexIndex(const RangeVar *relation,
Oid relId, Oid oldRelId, void *arg);
*************** CheckIndexCompatible(Oid oldId,
*** 268,327 ****
* DefineIndex
* Creates a new index.
*
! * 'heapRelation': the relation the index will apply to.
! * 'indexRelationName': the name for the new index, or NULL to indicate
! * 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.
- * 'attributeList': a list of IndexElem specifying columns and expressions
- * to index on.
- * 'predicate': the partial-index condition, or NULL if none.
- * 'options': reloptions from WITH (in list-of-DefElem form).
- * 'exclusionOpNames': list of names of exclusion-constraint operators,
- * or NIL if not an exclusion constraint.
- * 'unique': make the index enforce uniqueness.
- * 'primary': mark the index as a primary key in the catalogs.
- * 'isconstraint': index is for a PRIMARY KEY or UNIQUE constraint,
- * so build a pg_constraint entry for it.
- * 'deferrable': constraint is DEFERRABLE.
- * 'initdeferred': constraint is INITIALLY DEFERRED.
* 'is_alter_table': this is due to an ALTER rather than a CREATE operation.
* 'check_rights': check for CREATE rights in the namespace. (This should
* be true except when ALTER is deleting/recreating an index.)
* 'skip_build': make the catalog entries but leave the index file empty;
* 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,
- Expr *predicate,
- List *options,
- List *exclusionOpNames,
- bool unique,
- bool primary,
- bool isconstraint,
- bool deferrable,
- bool initdeferred,
bool is_alter_table,
bool check_rights,
bool skip_build,
! bool quiet,
! bool concurrent)
{
Oid *typeObjectId;
Oid *collationObjectId;
Oid *classObjectId;
--- 273,300 ----
* DefineIndex
* Creates a new index.
*
! * 'stmt': IndexStmt describing the properties of the new index.
* 'indexRelationId': normally InvalidOid, but during bootstrap can be
* nonzero to specify a preselected OID for the index.
* 'is_alter_table': this is due to an ALTER rather than a CREATE operation.
* 'check_rights': check for CREATE rights in the namespace. (This should
* be true except when ALTER is deleting/recreating an index.)
* 'skip_build': make the catalog entries but leave the index file empty;
* it will be filled later.
* 'quiet': suppress the NOTICE chatter ordinarily provided for constraints.
*
* Returns the OID of the created index.
*/
Oid
! DefineIndex(IndexStmt *stmt,
Oid indexRelationId,
bool is_alter_table,
bool check_rights,
bool skip_build,
! bool quiet)
{
+ char *indexRelationName;
+ char *accessMethodName;
Oid *typeObjectId;
Oid *collationObjectId;
Oid *classObjectId;
*************** DefineIndex(RangeVar *heapRelation,
*** 354,360 ****
/*
* count attributes in index
*/
! numberOfAttributes = list_length(attributeList);
if (numberOfAttributes <= 0)
ereport(ERROR,
(errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
--- 327,333 ----
/*
* count attributes in index
*/
! numberOfAttributes = list_length(stmt->indexParams);
if (numberOfAttributes <= 0)
ereport(ERROR,
(errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
*************** DefineIndex(RangeVar *heapRelation,
*** 372,379 ****
* index build; but for concurrent builds we allow INSERT/UPDATE/DELETE
* (but not VACUUM).
*/
! rel = heap_openrv(heapRelation,
! (concurrent ? ShareUpdateExclusiveLock : ShareLock));
relationId = RelationGetRelid(rel);
namespaceId = RelationGetNamespace(rel);
--- 345,352 ----
* index build; but for concurrent builds we allow INSERT/UPDATE/DELETE
* (but not VACUUM).
*/
! rel = heap_openrv(stmt->relation,
! (stmt->concurrent ? ShareUpdateExclusiveLock : ShareLock));
relationId = RelationGetRelid(rel);
namespaceId = RelationGetNamespace(rel);
*************** DefineIndex(RangeVar *heapRelation,
*** 389,400 ****
ereport(ERROR,
(errcode(ERRCODE_WRONG_OBJECT_TYPE),
errmsg("cannot create index on foreign table \"%s\"",
! heapRelation->relname)));
else
ereport(ERROR,
(errcode(ERRCODE_WRONG_OBJECT_TYPE),
errmsg("\"%s\" is not a table",
! heapRelation->relname)));
}
/*
--- 362,373 ----
ereport(ERROR,
(errcode(ERRCODE_WRONG_OBJECT_TYPE),
errmsg("cannot create index on foreign table \"%s\"",
! stmt->relation->relname)));
else
ereport(ERROR,
(errcode(ERRCODE_WRONG_OBJECT_TYPE),
errmsg("\"%s\" is not a table",
! stmt->relation->relname)));
}
/*
*************** DefineIndex(RangeVar *heapRelation,
*** 426,434 ****
* Select tablespace to use. If not specified, use default tablespace
* (which may in turn default to database's default).
*/
! if (tableSpaceName)
{
! tablespaceId = get_tablespace_oid(tableSpaceName, false);
}
else
{
--- 399,407 ----
* Select tablespace to use. If not specified, use default tablespace
* (which may in turn default to database's default).
*/
! if (stmt->tableSpace)
{
! tablespaceId = get_tablespace_oid(stmt->tableSpace, false);
}
else
{
*************** DefineIndex(RangeVar *heapRelation,
*** 463,484 ****
/*
* Choose the index column names.
*/
! indexColNames = ChooseIndexColumnNames(attributeList);
/*
* Select name for index if caller didn't specify
*/
if (indexRelationName == NULL)
indexRelationName = ChooseIndexName(RelationGetRelationName(rel),
namespaceId,
indexColNames,
! exclusionOpNames,
! primary,
! isconstraint);
/*
* look up the access method, verify it can handle the requested features
*/
tuple = SearchSysCache1(AMNAME, PointerGetDatum(accessMethodName));
if (!HeapTupleIsValid(tuple))
{
--- 436,459 ----
/*
* Choose the index column names.
*/
! indexColNames = ChooseIndexColumnNames(stmt->indexParams);
/*
* Select name for index if caller didn't specify
*/
+ indexRelationName = stmt->idxname;
if (indexRelationName == NULL)
indexRelationName = ChooseIndexName(RelationGetRelationName(rel),
namespaceId,
indexColNames,
! stmt->excludeOpNames,
! stmt->primary,
! stmt->isconstraint);
/*
* look up the access method, verify it can handle the requested features
*/
+ accessMethodName = stmt->accessMethod;
tuple = SearchSysCache1(AMNAME, PointerGetDatum(accessMethodName));
if (!HeapTupleIsValid(tuple))
{
*************** DefineIndex(RangeVar *heapRelation,
*** 503,509 ****
accessMethodId = HeapTupleGetOid(tuple);
accessMethodForm = (Form_pg_am) GETSTRUCT(tuple);
! if (unique && !accessMethodForm->amcanunique)
ereport(ERROR,
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
errmsg("access method \"%s\" does not support unique indexes",
--- 478,484 ----
accessMethodId = HeapTupleGetOid(tuple);
accessMethodForm = (Form_pg_am) GETSTRUCT(tuple);
! if (stmt->unique && !accessMethodForm->amcanunique)
ereport(ERROR,
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
errmsg("access method \"%s\" does not support unique indexes",
*************** DefineIndex(RangeVar *heapRelation,
*** 513,519 ****
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
errmsg("access method \"%s\" does not support multicolumn indexes",
accessMethodName)));
! if (exclusionOpNames != NIL && !OidIsValid(accessMethodForm->amgettuple))
ereport(ERROR,
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
errmsg("access method \"%s\" does not support exclusion constraints",
--- 488,494 ----
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
errmsg("access method \"%s\" does not support multicolumn indexes",
accessMethodName)));
! if (stmt->excludeOpNames && !OidIsValid(accessMethodForm->amgettuple))
ereport(ERROR,
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
errmsg("access method \"%s\" does not support exclusion constraints",
*************** DefineIndex(RangeVar *heapRelation,
*** 527,539 ****
/*
* Validate predicate, if given
*/
! if (predicate)
! CheckPredicate(predicate);
/*
* Parse AM-specific options, convert to text array form, validate.
*/
! reloptions = transformRelOptions((Datum) 0, options, NULL, NULL, false, false);
(void) index_reloptions(amoptions, reloptions, true);
--- 502,515 ----
/*
* Validate predicate, if given
*/
! if (stmt->whereClause)
! CheckPredicate((Expr *) stmt->whereClause);
/*
* Parse AM-specific options, convert to text array form, validate.
*/
! reloptions = transformRelOptions((Datum) 0, stmt->options,
! NULL, NULL, false, false);
(void) index_reloptions(amoptions, reloptions, true);
*************** DefineIndex(RangeVar *heapRelation,
*** 545,559 ****
indexInfo->ii_NumIndexAttrs = numberOfAttributes;
indexInfo->ii_Expressions = NIL; /* for now */
indexInfo->ii_ExpressionsState = NIL;
! indexInfo->ii_Predicate = make_ands_implicit(predicate);
indexInfo->ii_PredicateState = NIL;
indexInfo->ii_ExclusionOps = NULL;
indexInfo->ii_ExclusionProcs = NULL;
indexInfo->ii_ExclusionStrats = NULL;
! indexInfo->ii_Unique = unique;
/* In a concurrent build, mark it not-ready-for-inserts */
! indexInfo->ii_ReadyForInserts = !concurrent;
! indexInfo->ii_Concurrent = concurrent;
indexInfo->ii_BrokenHotChain = false;
typeObjectId = (Oid *) palloc(numberOfAttributes * sizeof(Oid));
--- 521,535 ----
indexInfo->ii_NumIndexAttrs = numberOfAttributes;
indexInfo->ii_Expressions = NIL; /* for now */
indexInfo->ii_ExpressionsState = NIL;
! indexInfo->ii_Predicate = make_ands_implicit((Expr *) stmt->whereClause);
indexInfo->ii_PredicateState = NIL;
indexInfo->ii_ExclusionOps = NULL;
indexInfo->ii_ExclusionProcs = NULL;
indexInfo->ii_ExclusionStrats = NULL;
! indexInfo->ii_Unique = stmt->unique;
/* In a concurrent build, mark it not-ready-for-inserts */
! indexInfo->ii_ReadyForInserts = !stmt->concurrent;
! indexInfo->ii_Concurrent = stmt->concurrent;
indexInfo->ii_BrokenHotChain = false;
typeObjectId = (Oid *) palloc(numberOfAttributes * sizeof(Oid));
*************** DefineIndex(RangeVar *heapRelation,
*** 562,591 ****
coloptions = (int16 *) palloc(numberOfAttributes * sizeof(int16));
ComputeIndexAttrs(indexInfo,
typeObjectId, collationObjectId, classObjectId,
! coloptions, attributeList,
! exclusionOpNames, relationId,
accessMethodName, accessMethodId,
! amcanorder, isconstraint);
/*
* Extra checks when creating a PRIMARY KEY index.
*/
! if (primary)
index_check_primary_key(rel, indexInfo, is_alter_table);
/*
* Report index creation if appropriate (delay this till after most of the
* error checks)
*/
! if (isconstraint && !quiet)
{
const char *constraint_type;
! if (primary)
constraint_type = "PRIMARY KEY";
! else if (unique)
constraint_type = "UNIQUE";
! else if (exclusionOpNames != NIL)
constraint_type = "EXCLUDE";
else
{
--- 538,567 ----
coloptions = (int16 *) palloc(numberOfAttributes * sizeof(int16));
ComputeIndexAttrs(indexInfo,
typeObjectId, collationObjectId, classObjectId,
! coloptions, stmt->indexParams,
! stmt->excludeOpNames, relationId,
accessMethodName, accessMethodId,
! amcanorder, stmt->isconstraint);
/*
* Extra checks when creating a PRIMARY KEY index.
*/
! if (stmt->primary)
index_check_primary_key(rel, indexInfo, is_alter_table);
/*
* Report index creation if appropriate (delay this till after most of the
* error checks)
*/
! if (stmt->isconstraint && !quiet)
{
const char *constraint_type;
! if (stmt->primary)
constraint_type = "PRIMARY KEY";
! else if (stmt->unique)
constraint_type = "UNIQUE";
! else if (stmt->excludeOpNames != NIL)
constraint_type = "EXCLUDE";
else
{
*************** DefineIndex(RangeVar *heapRelation,
*** 601,627 ****
}
/*
! * 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,
! coloptions, reloptions, primary,
! isconstraint, deferrable, initdeferred,
allowSystemTableMods,
! skip_build || concurrent,
! concurrent);
! if (!concurrent)
{
/* Close the heap and we're done, in the non-concurrent case */
heap_close(rel, NoLock);
--- 577,607 ----
}
/*
! * A valid stmt->oldNode implies that we already have a built form of the
* index. The caller should also decline any index build.
*/
! Assert(!OidIsValid(stmt->oldNode) || (skip_build && !stmt->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, stmt->oldNode,
indexInfo, indexColNames,
accessMethodId, tablespaceId,
collationObjectId, classObjectId,
! coloptions, reloptions, stmt->primary,
! stmt->isconstraint, stmt->deferrable, stmt->initdeferred,
allowSystemTableMods,
! skip_build || stmt->concurrent,
! stmt->concurrent);
! if (stmt->idxcomment != NULL)
! CreateComments(indexRelationId, RelationRelationId, 0,
! stmt->idxcomment);
!
! if (!stmt->concurrent)
{
/* Close the heap and we're done, in the non-concurrent case */
heap_close(rel, NoLock);
*************** DefineIndex(RangeVar *heapRelation,
*** 709,715 ****
*/
/* Open and lock the parent heap relation */
! rel = heap_openrv(heapRelation, ShareUpdateExclusiveLock);
/* And the target index relation */
indexRelation = index_open(indexRelationId, RowExclusiveLock);
--- 689,695 ----
*/
/* Open and lock the parent heap relation */
! rel = heap_openrv(stmt->relation, ShareUpdateExclusiveLock);
/* And the target index relation */
indexRelation = index_open(indexRelationId, RowExclusiveLock);
*************** DefineIndex(RangeVar *heapRelation,
*** 724,730 ****
indexInfo->ii_BrokenHotChain = false;
/* Now build the index */
! index_build(rel, indexRelation, indexInfo, primary, false);
/* Close both the relations, but keep the locks */
heap_close(rel, NoLock);
--- 704,710 ----
indexInfo->ii_BrokenHotChain = false;
/* Now build the index */
! index_build(rel, indexRelation, indexInfo, stmt->primary, false);
/* Close both the relations, but keep the locks */
heap_close(rel, NoLock);
*************** ChooseRelationName(const char *name1, co
*** 1594,1600 ****
*
* The argument list is pretty ad-hoc :-(
*/
! char *
ChooseIndexName(const char *tabname, Oid namespaceId,
List *colnames, List *exclusionOpNames,
bool primary, bool isconstraint)
--- 1574,1580 ----
*
* The argument list is pretty ad-hoc :-(
*/
! static char *
ChooseIndexName(const char *tabname, Oid namespaceId,
List *colnames, List *exclusionOpNames,
bool primary, bool isconstraint)
*************** ChooseIndexNameAddition(List *colnames)
*** 1676,1682 ****
*
* Returns a List of plain strings (char *, not String nodes).
*/
! List *
ChooseIndexColumnNames(List *indexElems)
{
List *result = NIL;
--- 1656,1662 ----
*
* Returns a List of plain strings (char *, not String nodes).
*/
! static List *
ChooseIndexColumnNames(List *indexElems)
{
List *result = NIL;
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index d69809a2f866de511e7ce278b8dca9bf9db33d40..70e408cb6ea1fc6a80b943f22d3a91d226226bb8 100644
*** a/src/backend/commands/tablecmds.c
--- b/src/backend/commands/tablecmds.c
*************** ATExecAddIndex(AlteredTableInfo *tab, Re
*** 5389,5394 ****
--- 5389,5395 ----
Oid new_index;
Assert(IsA(stmt, IndexStmt));
+ Assert(!stmt->concurrent);
/* suppress schema rights check when rebuilding existing index */
check_rights = !is_rebuild;
*************** ATExecAddIndex(AlteredTableInfo *tab, Re
*** 5399,5424 ****
/* 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
--- 5400,5411 ----
/* The IndexStmt has already been through transformIndexStmt */
! new_index = DefineIndex(stmt,
InvalidOid, /* no predefined OID */
true, /* is_alter_table */
check_rights,
skip_build,
! quiet);
/*
* If TryReuseIndex() stashed a relfilenode for us, we used it for the new
*************** ATPostAlterTypeParse(Oid oldId, char *cm
*** 7968,7974 ****
static void
TryReuseIndex(Oid oldId, IndexStmt *stmt)
{
-
if (CheckIndexCompatible(oldId,
stmt->relation,
stmt->accessMethod,
--- 7955,7960 ----
diff --git a/src/backend/nodes/copyfuncs.c b/src/backend/nodes/copyfuncs.c
index 1743b8fdc89735f0f619c290e7286be23f3e72e6..9bac99452d334b6e7a92c662b1287d552945c6d9 100644
*** a/src/backend/nodes/copyfuncs.c
--- b/src/backend/nodes/copyfuncs.c
*************** _copyIndexStmt(const IndexStmt *from)
*** 2826,2831 ****
--- 2826,2832 ----
COPY_NODE_FIELD(options);
COPY_NODE_FIELD(whereClause);
COPY_NODE_FIELD(excludeOpNames);
+ COPY_STRING_FIELD(idxcomment);
COPY_SCALAR_FIELD(indexOid);
COPY_SCALAR_FIELD(oldNode);
COPY_SCALAR_FIELD(unique);
diff --git a/src/backend/nodes/equalfuncs.c b/src/backend/nodes/equalfuncs.c
index f19ad7702618dcfb7f3be416dc535e6ac11b1b5e..2171d8d018bab95e0ed118384ddf8416502a86e8 100644
*** a/src/backend/nodes/equalfuncs.c
--- b/src/backend/nodes/equalfuncs.c
*************** _equalIndexStmt(const IndexStmt *a, cons
*** 1250,1255 ****
--- 1250,1256 ----
COMPARE_NODE_FIELD(options);
COMPARE_NODE_FIELD(whereClause);
COMPARE_NODE_FIELD(excludeOpNames);
+ COMPARE_STRING_FIELD(idxcomment);
COMPARE_SCALAR_FIELD(indexOid);
COMPARE_SCALAR_FIELD(oldNode);
COMPARE_SCALAR_FIELD(unique);
diff --git a/src/backend/nodes/outfuncs.c b/src/backend/nodes/outfuncs.c
index d6dff9de47a982d6e41ea191af2f0415be181d7f..91b54265afe6dadb4f171ec9abdf5ff7ec9c69c5 100644
*** a/src/backend/nodes/outfuncs.c
--- b/src/backend/nodes/outfuncs.c
*************** _outIndexStmt(StringInfo str, const Inde
*** 1994,1999 ****
--- 1994,2000 ----
WRITE_NODE_FIELD(options);
WRITE_NODE_FIELD(whereClause);
WRITE_NODE_FIELD(excludeOpNames);
+ WRITE_STRING_FIELD(idxcomment);
WRITE_OID_FIELD(indexOid);
WRITE_OID_FIELD(oldNode);
WRITE_BOOL_FIELD(unique);
diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y
index 7e6ceedc42af9362a6dbdb49c6373dcf02eecfb6..1a17337a7ec6a80353e6391964e12c0242a55854 100644
*** a/src/backend/parser/gram.y
--- b/src/backend/parser/gram.y
*************** IndexStmt: CREATE opt_unique INDEX opt_c
*** 5837,5843 ****
--- 5837,5850 ----
n->options = $12;
n->tableSpace = $13;
n->whereClause = $14;
+ n->excludeOpNames = NIL;
+ n->idxcomment = NULL;
n->indexOid = InvalidOid;
+ n->oldNode = InvalidOid;
+ n->primary = false;
+ n->isconstraint = false;
+ n->deferrable = false;
+ n->initdeferred = false;
$$ = (Node *)n;
}
;
diff --git a/src/backend/parser/parse_utilcmd.c b/src/backend/parser/parse_utilcmd.c
index e01e103774ea2c6b1d429eb12e716b5da54bc4f9..c22c6ed21f76e0846da1d2886e4371501878af2f 100644
*** a/src/backend/parser/parse_utilcmd.c
--- b/src/backend/parser/parse_utilcmd.c
*************** static void transformTableLikeClause(Cre
*** 106,112 ****
TableLikeClause *table_like_clause);
static void transformOfType(CreateStmtContext *cxt,
TypeName *ofTypename);
- static char *chooseIndexName(const RangeVar *relation, IndexStmt *index_stmt);
static IndexStmt *generateClonedIndexStmt(CreateStmtContext *cxt,
Relation source_idx,
const AttrNumber *attmap, int attmap_length);
--- 106,111 ----
*************** transformTableLikeClause(CreateStmtConte
*** 872,904 ****
index_stmt = generateClonedIndexStmt(cxt, parent_index,
attmap, tupleDesc->natts);
! /* Copy comment on index */
if (table_like_clause->options & CREATE_TABLE_LIKE_COMMENTS)
{
comment = GetComment(parent_index_oid, RelationRelationId, 0);
! if (comment != NULL)
! {
! CommentStmt *stmt;
!
! /*
! * We have to assign the index a name now, so that we can
! * reference it in CommentStmt.
! */
! if (index_stmt->idxname == NULL)
! index_stmt->idxname = chooseIndexName(cxt->relation,
! index_stmt);
!
! stmt = makeNode(CommentStmt);
! stmt->objtype = OBJECT_INDEX;
! stmt->objname =
! list_make2(makeString(cxt->relation->schemaname),
! makeString(index_stmt->idxname));
! stmt->objargs = NIL;
! stmt->comment = comment;
!
! cxt->alist = lappend(cxt->alist, stmt);
! }
}
/* Save it in the inh_indexes list for the time being */
--- 871,886 ----
index_stmt = generateClonedIndexStmt(cxt, parent_index,
attmap, tupleDesc->natts);
! /* Copy comment on index, if requested */
if (table_like_clause->options & CREATE_TABLE_LIKE_COMMENTS)
{
comment = GetComment(parent_index_oid, RelationRelationId, 0);
! /*
! * We make use of IndexStmt's idxcomment option, so as not to
! * need to know now what name the index will have.
! */
! index_stmt->idxcomment = comment;
}
/* Save it in the inh_indexes list for the time being */
*************** transformOfType(CreateStmtContext *cxt,
*** 961,989 ****
}
/*
- * chooseIndexName
- *
- * Compute name for an index. This must match code in indexcmds.c.
- *
- * XXX this is inherently broken because the indexes aren't created
- * immediately, so we fail to resolve conflicts when the same name is
- * derived for multiple indexes. However, that's a reasonably uncommon
- * situation, so we'll live with it for now.
- */
- static char *
- chooseIndexName(const RangeVar *relation, IndexStmt *index_stmt)
- {
- Oid namespaceId;
- List *colnames;
-
- namespaceId = RangeVarGetCreationNamespace(relation);
- colnames = ChooseIndexColumnNames(index_stmt->indexParams);
- return ChooseIndexName(relation->relname, namespaceId,
- colnames, index_stmt->excludeOpNames,
- index_stmt->primary, index_stmt->isconstraint);
- }
-
- /*
* Generate an IndexStmt node using information from an already existing index
* "source_idx". Attribute numbers should be adjusted according to attmap.
*/
--- 943,948 ----
*************** generateClonedIndexStmt(CreateStmtContex
*** 1046,1052 ****
--- 1005,1014 ----
index->tableSpace = get_tablespace_name(idxrelrec->reltablespace);
else
index->tableSpace = NULL;
+ index->excludeOpNames = NIL;
+ index->idxcomment = NULL;
index->indexOid = InvalidOid;
+ index->oldNode = InvalidOid;
index->unique = idxrec->indisunique;
index->primary = idxrec->indisprimary;
index->concurrent = false;
*************** transformIndexConstraint(Constraint *con
*** 1504,1510 ****
--- 1466,1474 ----
index->whereClause = constraint->where_clause;
index->indexParams = NIL;
index->excludeOpNames = NIL;
+ index->idxcomment = NULL;
index->indexOid = InvalidOid;
+ index->oldNode = InvalidOid;
index->concurrent = false;
/*
diff --git a/src/backend/tcop/utility.c b/src/backend/tcop/utility.c
index 33b292e6b7a9c1e47fb34de126cbc34dc5246693..7f36a09f46d297d4e98782c842a0a0f9d7ccb8e7 100644
*** a/src/backend/tcop/utility.c
--- b/src/backend/tcop/utility.c
*************** standard_ProcessUtility(Node *parsetree,
*** 921,946 ****
stmt = transformIndexStmt(stmt, queryString);
/* ... and do it */
! 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 */
- (Expr *) stmt->whereClause,
- stmt->options,
- stmt->excludeOpNames,
- stmt->unique,
- stmt->primary,
- stmt->isconstraint,
- stmt->deferrable,
- stmt->initdeferred,
false, /* is_alter_table */
true, /* check_rights */
false, /* skip_build */
! false, /* quiet */
! stmt->concurrent); /* concurrent */
}
break;
--- 921,932 ----
stmt = transformIndexStmt(stmt, queryString);
/* ... and do it */
! DefineIndex(stmt,
InvalidOid, /* no predefined OID */
false, /* is_alter_table */
true, /* check_rights */
false, /* skip_build */
! false); /* quiet */
}
break;
diff --git a/src/include/commands/defrem.h b/src/include/commands/defrem.h
index 8f3d2c358dceb73587a934284fb6340b91d91cb4..9b6d57a21e23c38688874bef9fe4a423c6d49289 100644
*** a/src/include/commands/defrem.h
--- b/src/include/commands/defrem.h
***************
*** 20,45 ****
extern void RemoveObjects(DropStmt *stmt);
/* commands/indexcmds.c */
! extern Oid DefineIndex(RangeVar *heapRelation,
! char *indexRelationName,
Oid indexRelationId,
- Oid relFileNode,
- char *accessMethodName,
- char *tableSpaceName,
- List *attributeList,
- Expr *predicate,
- List *options,
- List *exclusionOpNames,
- bool unique,
- bool primary,
- bool isconstraint,
- bool deferrable,
- bool initdeferred,
bool is_alter_table,
bool check_rights,
bool skip_build,
! bool quiet,
! bool concurrent);
extern void ReindexIndex(RangeVar *indexRelation);
extern void ReindexTable(RangeVar *relation);
extern void ReindexDatabase(const char *databaseName,
--- 20,31 ----
extern void RemoveObjects(DropStmt *stmt);
/* commands/indexcmds.c */
! extern Oid DefineIndex(IndexStmt *stmt,
Oid indexRelationId,
bool is_alter_table,
bool check_rights,
bool skip_build,
! bool quiet);
extern void ReindexIndex(RangeVar *indexRelation);
extern void ReindexTable(RangeVar *relation);
extern void ReindexDatabase(const char *databaseName,
*************** extern char *makeObjectName(const char *
*** 48,57 ****
const char *label);
extern char *ChooseRelationName(const char *name1, const char *name2,
const char *label, Oid namespaceid);
- extern char *ChooseIndexName(const char *tabname, Oid namespaceId,
- List *colnames, List *exclusionOpNames,
- bool primary, bool isconstraint);
- extern List *ChooseIndexColumnNames(List *indexElems);
extern bool CheckIndexCompatible(Oid oldId,
RangeVar *heapRelation,
char *accessMethodName,
--- 34,39 ----
diff --git a/src/include/nodes/parsenodes.h b/src/include/nodes/parsenodes.h
index 50111cba3c0eca1f5abe341bbee6bb0464b0439f..a4591cc9aa4060ebbc569d014f0404eadbe8af56 100644
*** a/src/include/nodes/parsenodes.h
--- b/src/include/nodes/parsenodes.h
*************** typedef struct FetchStmt
*** 2018,2027 ****
* Create Index Statement
*
* This represents creation of an index and/or an associated constraint.
! * If indexOid isn't InvalidOid, we are not creating an index, just a
! * UNIQUE/PKEY constraint using an existing index. isconstraint must always
! * be true in this case, and the fields describing the index properties are
! * empty.
* ----------------------
*/
typedef struct IndexStmt
--- 2018,2028 ----
* Create Index Statement
*
* This represents creation of an index and/or an associated constraint.
! * If isconstraint is true, we should create a pg_constraint entry along
! * with the index. But if indexOid isn't InvalidOid, we are not creating an
! * index, just a UNIQUE/PKEY constraint using an existing index. isconstraint
! * must always be true in this case, and the fields describing the index
! * properties are empty.
* ----------------------
*/
typedef struct IndexStmt
*************** typedef struct IndexStmt
*** 2031,2045 ****
RangeVar *relation; /* relation to build index on */
char *accessMethod; /* name of access method (eg. btree) */
char *tableSpace; /* tablespace, or NULL for default */
! List *indexParams; /* a list of IndexElem */
! List *options; /* options from WITH clause */
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? */
bool deferrable; /* is the constraint DEFERRABLE? */
bool initdeferred; /* is the constraint INITIALLY DEFERRED? */
bool concurrent; /* should this be a concurrent index build? */
--- 2032,2047 ----
RangeVar *relation; /* relation to build index on */
char *accessMethod; /* name of access method (eg. btree) */
char *tableSpace; /* tablespace, or NULL for default */
! List *indexParams; /* columns to index: a list of IndexElem */
! List *options; /* WITH clause options: a list of DefElem */
Node *whereClause; /* qualification (partial-index predicate) */
List *excludeOpNames; /* exclusion operator names, or NIL if none */
+ char *idxcomment; /* comment to apply to index, or NULL */
Oid indexOid; /* OID of an existing index, if any */
! Oid oldNode; /* relfilenode of existing storage, if any */
bool unique; /* is index unique? */
! bool primary; /* is index a primary key? */
! bool isconstraint; /* is it for a pkey/unique constraint? */
bool deferrable; /* is the constraint DEFERRABLE? */
bool initdeferred; /* is the constraint INITIALLY DEFERRED? */
bool concurrent; /* should this be a concurrent index build? */
On Sat, Jul 14, 2012 at 4:02 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
I'd like us to stick to the standard practice of not changing features/API in beta releases.
While I was at it, it seemed like DefineIndex's parameter list had grown
well past any sane bound, so I refactored it to pass the IndexStmt
struct as-is rather than passing all the fields individually.
With or without that choice, though, this approach means a change in
DefineIndex's API, as well as the contents of struct IndexStmt. That
means it's probably unsafe to back-patch, since it seems plausible that
there might be third-party code out there that creates indexes and would
use these interfaces.
I would like to sneak this fix into 9.2, though. Does anyone think
it's already too late to be touching these APIs for 9.2?
I'd like us to stick to the standard practice of not changing features/API in beta releases.
Best regards,
--
Gurjeet Singh
EnterpriseDB Corporation
The Enterprise PostgreSQL Company
EnterpriseDB Corporation
The Enterprise PostgreSQL Company
Gurjeet Singh <singh.gurjeet@gmail.com> writes:
> On Sat, Jul 14, 2012 at 4:02 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> I would like to sneak this fix into 9.2, though. Does anyone think
>> it's already too late to be touching these APIs for 9.2?
> I'd like us to stick to the standard practice of not changing features/API
> in beta releases.
This is a bug fix, not a feature addition, and sometimes you can't fix
bugs without touching APIs that might be used by third party code.
So the question here is whether this bug fix is sufficiently important,
and on the other side how likely it is that anyone has already built
extensions for 9.2 that depend on IndexStmt or DefineIndex. I don't
think trying to treat it as a "policy" matter is helpful -- it's a
tradeoff.
If you happen to know of EDB-private code that would be broken by
this change, telling us so (and why a mid-beta change would be
problematic) would be helpful.
regards, tom lane
On Sun, Jul 15, 2012 at 11:49 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
I was hoping that we could fix the bug in released code without having to change the structure or the API, but if that's not feasible, I will withdraw my objection.
I checked, and I don't see any EDB code that would be affected by this change.
Gurjeet Singh <singh.gurjeet@gmail.com> writes:
> On Sat, Jul 14, 2012 at 4:02 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:>> I would like to sneak this fix into 9.2, though. Does anyone thinkThis is a bug fix, not a feature addition, and sometimes you can't fix
>> it's already too late to be touching these APIs for 9.2?
> I'd like us to stick to the standard practice of not changing features/API
> in beta releases.
bugs without touching APIs that might be used by third party code.
So the question here is whether this bug fix is sufficiently important,
and on the other side how likely it is that anyone has already built
extensions for 9.2 that depend on IndexStmt or DefineIndex. I don't
think trying to treat it as a "policy" matter is helpful -- it's a
tradeoff.
I was hoping that we could fix the bug in released code without having to change the structure or the API, but if that's not feasible, I will withdraw my objection.
If you happen to know of EDB-private code that would be broken by
this change, telling us so (and why a mid-beta change would be
problematic) would be helpful.
I checked, and I don't see any EDB code that would be affected by this change.
Best regards,
--
Gurjeet Singh
EnterpriseDB Corporation
The Enterprise PostgreSQL Company
EnterpriseDB Corporation
The Enterprise PostgreSQL Company
On Sat, Jul 14, 2012 at 4:02 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > In bug #6734 we have a complaint about a longstanding misfeature of > CREATE TABLE LIKE. Ordinarily, this command doesn't select names for > copied indexes, but leaves that to be done at runtime by DefineIndex. > But if it's copying comments, and an index of the source table has a > comment, it's forced to preassign a name to the new index so that it can > build a CommentStmt that can apply the comment after the index is made. > Apart from being something of a modularity violation, this isn't very safe > because of the danger of name collision with earlier indexes for the new > table. And that's exactly what's happening in bug #6734. > > I suggested that we could dodge the problem by allowing IndexStmt to > carry a comment to be attached to the new index, and thereby avoid > needing an explicit COMMENT command. Attached is a patch that fixes it > that way. I agree with this approach. I think it's pretty much always a bad idea for DDL command A to fake up a parse node of the type used by DDL command B. It tends to make the code ugly and unmaintainable and propagates nasty abstraction violations all over the place. We should really aim to break every DDL command into a high-level part that does permissions checks, sanity checks, locking, etc. and a low-level part that actually performs the requested operation. In the case of comments, we happen to have it broken up pretty much correctly, with CreateComments and CreateSharedComments as the workhorse routines and CommentObject as the high-level routine; there are other places where things are not so happy. > While I was at it, it seemed like DefineIndex's parameter list had grown > well past any sane bound, so I refactored it to pass the IndexStmt > struct as-is rather than passing all the fields individually. I agree with this as well. > With or without that choice, though, this approach means a change in > DefineIndex's API, as well as the contents of struct IndexStmt. That > means it's probably unsafe to back-patch, since it seems plausible that > there might be third-party code out there that creates indexes and would > use these interfaces. > > I would like to sneak this fix into 9.2, though. Does anyone think > it's already too late to be touching these APIs for 9.2? I do not. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Robert Haas <robertmhaas@gmail.com> writes:
> On Sat, Jul 14, 2012 at 4:02 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> I suggested that we could dodge the problem by allowing IndexStmt to
>> carry a comment to be attached to the new index, and thereby avoid
>> needing an explicit COMMENT command. Attached is a patch that fixes it
>> that way.
> I agree with this approach. I think it's pretty much always a bad
> idea for DDL command A to fake up a parse node of the type used by DDL
> command B. It tends to make the code ugly and unmaintainable and
> propagates nasty abstraction violations all over the place.
Hmm, well, if that's the argument for doing this then we really need to
throw away the entire implementation of CREATE TABLE LIKE, because it's
doing that all over the place; I'm only proposing to remove one specific
instance.
regards, tom lane
On Mon, Jul 16, 2012 at 12:43 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Robert Haas <robertmhaas@gmail.com> writes: >> On Sat, Jul 14, 2012 at 4:02 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: >>> I suggested that we could dodge the problem by allowing IndexStmt to >>> carry a comment to be attached to the new index, and thereby avoid >>> needing an explicit COMMENT command. Attached is a patch that fixes it >>> that way. > >> I agree with this approach. I think it's pretty much always a bad >> idea for DDL command A to fake up a parse node of the type used by DDL >> command B. It tends to make the code ugly and unmaintainable and >> propagates nasty abstraction violations all over the place. > > Hmm, well, if that's the argument for doing this then we really need to > throw away the entire implementation of CREATE TABLE LIKE, because it's > doing that all over the place; I'm only proposing to remove one specific > instance. The problem isn't confined to CREATE TABLE LIKE; it's a widespread design flaw that will likely take years of work to clean up completely. I don't think that's a reason not to commit your change though; it fixes a bug and is an incremental improvement, even if a small one. That having been said, if you're feeling an urge to tackle the problem more broadly, don't let me stand in your way... -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Robert Haas <robertmhaas@gmail.com> writes:
> The problem isn't confined to CREATE TABLE LIKE; it's a widespread
> design flaw that will likely take years of work to clean up
> completely. I don't think that's a reason not to commit your change
> though; it fixes a bug and is an incremental improvement, even if a
> small one. That having been said, if you're feeling an urge to tackle
> the problem more broadly, don't let me stand in your way...
Not me; I'm just trying to close out a bug report.
FWIW, I think your argument only barely supports this change at all,
because CREATE TABLE LIKE is still generating an IndexStmt which after
all is the representation of a CREATE INDEX command. We've overloaded
it to do a few other things, and now it will be able to do one more
thing, but this isn't moving things at all towards separating high-
and low-level operations.
regards, tom lane
On Mon, Jul 16, 2012 at 1:10 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Robert Haas <robertmhaas@gmail.com> writes: >> The problem isn't confined to CREATE TABLE LIKE; it's a widespread >> design flaw that will likely take years of work to clean up >> completely. I don't think that's a reason not to commit your change >> though; it fixes a bug and is an incremental improvement, even if a >> small one. That having been said, if you're feeling an urge to tackle >> the problem more broadly, don't let me stand in your way... > > Not me; I'm just trying to close out a bug report. > > FWIW, I think your argument only barely supports this change at all, > because CREATE TABLE LIKE is still generating an IndexStmt which after > all is the representation of a CREATE INDEX command. We've overloaded > it to do a few other things, and now it will be able to do one more > thing, but this isn't moving things at all towards separating high- > and low-level operations. :-( -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company