I reviewed and tested
0001-Dependency-between-partitioned-table-and-partition_v1.patch
It applies cleanly on master and make check passes.
Following are few comments:
>/*
> * Drop the dependency created by StoreCatalogInheritance1 (CREATE TABLE
> * INHERITS/ALTER TABLE INHERIT -- refclassid will be RelationRelationId)
or
> * heap_create_with_catalog (CREATE TABLE OF/ALTER TABLE OF -- refclassid
will
> * be TypeRelationId). There's no convenient way to do this, so go
trawling
> * through pg_depend.
> */
>static void
>drop_parent_dependency(Oid relid, Oid refclassid, Oid refobjid,
DependencyType deptype)
This is not directly related to this patch but still would like to mention.
drop_parent_dependency() is being used to drop the dependency created
by CREATE TABLE PARTITION OF command also, so probably the comment
needs to be modified to reflect that.
>+/*
>+ * Partition tables are expected to be dropped when the parent partitioned
>+ * table gets dropped. Hence for partitioning we use AUTO dependency.
>+ * Otherwise, for regular inheritance use NORMAL dependency.
A minor cosmetic correction:
+ Hence for *declarative* partitioning we use AUTO dependency
+ * Otherwise, for regular inheritance *we* use NORMAL dependency.
I have added tests to the
0001-Dependency-between-partitioned-table-and-partition_v1.patch. Please
find attached the v2 patch.
On Tue, Jun 13, 2017 at 10:25 AM, Amit Langote <
langote_amit...@lab.ntt.co.jp> wrote:
> Hi Ashutosh,
>
> On 2017/06/12 20:56, Ashutosh Bapat wrote:
> > Hi,
> > If we detach a partition and drop the corresponding partitioned table,
> > it drops the once-partition now-standalone table as well.
>
> Thanks for the report. Looks like 8b4d582d279d78 missed the bit about
> drop_parent_dependency() that you describe in your analysis below.
>
> > The reason for this is as follows
> > An AUTO dependency is recorded between a partitioned table and
> partition. In
> > case of inheritance we record a NORMAL dependency between parent
> > and child. While
> > detaching a partition, we call RemoveInheritance(), which should
> have taken
> > care of removing this dependency. But it removes the dependencies
> which are
> > marked as NORMAL. When we changed the dependency for partitioned
> case from
> > NORMAL to AUTO by updating StoreCatalogInheritance1(), this function
> was not
> > updated. This caused the dependency to linger behind even after
> > detaching the
> > partition, thus causing the now standalone table (which was once a
> > partition)
> > to be dropped when the parent partitioned table is dropped. This
> patch fixes
> > RemoveInheritance() to choose appropriate dependency.
> >
> > Attached patch 0001 to fix this.
>
> Looks good to me. Perhaps, the example in your email could be added as a
> test case.
>
> > I see a similar issue-in-baking in case we change the type of
> > dependency recorded between a table and the composite type associated
> > with using CREATE TABLE ... OF command. 0002 patch addresses the
> > potential issue by using a macro while creating and dropping the
> > dependency in corresponding functions. There might be more such
> > places, but I haven't searched for those.
>
> Might be a good idea too.
>
> Adding this to the open items list.
>
> Thanks,
> Amit
>
>
>
> --
> Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers
>
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index b61fda9..9aef67b 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -283,6 +283,14 @@ struct DropRelationCallbackState
#define ATT_COMPOSITE_TYPE 0x0010
#define ATT_FOREIGN_TABLE 0x0020
+/*
+ * Partition tables are expected to be dropped when the parent partitioned
+ * table gets dropped. Hence for partitioning we use AUTO dependency.
+ * Otherwise, for regular inheritance use NORMAL dependency.
+ */
+#define child_dependency_type(child_is_partition) \
+ ((child_is_partition) ? DEPENDENCY_AUTO : DEPENDENCY_NORMAL)
+
static void truncate_check_rel(Relation rel);
static List *MergeAttributes(List *schema, List *supers, char relpersistence,
bool is_partition, List **supOids, List **supconstr,
@@ -439,7 +447,8 @@ static void ATExecEnableDisableRule(Relation rel, char *rulename,
static void ATPrepAddInherit(Relation child_rel);
static ObjectAddress ATExecAddInherit(Relation child_rel, RangeVar *parent, LOCKMODE lockmode);
static ObjectAddress ATExecDropInherit(Relation rel, RangeVar *parent, LOCKMODE lockmode);
-static void drop_parent_dependency(Oid relid, Oid refclassid, Oid refobjid);
+static void drop_parent_dependency(Oid relid, Oid refclassid, Oid refobjid,
+ DependencyType deptype);
static ObjectAddress ATExecAddOf(Relation rel, const TypeName *ofTypename, LOCKMODE lockmode);
static void ATExecDropOf(Relation rel, LOCKMODE lockmode);
static void ATExecReplicaIdentity(Relation rel, ReplicaIdentityStmt *stmt, LOCKMODE lockmode);
@@ -2367,14 +2376,8 @@ StoreCatalogInheritance1(Oid relationId, Oid parentOid,
childobject.objectId = relationId;
childobject.objectSubId = 0;
- /*
- * Partition tables are expected to be dropped when the parent partitioned
- * table gets dropped.
- */
- if (child_is_partition)
- recordDependencyOn(&childobject, &parentobject, DEPENDENCY_AUTO);
- else
- recordDependencyOn(&childobject, &parentobject, DEPENDENCY_NORMAL);
+ recordDependencyOn(&childobject, &parentobject,
+ child_dependency_type(child_is_partition));
/*
* Post creation hook of this inheritance. Since object_access_hook
@@ -11666,7 +11669,8 @@ RemoveInheritance(Relation child_rel, Relation parent_rel)
drop_parent_dependency(RelationGetRelid(child_rel),
RelationRelationId,
- RelationGetRelid(parent_rel));
+ RelationGetRelid(parent_rel),
+ child_dependency_type(child_is_partition));
/*
* Post alter hook of this inherits. Since object_access_hook doesn't take
@@ -11686,7 +11690,8 @@ RemoveInheritance(Relation child_rel, Relation parent_rel)
* through pg_depend.
*/
static void
-drop_parent_dependency(Oid relid, Oid refclassid, Oid refobjid)
+drop_parent_dependency(Oid relid, Oid refclassid, Oid refobjid,
+ DependencyType deptype)
{
Relation catalogRelation;
SysScanDesc scan;
@@ -11718,7 +11723,7 @@ drop_parent_dependency(Oid relid, Oid refclassid, Oid refobjid)
if (dep->refclassid == refclassid &&
dep->refobjid == refobjid &&
dep->refobjsubid == 0 &&
- dep->deptype == DEPENDENCY_NORMAL)
+ dep->deptype == deptype)
CatalogTupleDelete(catalogRelation, &depTuple->t_self);
}
@@ -11839,7 +11844,8 @@ ATExecAddOf(Relation rel, const TypeName *ofTypename, LOCKMODE lockmode)
/* If the table was already typed, drop the existing dependency. */
if (rel->rd_rel->reloftype)
- drop_parent_dependency(relid, TypeRelationId, rel->rd_rel->reloftype);
+ drop_parent_dependency(relid, TypeRelationId, rel->rd_rel->reloftype,
+ DEPENDENCY_NORMAL);
/* Record a dependency on the new type. */
tableobj.classId = RelationRelationId;
@@ -11892,7 +11898,8 @@ ATExecDropOf(Relation rel, LOCKMODE lockmode)
* table is presumed enough rights. No lock required on the type, either.
*/
- drop_parent_dependency(relid, TypeRelationId, rel->rd_rel->reloftype);
+ drop_parent_dependency(relid, TypeRelationId, rel->rd_rel->reloftype,
+ DEPENDENCY_NORMAL);
/* Clear pg_class.reloftype */
relationRelation = heap_open(RelationRelationId, RowExclusiveLock);
diff --git a/src/test/regress/expected/alter_table.out b/src/test/regress/expected/alter_table.out
index 8aadbb8..d4dbe65 100644
--- a/src/test/regress/expected/alter_table.out
+++ b/src/test/regress/expected/alter_table.out
@@ -3385,6 +3385,19 @@ SELECT coninhcount, conislocal FROM pg_constraint WHERE conrelid = 'part_3_4'::r
(1 row)
DROP TABLE part_3_4;
+-- check that a detached partition is not dropped on dropping a partitioned table
+CREATE TABLE range_parted2 (
+ a int
+) PARTITION BY RANGE(a);
+CREATE TABLE part_rp PARTITION OF range_parted2 FOR VALUES FROM (0) to (100);
+ALTER TABLE range_parted2 DETACH PARTITION part_rp;
+DROP TABLE range_parted2;
+SELECT * from part_rp;
+ a
+---
+(0 rows)
+
+DROP TABLE part_rp;
-- Check ALTER TABLE commands for partitioned tables and partitions
-- cannot add/drop column to/from *only* the parent
ALTER TABLE ONLY list_parted2 ADD COLUMN c int;
diff --git a/src/test/regress/sql/alter_table.sql b/src/test/regress/sql/alter_table.sql
index c41b487..001717d 100644
--- a/src/test/regress/sql/alter_table.sql
+++ b/src/test/regress/sql/alter_table.sql
@@ -2204,6 +2204,16 @@ SELECT attinhcount, attislocal FROM pg_attribute WHERE attrelid = 'part_3_4'::re
SELECT coninhcount, conislocal FROM pg_constraint WHERE conrelid = 'part_3_4'::regclass AND conname = 'check_a';
DROP TABLE part_3_4;
+-- check that a detached partition is not dropped on dropping a partitioned table
+CREATE TABLE range_parted2 (
+ a int
+) PARTITION BY RANGE(a);
+CREATE TABLE part_rp PARTITION OF range_parted2 FOR VALUES FROM (0) to (100);
+ALTER TABLE range_parted2 DETACH PARTITION part_rp;
+DROP TABLE range_parted2;
+SELECT * from part_rp;
+DROP TABLE part_rp;
+
-- Check ALTER TABLE commands for partitioned tables and partitions
-- cannot add/drop column to/from *only* the parent
From 35943c90783d17b61a2f2ea39230e0242a251e3e Mon Sep 17 00:00:00 2001
From: Ashutosh Bapat <ashutosh.ba...@enterprisedb.com>
Date: Mon, 12 Jun 2017 16:49:07 +0530
Subject: [PATCH 2/2] Macro for dendency between table and its composite type.
Create a macro for type dependency between a table and the composite type
associated with it using CREATE/ALTER TABLE ... OF ... command, so as to use
the same dependency type while creating and dropping the dependency in
different functions.
Ashutosh Bapat.
---
src/backend/commands/tablecmds.c | 7 +++++--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 9aef67b..2ac3646 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -291,6 +291,9 @@ struct DropRelationCallbackState
#define child_dependency_type(child_is_partition) \
((child_is_partition) ? DEPENDENCY_AUTO : DEPENDENCY_NORMAL)
+/* Dependency between a table and its associated composite type. */
+#define TABLE_COMPOSITE_TYPE_DEPENDENCY DEPENDENCY_NORMAL
+
static void truncate_check_rel(Relation rel);
static List *MergeAttributes(List *schema, List *supers, char relpersistence,
bool is_partition, List **supOids, List **supconstr,
@@ -11845,7 +11848,7 @@ ATExecAddOf(Relation rel, const TypeName *ofTypename, LOCKMODE lockmode)
/* If the table was already typed, drop the existing dependency. */
if (rel->rd_rel->reloftype)
drop_parent_dependency(relid, TypeRelationId, rel->rd_rel->reloftype,
- DEPENDENCY_NORMAL);
+ TABLE_COMPOSITE_TYPE_DEPENDENCY);
/* Record a dependency on the new type. */
tableobj.classId = RelationRelationId;
@@ -11899,7 +11902,7 @@ ATExecDropOf(Relation rel, LOCKMODE lockmode)
*/
drop_parent_dependency(relid, TypeRelationId, rel->rd_rel->reloftype,
- DEPENDENCY_NORMAL);
+ TABLE_COMPOSITE_TYPE_DEPENDENCY);
/* Clear pg_class.reloftype */
relationRelation = heap_open(RelationRelationId, RowExclusiveLock);
--
1.7.9.5
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
