On 2018/10/05 16:05, Michael Paquier wrote:
>> As of commit 2fbdf1b38bc [1], which has been applied in 11 and HEAD
>> branches, RelationBuildPartitionDesc emits an error if we don't find
>> relpartbound set for a child found by scanning pg_inherits, instead of
>> skipping such children. While that commit switched the order of creating
>> pg_inherits entry and checking a new bound against existing bounds in
>> DefineRelation in light of aforementioned change, it didn't in
>> ATExecAttachPartition, hence this error.
>>
>> Attached patch fixes that.
>
> Could you please add a minimal regression test in your patch? That's
> the second bug related to ATTACH PARTITION I am looking at today..
OK, done, although I hope that's not bloating the tests.
>> I thought we'd need to apply this to 10, 11, HEAD, but I couldn't
>> reproduce this in 10. That's because the above commit wasn't applied to
>> 10, so the child that causes this error is being skipped in 10's case.
>
> Hmm. Indeed, v10 does not complain but HEAD does. (I ran the attached
> SQL file, which is the complete test case both of you have compiled).
Did you forget to attach some file?
Thanks,
Amit
diff --git a/src/backend/catalog/partition.c b/src/backend/catalog/partition.c
index 8219d05d83..75e41ed21c 100644
--- a/src/backend/catalog/partition.c
+++ b/src/backend/catalog/partition.c
@@ -195,23 +195,12 @@ RelationBuildPartitionDesc(Relation rel)
if (!HeapTupleIsValid(tuple))
elog(ERROR, "cache lookup failed for relation %u",
inhrelid);
- /*
- * It is possible that the pg_class tuple of a partition has
not been
- * updated yet to set its relpartbound field. The only case
where
- * this happens is when we open the parent relation to check
using its
- * partition descriptor that a new partition's bound does not
overlap
- * some existing partition.
- */
- if (!((Form_pg_class) GETSTRUCT(tuple))->relispartition)
- {
- ReleaseSysCache(tuple);
- continue;
- }
-
datum = SysCacheGetAttr(RELOID, tuple,
Anum_pg_class_relpartbound,
&isnull);
- Assert(!isnull);
+ if(isnull)
+ elog(ERROR, "null relpartbound for relation %u",
inhrelid);
+
boundspec = (Node *) stringToNode(TextDatumGetCString(datum));
boundspecs = lappend(boundspecs, boundspec);
partoids = lappend_oid(partoids, inhrelid);
@@ -1856,8 +1845,7 @@ generate_partition_qual(Relation rel)
Anum_pg_class_relpartbound,
&isnull);
if (isnull) /* should not happen */
- elog(ERROR, "relation \"%s\" has relpartbound = null",
- RelationGetRelationName(rel));
+ elog(ERROR, "null relpartbound for relation %u",
RelationGetRelid(rel));
bound = castNode(PartitionBoundSpec,
stringToNode(TextDatumGetCString(boundDatum)));
ReleaseSysCache(tuple);
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index f143101b5d..43b76812d2 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -745,9 +745,6 @@ DefineRelation(CreateStmt *stmt, char relkind, Oid ownerId,
false,
typaddress);
- /* Store inheritance information for new rel. */
- StoreCatalogInheritance(relationId, inheritOids, stmt->partbound !=
NULL);
-
/*
* We must bump the command counter to make the newly-created relation
* tuple visible for opening.
@@ -809,6 +806,9 @@ DefineRelation(CreateStmt *stmt, char relkind, Oid ownerId,
CommandCounterIncrement();
}
+ /* Store inheritance information for new rel. */
+ StoreCatalogInheritance(relationId, inheritOids, stmt->partbound !=
NULL);
+
/*
* Process the partitioning specification (if any) and store the
partition
* key information into the catalog.
@@ -13611,9 +13611,6 @@ ATExecAttachPartition(List **wqueue, Relation rel,
PartitionCmd *cmd)
trigger_name,
RelationGetRelationName(attachrel)),
errdetail("ROW triggers with transition tables
are not supported on partitions")));
- /* OK to create inheritance. Rest of the checks performed there */
- CreateInheritance(attachrel, rel);
-
/*
* Check that the new partition's bound is valid and does not overlap
any
* of existing partitions of the parent - note that it does not return
on
@@ -13622,6 +13619,9 @@ ATExecAttachPartition(List **wqueue, Relation rel,
PartitionCmd *cmd)
check_new_partition_bound(RelationGetRelationName(attachrel), rel,
cmd->bound);
+ /* OK to create inheritance. Rest of the checks performed there */
+ CreateInheritance(attachrel, rel);
+
/* Update the pg_class entry. */
StorePartitionBound(attachrel, rel, cmd->bound);
@@ -13837,10 +13837,6 @@ ATExecDetachPartition(Relation rel, RangeVar *name)
RelationGetRelid(partRel));
Assert(((Form_pg_class) GETSTRUCT(tuple))->relispartition);
- (void) SysCacheGetAttr(RELOID, tuple, Anum_pg_class_relpartbound,
- &isnull);
- Assert(!isnull);
-
/* Clear relpartbound and reset relispartition */
memset(new_val, 0, sizeof(new_val));
memset(new_null, false, sizeof(new_null));
diff --git a/src/test/regress/expected/alter_table.out
b/src/test/regress/expected/alter_table.out
index 86127ce0df..bd3f918e48 100644
--- a/src/test/regress/expected/alter_table.out
+++ b/src/test/regress/expected/alter_table.out
@@ -3669,3 +3669,18 @@ alter table temp_part_parent attach partition
temp_part_child
for values in (1, 2); -- ok
drop table perm_part_parent cascade;
drop table temp_part_parent cascade;
+-- test case where the partitioning operator is a SQL function whose
+-- evaluation results in the table's relcache being rebuilt partway through
+-- the execution of an ATTACH PARTITION command
+create function at_test_sql_partop (int4, int4) returns int language sql
+as $$ select case when $1 = $2 then 0 when $1 > $2 then 1 else -1 end; $$;
+create operator class at_test_sql_partop for type int4 using btree as
+ operator 1 < (int4, int4), operator 2 <= (int4, int4),
+ operator 3 = (int4, int4), operator 4 >= (int4, int4),
+ operator 5 > (int4, int4), function 1 at_test_sql_partop(int4, int4);
+create table at_test_sql_partop (a int) partition by range (a
at_test_sql_partop);
+create table at_test_sql_partop_1 (a int);
+alter table at_test_sql_partop attach partition at_test_sql_partop_1 for
values from (0) to (10);
+drop table at_test_sql_partop;
+drop operator class at_test_sql_partop using btree;
+drop function at_test_sql_partop;
diff --git a/src/test/regress/sql/alter_table.sql
b/src/test/regress/sql/alter_table.sql
index 519d984fd9..24b60f9b6e 100644
--- a/src/test/regress/sql/alter_table.sql
+++ b/src/test/regress/sql/alter_table.sql
@@ -2388,3 +2388,19 @@ alter table temp_part_parent attach partition
temp_part_child
for values in (1, 2); -- ok
drop table perm_part_parent cascade;
drop table temp_part_parent cascade;
+
+-- test case where the partitioning operator is a SQL function whose
+-- evaluation results in the table's relcache being rebuilt partway through
+-- the execution of an ATTACH PARTITION command
+create function at_test_sql_partop (int4, int4) returns int language sql
+as $$ select case when $1 = $2 then 0 when $1 > $2 then 1 else -1 end; $$;
+create operator class at_test_sql_partop for type int4 using btree as
+ operator 1 < (int4, int4), operator 2 <= (int4, int4),
+ operator 3 = (int4, int4), operator 4 >= (int4, int4),
+ operator 5 > (int4, int4), function 1 at_test_sql_partop(int4, int4);
+create table at_test_sql_partop (a int) partition by range (a
at_test_sql_partop);
+create table at_test_sql_partop_1 (a int);
+alter table at_test_sql_partop attach partition at_test_sql_partop_1 for
values from (0) to (10);
+drop table at_test_sql_partop;
+drop operator class at_test_sql_partop using btree;
+drop function at_test_sql_partop;
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index c145385f84..2d0043d1db 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -14247,9 +14247,6 @@ ATExecAttachPartition(List **wqueue, Relation rel,
PartitionCmd *cmd)
trigger_name,
RelationGetRelationName(attachrel)),
errdetail("ROW triggers with transition tables
are not supported on partitions")));
- /* OK to create inheritance. Rest of the checks performed there */
- CreateInheritance(attachrel, rel);
-
/*
* Check that the new partition's bound is valid and does not overlap
any
* of existing partitions of the parent - note that it does not return
on
@@ -14258,6 +14255,9 @@ ATExecAttachPartition(List **wqueue, Relation rel,
PartitionCmd *cmd)
check_new_partition_bound(RelationGetRelationName(attachrel), rel,
cmd->bound);
+ /* OK to create inheritance. Rest of the checks performed there */
+ CreateInheritance(attachrel, rel);
+
/* Update the pg_class entry. */
StorePartitionBound(attachrel, rel, cmd->bound);
diff --git a/src/test/regress/expected/alter_table.out
b/src/test/regress/expected/alter_table.out
index dccc9b27c5..be0b54cc52 100644
--- a/src/test/regress/expected/alter_table.out
+++ b/src/test/regress/expected/alter_table.out
@@ -3960,3 +3960,18 @@ ERROR: cannot attach a temporary relation as partition
of permanent relation "p
alter table temp_part_parent attach partition temp_part_child default; -- ok
drop table perm_part_parent cascade;
drop table temp_part_parent cascade;
+-- test case where the partitioning operator is a SQL function whose
+-- evaluation results in the table's relcache being rebuilt partway through
+-- the execution of an ATTACH PARTITION command
+create function at_test_sql_partop (int4, int4) returns int language sql
+as $$ select case when $1 = $2 then 0 when $1 > $2 then 1 else -1 end; $$;
+create operator class at_test_sql_partop for type int4 using btree as
+ operator 1 < (int4, int4), operator 2 <= (int4, int4),
+ operator 3 = (int4, int4), operator 4 >= (int4, int4),
+ operator 5 > (int4, int4), function 1 at_test_sql_partop(int4, int4);
+create table at_test_sql_partop (a int) partition by range (a
at_test_sql_partop);
+create table at_test_sql_partop_1 (a int);
+alter table at_test_sql_partop attach partition at_test_sql_partop_1 for
values from (0) to (10);
+drop table at_test_sql_partop;
+drop operator class at_test_sql_partop using btree;
+drop function at_test_sql_partop;
diff --git a/src/test/regress/sql/alter_table.sql
b/src/test/regress/sql/alter_table.sql
index b90497804b..179bbfb9a1 100644
--- a/src/test/regress/sql/alter_table.sql
+++ b/src/test/regress/sql/alter_table.sql
@@ -2618,3 +2618,19 @@ alter table perm_part_parent attach partition
temp_part_child default; -- error
alter table temp_part_parent attach partition temp_part_child default; -- ok
drop table perm_part_parent cascade;
drop table temp_part_parent cascade;
+
+-- test case where the partitioning operator is a SQL function whose
+-- evaluation results in the table's relcache being rebuilt partway through
+-- the execution of an ATTACH PARTITION command
+create function at_test_sql_partop (int4, int4) returns int language sql
+as $$ select case when $1 = $2 then 0 when $1 > $2 then 1 else -1 end; $$;
+create operator class at_test_sql_partop for type int4 using btree as
+ operator 1 < (int4, int4), operator 2 <= (int4, int4),
+ operator 3 = (int4, int4), operator 4 >= (int4, int4),
+ operator 5 > (int4, int4), function 1 at_test_sql_partop(int4, int4);
+create table at_test_sql_partop (a int) partition by range (a
at_test_sql_partop);
+create table at_test_sql_partop_1 (a int);
+alter table at_test_sql_partop attach partition at_test_sql_partop_1 for
values from (0) to (10);
+drop table at_test_sql_partop;
+drop operator class at_test_sql_partop using btree;
+drop function at_test_sql_partop;
