set relispartition when attaching child index
Hi,
It seems that DefineIndex() is forgetting to update_relispartition()
on a partition's index when it's attached to an index being added to
the parent. That results in unexpected behavior when adding a foreign
key referencing the parent.
create table foo (a int) partition by list (a);
create table foo1 partition of foo for values in (1);
alter table foo1 add primary key (a);
alter table foo add primary key (a);
select relname, relispartition from pg_class where relname = 'foo1_pkey';
relname | relispartition
-----------+----------------
foo1_pkey | f
(1 row)
create table bar (a int references foo);
ERROR: index for 24683 not found in partition foo1
Attached patch fixes that, but I haven't added any new tests.
PS: Came to know that that's the case when reading this blog on the
new foreign key feature:
https://www.depesz.com/2019/04/24/waiting-for-postgresql-12-support-foreign-keys-that-reference-partitioned-tables/
Thanks,
Amit
Attachments:
DefineIndex-update_relispartition.patchapplication/octet-stream; name=DefineIndex-update_relispartition.patchDownload
diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c
index a1c91b5fb8..cb92b1975e 100644
--- a/src/backend/commands/indexcmds.c
+++ b/src/backend/commands/indexcmds.c
@@ -1095,6 +1095,8 @@ DefineIndex(Oid relationId,
/* Attach index to parent and we're done. */
IndexSetParentIndex(cldidx, indexRelationId);
+ /* Also, set child index as the partition. */
+ update_relispartition(NULL, cldidxid, true);
if (createdConstraintId != InvalidOid)
ConstraintSetParentConstraint(cldConstrOid,
createdConstraintId,
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index aa7328ea40..a50a865d4c 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -525,8 +525,6 @@ static ObjectAddress ATExecAttachPartitionIdx(List **wqueue, Relation rel,
static void validatePartitionedIndex(Relation partedIdx, Relation partedTbl);
static void refuseDupeIndexAttach(Relation parentIdx, Relation partIdx,
Relation partitionTbl);
-static void update_relispartition(Relation classRel, Oid relationId,
- bool newval);
static List *GetParentedForeignKeyRefs(Relation partition);
static void ATDetachCheckNoForeignKeyRefs(Relation partition);
@@ -16407,7 +16405,7 @@ validatePartitionedIndex(Relation partedIdx, Relation partedTbl)
* classRel is the pg_class relation, already open and suitably locked.
* It can be passed as NULL, in which case it's opened and closed locally.
*/
-static void
+void
update_relispartition(Relation classRel, Oid relationId, bool newval)
{
HeapTuple tup;
diff --git a/src/include/commands/tablecmds.h b/src/include/commands/tablecmds.h
index 96927b900d..6898fa864c 100644
--- a/src/include/commands/tablecmds.h
+++ b/src/include/commands/tablecmds.h
@@ -57,6 +57,8 @@ extern void ExecuteTruncateGuts(List *explicit_rels, List *relids, List *relids_
DropBehavior behavior, bool restart_seqs);
extern void SetRelationHasSubclass(Oid relationId, bool relhassubclass);
+extern void update_relispartition(Relation classRel, Oid relationId,
+ bool newval);
extern ObjectAddress renameatt(RenameStmt *stmt);
On 2019-Apr-25, Amit Langote wrote:
It seems that DefineIndex() is forgetting to update_relispartition()
on a partition's index when it's attached to an index being added to
the parent. That results in unexpected behavior when adding a foreign
key referencing the parent.
Ah, thanks for fixing. I also read Depesz's post this morning and was
to see what was going on after I push the pg_dump fix.
I'll get this pushed later.
--
�lvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On 2019-Apr-25, Amit Langote wrote:
It seems that DefineIndex() is forgetting to update_relispartition()
on a partition's index when it's attached to an index being added to
the parent. That results in unexpected behavior when adding a foreign
key referencing the parent.
BTW, maybe IndexSetParentIndex ought to be the one calling
update_relispartition() ...
--
�lvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Thu, Apr 25, 2019 at 12:35 AM Alvaro Herrera
<alvherre@2ndquadrant.com> wrote:
On 2019-Apr-25, Amit Langote wrote:
It seems that DefineIndex() is forgetting to update_relispartition()
on a partition's index when it's attached to an index being added to
the parent. That results in unexpected behavior when adding a foreign
key referencing the parent.BTW, maybe IndexSetParentIndex ought to be the one calling
update_relispartition() ...
I thought so too, but other sites are doing what I did in the patch.
Thanks,
Amit
On Thu, Apr 25, 2019 at 12:38 AM Amit Langote <amitlangote09@gmail.com> wrote:
On Thu, Apr 25, 2019 at 12:35 AM Alvaro Herrera
<alvherre@2ndquadrant.com> wrote:On 2019-Apr-25, Amit Langote wrote:
It seems that DefineIndex() is forgetting to update_relispartition()
on a partition's index when it's attached to an index being added to
the parent. That results in unexpected behavior when adding a foreign
key referencing the parent.BTW, maybe IndexSetParentIndex ought to be the one calling
update_relispartition() ...I thought so too, but other sites are doing what I did in the patch.
Although, we wouldn't have this bug if it was IndexSetParentIndex
calling it. Maybe a good idea to do that now.
Thanks,
Amit
On Thu, Apr 25, 2019 at 12:39 AM Amit Langote <amitlangote09@gmail.com> wrote:
On Thu, Apr 25, 2019 at 12:38 AM Amit Langote <amitlangote09@gmail.com> wrote:
On Thu, Apr 25, 2019 at 12:35 AM Alvaro Herrera
<alvherre@2ndquadrant.com> wrote:On 2019-Apr-25, Amit Langote wrote:
It seems that DefineIndex() is forgetting to update_relispartition()
on a partition's index when it's attached to an index being added to
the parent. That results in unexpected behavior when adding a foreign
key referencing the parent.BTW, maybe IndexSetParentIndex ought to be the one calling
update_relispartition() ...I thought so too, but other sites are doing what I did in the patch.
Although, we wouldn't have this bug if it was IndexSetParentIndex
calling it. Maybe a good idea to do that now.
I tried that in the attached.
Thanks,
Amit
Attachments:
IndexSetParentIndex-update_relispartition.patchapplication/octet-stream; name=IndexSetParentIndex-update_relispartition.patchDownload
diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c
index a1c91b5fb8..c84ea7ad2a 100644
--- a/src/backend/commands/indexcmds.c
+++ b/src/backend/commands/indexcmds.c
@@ -89,6 +89,7 @@ static void RangeVarCallbackForReindexIndex(const RangeVar *relation,
Oid relId, Oid oldRelId, void *arg);
static bool ReindexRelationConcurrently(Oid relationOid, int options);
static void ReindexPartitionedIndex(Relation parentIdx);
+static void update_relispartition(Oid relationId, bool newval);
/*
* CheckIndexCompatible
@@ -3361,7 +3362,12 @@ IndexSetParentIndex(Relation partitionIdx, Oid parentOid)
/* set relhassubclass if an index partition has been added to the parent */
if (OidIsValid(parentOid))
+ {
SetRelationHasSubclass(parentOid, true);
+ update_relispartition(partRelid, true);
+ }
+ else
+ update_relispartition(partRelid, false);
if (fix_dependencies)
{
@@ -3399,3 +3405,29 @@ IndexSetParentIndex(Relation partitionIdx, Oid parentOid)
CommandCounterIncrement();
}
}
+
+/*
+ * Update the relispartition flag of the given relation to the given value.
+ *
+ * classRel is the pg_class relation, already open and suitably locked.
+ * It can be passed as NULL, in which case it's opened and closed locally.
+ */
+static void
+update_relispartition(Oid relationId, bool newval)
+{
+ HeapTuple tup;
+ HeapTuple newtup;
+ Form_pg_class classForm;
+ Relation classRel;
+
+ classRel = table_open(RelationRelationId, RowExclusiveLock);
+ tup = SearchSysCache1(RELOID, ObjectIdGetDatum(relationId));
+ newtup = heap_copytuple(tup);
+ classForm = (Form_pg_class) GETSTRUCT(newtup);
+ classForm->relispartition = newval;
+ CatalogTupleUpdate(classRel, &tup->t_self, newtup);
+ heap_freetuple(newtup);
+ ReleaseSysCache(tup);
+
+ table_close(classRel, RowExclusiveLock);
+}
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index aa7328ea40..0b86e8cd50 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -525,8 +525,6 @@ static ObjectAddress ATExecAttachPartitionIdx(List **wqueue, Relation rel,
static void validatePartitionedIndex(Relation partedIdx, Relation partedTbl);
static void refuseDupeIndexAttach(Relation parentIdx, Relation partIdx,
Relation partitionTbl);
-static void update_relispartition(Relation classRel, Oid relationId,
- bool newval);
static List *GetParentedForeignKeyRefs(Relation partition);
static void ATDetachCheckNoForeignKeyRefs(Relation partition);
@@ -15714,7 +15712,6 @@ AttachPartitionEnsureIndexes(Relation rel, Relation attachrel)
if (OidIsValid(constraintOid))
ConstraintSetParentConstraint(cldConstrOid, constraintOid,
RelationGetRelid(attachrel));
- update_relispartition(NULL, cldIdxId, true);
found = true;
CommandCounterIncrement();
@@ -15970,7 +15967,6 @@ ATExecDetachPartition(Relation rel, RangeVar *name)
idx = index_open(idxid, AccessExclusiveLock);
IndexSetParentIndex(idx, InvalidOid);
- update_relispartition(classRel, idxid, false);
/* If there's a constraint associated with the index, detach it too */
constrOid = get_relation_idx_constraint_oid(RelationGetRelid(partRel),
@@ -16268,7 +16264,6 @@ ATExecAttachPartitionIdx(List **wqueue, Relation parentIdx, RangeVar *name)
if (OidIsValid(constraintOid))
ConstraintSetParentConstraint(cldConstrId, constraintOid,
RelationGetRelid(partTbl));
- update_relispartition(NULL, partIdxId, true);
pfree(attmap);
@@ -16401,38 +16396,6 @@ validatePartitionedIndex(Relation partedIdx, Relation partedTbl)
}
}
-/*
- * Update the relispartition flag of the given relation to the given value.
- *
- * classRel is the pg_class relation, already open and suitably locked.
- * It can be passed as NULL, in which case it's opened and closed locally.
- */
-static void
-update_relispartition(Relation classRel, Oid relationId, bool newval)
-{
- HeapTuple tup;
- HeapTuple newtup;
- Form_pg_class classForm;
- bool opened = false;
-
- if (classRel == NULL)
- {
- classRel = table_open(RelationRelationId, RowExclusiveLock);
- opened = true;
- }
-
- tup = SearchSysCache1(RELOID, ObjectIdGetDatum(relationId));
- newtup = heap_copytuple(tup);
- classForm = (Form_pg_class) GETSTRUCT(newtup);
- classForm->relispartition = newval;
- CatalogTupleUpdate(classRel, &tup->t_self, newtup);
- heap_freetuple(newtup);
- ReleaseSysCache(tup);
-
- if (opened)
- table_close(classRel, RowExclusiveLock);
-}
-
/*
* Return an OID list of constraints that reference the given relation
* that are marked as having a parent constraints.
On 2019/04/25 0:55, Amit Langote wrote:
On Thu, Apr 25, 2019 at 12:39 AM Amit Langote <amitlangote09@gmail.com> wrote:
On Thu, Apr 25, 2019 at 12:38 AM Amit Langote <amitlangote09@gmail.com> wrote:
On Thu, Apr 25, 2019 at 12:35 AM Alvaro Herrera
<alvherre@2ndquadrant.com> wrote:On 2019-Apr-25, Amit Langote wrote:
It seems that DefineIndex() is forgetting to update_relispartition()
on a partition's index when it's attached to an index being added to
the parent. That results in unexpected behavior when adding a foreign
key referencing the parent.BTW, maybe IndexSetParentIndex ought to be the one calling
update_relispartition() ...I thought so too, but other sites are doing what I did in the patch.
Although, we wouldn't have this bug if it was IndexSetParentIndex
calling it. Maybe a good idea to do that now.I tried that in the attached.
BTW, this will need to be back-patched to 11.
Thanks,
Amit
On 2019-Apr-25, Amit Langote wrote:
BTW, this will need to be back-patched to 11.
Done, thanks for the patch. I added the test in master, but obviously
it doesn't work in pg11, so I just verified manually that relispartition
is set correctly. I don't think it's worth doing more, though there are
other things that are affected by a bogus relispartition marking for an
index (example: creating the index in the last partition that didn't
have it, should mark the index on parent valid; I think that would fail
to propagate to upper levels correctly.)
--
�lvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On 2019/04/26 23:12, Alvaro Herrera wrote:
On 2019-Apr-25, Amit Langote wrote:
BTW, this will need to be back-patched to 11.
Done, thanks for the patch. I added the test in master, but obviously
it doesn't work in pg11, so I just verified manually that relispartition
is set correctly.
Thank you.
I don't think it's worth doing more, though there are
other things that are affected by a bogus relispartition marking for an
index (example: creating the index in the last partition that didn't
have it, should mark the index on parent valid; I think that would fail
to propagate to upper levels correctly.)
Hmm, I couldn't see any misbehavior for this example:
create table p (a int, b int) partition by list (a);
create table p1 partition of p for values in (1) partition by list (b);
create table p11 partition of p1 for values in (1);
create index on only p (a);
create index on only p1 (a);
alter index p_a_idx attach partition p1_a_idx ;
select relname, relispartition from pg_class where relname like 'p%idx';
relname │ relispartition
──────────┼────────────────
p_a_idx │ f
p1_a_idx │ t
(2 rows)
\d p
Table "public.p"
Column │ Type │ Collation │ Nullable │ Default
────────┼─────────┼───────────┼──────────┼─────────
a │ integer │ │ │
b │ integer │ │ │
Partition key: LIST (a)
Indexes:
"p_a_idx" btree (a) INVALID
Number of partitions: 1 (Use \d+ to list them.)
create index on p11 (a);
alter index p1_a_idx attach partition p11_a_idx ;
select relname, relispartition from pg_class where relname like 'p%idx';
relname │ relispartition
───────────┼────────────────
p_a_idx │ f
p1_a_idx │ t
p11_a_idx │ t
(3 rows)
\d p
Table "public.p"
Column │ Type │ Collation │ Nullable │ Default
────────┼─────────┼───────────┼──────────┼─────────
a │ integer │ │ │
b │ integer │ │ │
Partition key: LIST (a)
Indexes:
"p_a_idx" btree (a)
Number of partitions: 1 (Use \d+ to list them.)
Maybe, because the code path we fixed has nothing to do with this case?
Thanks,
Amit