Hi Alvaro.
On 2018/01/23 7:55, Alvaro Herrera wrote:
> Alvaro Herrera wrote:
>> Version 4 of this patch, rebased on today's master.
With the latest patch, I noticed what I think is an unintended behavior.
create table p (a int, b int) partition by list (a);
create table p1 partition of p for values in (1) partition by range (b);
create table p11 partition of p1 for values from (1) to (10);
create table p2 partition of p for values in (2);
create unique index on p (a);
ERROR: insufficient columns in UNIQUE constraint definition
DETAIL: UNIQUE constraint on table "p1" lacks column "b" which is part of
the partition key.
It seems that after recursing to p1 which is itself partitioned,
DefineIndex() mistakenly looks for column b (which is in the p1's
partition key) in the unique key. I think that's unnecessary.
DefineIndex() should check that only once, that is, before recursing.
Please find attached a fix, a delta patch which applies on top of your v4
patch. With it:
create unique index on p (a);
insert into p values (1, 1);
insert into p values (1, 1);
ERROR: duplicate key value violates unique constraint "p11_a_idx"
DETAIL: Key (a)=(1) already exists.
insert into p values (2, 1);
insert into p values (2, 1);
ERROR: duplicate key value violates unique constraint "p2_a_idx"
DETAIL: Key (a)=(2) already exists.
drop index p_a_idx;
create unique index on p (a, b);
insert into p values (1, 1);
insert into p values (1, 1);
ERROR: duplicate key value violates unique constraint "p11_a_b_idx"
DETAIL: Key (a, b)=(1, 1) already exists.
insert into p values (2, 1);
insert into p values (2, 1);
ERROR: duplicate key value violates unique constraint "p2_a_b_idx"
DETAIL: Key (a, b)=(2, 1) already exists.
Am I missing something?
Thanks,
Amit
diff --git a/src/backend/bootstrap/bootparse.y
b/src/backend/bootstrap/bootparse.y
index 9e81f9514d..d15e13d984 100644
--- a/src/backend/bootstrap/bootparse.y
+++ b/src/backend/bootstrap/bootparse.y
@@ -327,6 +327,7 @@ Boot_DeclareIndexStmt:
false,
false,
true, /*
skip_build */
+ false,
false);
do_end();
}
@@ -373,6 +374,7 @@ Boot_DeclareUniqueIndexStmt:
false,
false,
true, /*
skip_build */
+ false,
false);
do_end();
}
diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c
index b0e5ede488..f994c478d9 100644
--- a/src/backend/commands/indexcmds.c
+++ b/src/backend/commands/indexcmds.c
@@ -325,7 +325,8 @@ DefineIndex(Oid relationId,
bool check_rights,
bool check_not_in_use,
bool skip_build,
- bool quiet)
+ bool quiet,
+ bool recursing)
{
char *indexRelationName;
char *accessMethodName;
@@ -642,8 +643,11 @@ DefineIndex(Oid relationId,
*
* We could lift this limitation if we had global indexes, but those
have
* their own problems, so this is a useful feature combination.
+ *
+ * If recursing for an index being defined on some ancestor, this must
+ * have been checked already.
*/
- if (partitioned && (stmt->unique || stmt->primary))
+ if (partitioned && (stmt->unique || stmt->primary) && !recursing)
{
PartitionKey key = rel->rd_partkey;
int i;
@@ -969,7 +973,7 @@ DefineIndex(Oid relationId,
indexRelationId, /* this is our child */
createdConstraintId,
false,
check_rights, check_not_in_use,
- false, quiet);
+ false, quiet,
true);
}
pfree(attmap);
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 2b22946c3c..a32fd4d86c 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -953,7 +953,7 @@ DefineRelation(CreateStmt *stmt, char relkind, Oid ownerId,
InvalidOid,
RelationGetRelid(idxRel),
constraintOid,
- false, false, false, false,
false);
+ false, false, false, false,
false, false);
index_close(idxRel, AccessShareLock);
}
@@ -6817,7 +6817,7 @@ ATExecAddIndex(AlteredTableInfo *tab, Relation rel,
check_rights,
false, /*
check_not_in_use - we did it already */
skip_build,
- quiet);
+ quiet, false);
/*
* If TryReuseIndex() stashed a relfilenode for us, we used it for the
new
@@ -14218,7 +14218,7 @@ AttachPartitionEnsureIndexes(Relation rel, Relation
attachrel)
DefineIndex(RelationGetRelid(attachrel), stmt,
InvalidOid,
RelationGetRelid(idxRel),
constraintOid,
- false, false, false, false,
false);
+ false, false, false, false,
false, false);
}
index_close(idxRel, AccessShareLock);
diff --git a/src/backend/tcop/utility.c b/src/backend/tcop/utility.c
index 8c23ee53e2..ab96c1f2ca 100644
--- a/src/backend/tcop/utility.c
+++ b/src/backend/tcop/utility.c
@@ -1358,7 +1358,8 @@ ProcessUtilitySlow(ParseState *pstate,
true,
/* check_rights */
true,
/* check_not_in_use */
false,
/* skip_build */
- false);
/* quiet */
+ false,
/* quiet */
+ false);
/* not recursing */
/*
* Add the CREATE INDEX node itself to
stash right away;
diff --git a/src/include/commands/defrem.h b/src/include/commands/defrem.h
index f510f40945..e38d173c3a 100644
--- a/src/include/commands/defrem.h
+++ b/src/include/commands/defrem.h
@@ -31,7 +31,8 @@ extern ObjectAddress DefineIndex(Oid relationId,
bool check_rights,
bool check_not_in_use,
bool skip_build,
- bool quiet);
+ bool quiet,
+ bool recursing);
extern void ReindexIndex(RangeVar *indexRelation, int options);
extern Oid ReindexTable(RangeVar *relation, int options);
extern void ReindexMultipleTables(const char *objectName, ReindexObjectType
objectKind,
