don't mark indexes invalid unnecessarily
While working on FKs pointing to partitioned tables, I noticed that in
PG11 we fail to produce a working dump in the case of a partitioned
table that doesn't have partitions.
The attached patch fixes that. In doing so, it breaks a test ... and
analyzing that, it turns out that the test was broken, it wasn't testing
what it was supposed to be testing. I patched it so that it continues
to work (and now it tests the correct thing). To be precise, the test
was doing this:
create table parted_conflict (a int, b text) partition by range (a);
create table parted_conflict_1 partition of parted_conflict for values from (0) to (1000) partition by range (a);
create unique index on only parted_conflict_1 (a);
create unique index on only parted_conflict (a);
alter index parted_conflict_a_idx attach partition parted_conflict_1_a_idx;
create table parted_conflict_1_1 partition of parted_conflict_1 for values from (0) to (500);
with the expectation that the partition would not have a proper index on
which to run an INSERT INTO parted_conflict_1 ON CONFLICT (a), expecting
that partition parted_conflict_1_1 would not have the index. But in
reality parted_conflict_1_1 does have the index (and it only fails
because the index in its parent is marked invalid). So the patch moves
the CREATE TABLE parted_conflict_1_1 to before the indexes creation, so
that the partition really does not have the index, and then it gets the
expected error.
If you were to run the pg_upgrade test with my fks-to-partitioned-tables
patch, it will fail because of a PK on an partitionless partitioned
table being marked invalid after restore; but if you run it after
applying this patch, it should work (it works for me).
--
�lvaro Herrera 39�50'S 73�21'W
Attachments:
needless-invalid.patchtext/x-diff; charset=us-asciiDownload
commit 444e92a363bafc65dde710d3c21a5f0b25b308e1[m
Author: Alvaro Herrera <alvherre@alvh.no-ip.org>
AuthorDate: Mon Dec 3 19:30:37 2018 -0300
CommitDate: Mon Dec 3 19:31:04 2018 -0300
Don't mark partitioned indexes invalid unnecessarily
If the partitioned table doesn't have partitions, then an index created
with CREATE INDEX/ON ONLY doesn't have to be marked as invalid ... and
doing so breaks pg_dump for those tables, so don't.
diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c
index 3975f62c00..965b9f0d23 100644
--- a/src/backend/commands/indexcmds.c
+++ b/src/backend/commands/indexcmds.c
@@ -834,8 +834,18 @@ DefineIndex(Oid relationId,
flags |= INDEX_CREATE_PARTITIONED;
if (stmt->primary)
flags |= INDEX_CREATE_IS_PRIMARY;
+
+ /*
+ * If the table is partitioned, and recursion was declined but partitions
+ * exist, mark the index as invalid.
+ */
if (partitioned && stmt->relation && !stmt->relation->inh)
- flags |= INDEX_CREATE_INVALID;
+ {
+ PartitionDesc pd = RelationGetPartitionDesc(rel);
+
+ if (pd->nparts != 0)
+ flags |= INDEX_CREATE_INVALID;
+ }
if (stmt->deferrable)
constr_flags |= INDEX_CONSTR_CREATE_DEFERRABLE;
diff --git a/src/test/regress/expected/insert_conflict.out b/src/test/regress/expected/insert_conflict.out
index 27cf5a01b3..a28611745c 100644
--- a/src/test/regress/expected/insert_conflict.out
+++ b/src/test/regress/expected/insert_conflict.out
@@ -876,10 +876,10 @@ drop table parted_conflict;
-- partition
create table parted_conflict (a int, b text) partition by range (a);
create table parted_conflict_1 partition of parted_conflict for values from (0) to (1000) partition by range (a);
+create table parted_conflict_1_1 partition of parted_conflict_1 for values from (0) to (500);
create unique index on only parted_conflict_1 (a);
create unique index on only parted_conflict (a);
alter index parted_conflict_a_idx attach partition parted_conflict_1_a_idx;
-create table parted_conflict_1_1 partition of parted_conflict_1 for values from (0) to (500);
insert into parted_conflict values (40, 'forty');
insert into parted_conflict_1 values (40, 'cuarenta')
on conflict (a) do update set b = excluded.b;
diff --git a/src/test/regress/sql/insert_conflict.sql b/src/test/regress/sql/insert_conflict.sql
index c677d70fb7..c68013e179 100644
--- a/src/test/regress/sql/insert_conflict.sql
+++ b/src/test/regress/sql/insert_conflict.sql
@@ -551,10 +551,10 @@ drop table parted_conflict;
-- partition
create table parted_conflict (a int, b text) partition by range (a);
create table parted_conflict_1 partition of parted_conflict for values from (0) to (1000) partition by range (a);
+create table parted_conflict_1_1 partition of parted_conflict_1 for values from (0) to (500);
create unique index on only parted_conflict_1 (a);
create unique index on only parted_conflict (a);
alter index parted_conflict_a_idx attach partition parted_conflict_1_a_idx;
-create table parted_conflict_1_1 partition of parted_conflict_1 for values from (0) to (500);
insert into parted_conflict values (40, 'forty');
insert into parted_conflict_1 values (40, 'cuarenta')
on conflict (a) do update set b = excluded.b;
Hi,
On 2018/12/04 7:50, Alvaro Herrera wrote:
While working on FKs pointing to partitioned tables, I noticed that in
PG11 we fail to produce a working dump in the case of a partitioned
table that doesn't have partitions.The attached patch fixes that.
The fix makes sense. I see that the fix is similar in spirit to:
commit 9139aa19423b736470f669e566f8ef6a7f19b801
Author: Stephen Frost <sfrost@snowman.net>
Date: Tue Apr 25 16:57:43 2017 -0400
Allow ALTER TABLE ONLY on partitioned tables
There is no need to forbid ALTER TABLE ONLY on partitioned tables,
when no partitions exist yet. This can be handy for users who are
building up their partitioned table independently and will create actual
partitions later.
In addition, this is how pg_dump likes to operate in certain instances.
In doing so, it breaks a test ... and
analyzing that, it turns out that the test was broken, it wasn't testing
what it was supposed to be testing.
Hadn't considered it this closely at the time, but...
I patched it so that it continues
to work (and now it tests the correct thing). To be precise, the test
was doing this:create table parted_conflict (a int, b text) partition by range (a);
create table parted_conflict_1 partition of parted_conflict for values from (0) to (1000) partition by range (a);
create unique index on only parted_conflict_1 (a);
create unique index on only parted_conflict (a);
alter index parted_conflict_a_idx attach partition parted_conflict_1_a_idx;
create table parted_conflict_1_1 partition of parted_conflict_1 for values from (0) to (500);with the expectation that the partition would not have a proper index on
which to run an INSERT INTO parted_conflict_1 ON CONFLICT (a), expecting
that partition parted_conflict_1_1 would not have the index. But in
reality parted_conflict_1_1 does have the index (and it only fails
because the index in its parent is marked invalid). So the patch moves
the CREATE TABLE parted_conflict_1_1 to before the indexes creation, so
that the partition really does not have the index, and then it gets the
expected error.
... isn't the test really trying to show that invalid unique index on
table mentioned in the insert command cannot be used to proceed with the
on conflict clause, and so isn't broken per se? But maybe I misunderstood
you.
Thanks,
Amit
On 2018-Dec-05, Amit Langote wrote:
Hi,
On 2018/12/04 7:50, Alvaro Herrera wrote:
While working on FKs pointing to partitioned tables, I noticed that in
PG11 we fail to produce a working dump in the case of a partitioned
table that doesn't have partitions.The attached patch fixes that.
The fix makes sense. I see that the fix is similar in spirit to:
commit 9139aa19423b736470f669e566f8ef6a7f19b801
Ooh, right, it's pretty much the same. I wasn't aware of this commit,
thanks for pointing it out.
I patched it so that it continues
to work (and now it tests the correct thing). To be precise, the test
was doing this:create table parted_conflict (a int, b text) partition by range (a);
create table parted_conflict_1 partition of parted_conflict for values from (0) to (1000) partition by range (a);
create unique index on only parted_conflict_1 (a);
create unique index on only parted_conflict (a);
alter index parted_conflict_a_idx attach partition parted_conflict_1_a_idx;
create table parted_conflict_1_1 partition of parted_conflict_1 for values from (0) to (500);with the expectation that the partition would not have a proper index on
which to run an INSERT INTO parted_conflict_1 ON CONFLICT (a), expecting
that partition parted_conflict_1_1 would not have the index. But in
reality parted_conflict_1_1 does have the index (and it only fails
because the index in its parent is marked invalid). So the patch moves
the CREATE TABLE parted_conflict_1_1 to before the indexes creation, so
that the partition really does not have the index, and then it gets the
expected error.... isn't the test really trying to show that invalid unique index on
table mentioned in the insert command cannot be used to proceed with the
on conflict clause, and so isn't broken per se? But maybe I misunderstood
you.
You're right, I misinterpreted one bit: I was thinking that when we
create the first partition parted_conflict_1_1, then the index
automatically created in it causes the index in parted_conflict_1 to
become valid. But that's not so: the index remains invalid. This seems
a bit broken in itself, but it's not really important because that's
precisely what's getting fixed by my patch, namely that the index
created ONLY is valid if there are no partitions. So if we want the
index to appear invalid (to continue to reproduce the scenario), we need
to create the partition first.
Thanks for reviewing.
--
�lvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Pushed, thanks for the review.
--
�lvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services