Dropping partitioned table drops a previously detached partition

Started by Ashutosh Bapatover 8 years ago7 messages

ashutosh.bapat@enterprisedb.com

over 8 years ago

2 attachment(s)

Hi,
If we detach a partition and drop the corresponding partitioned table,
it drops the once-partition now-standalone table as well.

create table prt1 (a int, b int) partition by range(a);
create table prt1_p1 partition of prt1 for values from (0) to (100);
select oid, relname, relpartbound, relispartition, relkind from
pg_class where relname like 'prt%';
oid | relname
-------+---------
16453 | prt1
16456 | prt1_p1
(2 rows)

alter table prt1 detach partition prt1_p1;

drop table prt1;

-- Oops, we deleted the once-partition now-standalone table as well
select oid, relname, relpartbound, relispartition, relkind from
pg_class where relname like 'prt%';
oid | relname | relpartbound | relispartition | relkind
-----+---------+--------------+----------------+---------
(0 rows)

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.

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.

--
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company

Attachments:

0001-Dependency-between-partitioned-table-and-partition_v1.patchtext/x-patch; charset=US-ASCII; name=0001-Dependency-between-partitioned-table-and-partition_v1.patchDownload

From 97a647c487761782340d7ebca82b6d3200d79142 Mon Sep 17 00:00:00 2001
From: Ashutosh Bapat <ashutosh.bapat@enterprisedb.com>
Date: Mon, 12 Jun 2017 16:32:53 +0530
Subject: [PATCH 1/2] Dependency between partitioned table and partition.

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.

Ashutosh Bapat.
---
 src/backend/commands/tablecmds.c |   35 +++++++++++++++++++++--------------
 1 file changed, 21 insertions(+), 14 deletions(-)

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);
-- 
1.7.9.5

0002-Macro-for-dendency-between-table-and-its-composite-t_v1.patchtext/x-patch; charset=US-ASCII; name=0002-Macro-for-dendency-between-table-and-its-composite-t_v1.patchDownload

From 35943c90783d17b61a2f2ea39230e0242a251e3e Mon Sep 17 00:00:00 2001
From: Ashutosh Bapat <ashutosh.bapat@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

Amit Langote

Langote_Amit_f8@lab.ntt.co.jp

over 8 years ago

In reply to: Ashutosh Bapat (#1)

Re: Dropping partitioned table drops a previously detached partition

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

Rahila Syed

rahilasyed90@gmail.com

over 8 years ago

In reply to: Amit Langote (#2)

2 attachment(s)

Re: Dropping partitioned table drops a previously detached partition

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)

* 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_f8@lab.ntt.co.jp> wrote:

Show quoted text

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

Attachments:

0001-Dependency-between-partitioned-table-and-partition_v2.patchtext/x-patch; charset=US-ASCII; name=0001-Dependency-between-partitioned-table-and-partition_v2.patchDownload

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

0002-Macro-for-dendency-between-table-and-its-composite-t_v1.patchtext/x-patch; charset=US-ASCII; name=0002-Macro-for-dendency-between-table-and-its-composite-t_v1.patchDownload

From 35943c90783d17b61a2f2ea39230e0242a251e3e Mon Sep 17 00:00:00 2001
From: Ashutosh Bapat <ashutosh.bapat@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

Robert Haas

robertmhaas@gmail.com

over 8 years ago

In reply to: Rahila Syed (#3)

Re: Dropping partitioned table drops a previously detached partition

On Tue, Jun 13, 2017 at 9:44 AM, Rahila Syed <rahilasyed90@gmail.com> wrote:

I have added tests to the
0001-Dependency-between-partitioned-table-and-partition_v1.patch. Please
find attached the v2 patch.

Thanks. Committed. I don't think the 0002 patch is an improvement -
sure, it keeps those things in sync, but it also makes the code harder
to read. On balance I think it's a negative.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Ashutosh Bapat

ashutosh.bapat@enterprisedb.com

over 8 years ago

In reply to: Rahila Syed (#3)

Re: Dropping partitioned table drops a previously detached partition

On Tue, Jun 13, 2017 at 7:14 PM, Rahila Syed <rahilasyed90@gmail.com> wrote:

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.

The comment is fine for dependency created by CREATE TABLE PARTITION
OF since that too goes through StoreCatalogInheritance1(). But it's
not correct for CREATE TABLE ... OF <composite type> since that does
not go through StoreCatalogInheritance1().

+/*
+ * 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_f8@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

--
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Ashutosh Bapat

ashutosh.bapat@enterprisedb.com

over 8 years ago

In reply to: Robert Haas (#4)

Re: Dropping partitioned table drops a previously detached partition

On Tue, Jun 13, 2017 at 9:23 PM, Robert Haas <robertmhaas@gmail.com> wrote:

On Tue, Jun 13, 2017 at 9:44 AM, Rahila Syed <rahilasyed90@gmail.com> wrote:

I have added tests to the
0001-Dependency-between-partitioned-table-and-partition_v1.patch. Please
find attached the v2 patch.

Thanks. Committed.

Thanks.

I don't think the 0002 patch is an improvement -
sure, it keeps those things in sync, but it also makes the code harder
to read. On balance I think it's a negative.

I don't think the code is hard to read, but I agree that the macro
name TABLE_COMPOSITE_TYPE_DEPENDENCY isn't conveying the real sense.
But that's not a topic for this thread. I will start a separate a
thread.

--
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Ashutosh Bapat

ashutosh.bapat@enterprisedb.com

over 8 years ago

In reply to: Ashutosh Bapat (#5)

Re: Dropping partitioned table drops a previously detached partition

On Wed, Jun 14, 2017 at 10:21 AM, Ashutosh Bapat
<ashutosh.bapat@enterprisedb.com> wrote:

On Tue, Jun 13, 2017 at 7:14 PM, Rahila Syed <rahilasyed90@gmail.com> wrote:

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.

The comment is fine for dependency created by CREATE TABLE PARTITION
OF since that too goes through StoreCatalogInheritance1(). But it's
not correct for CREATE TABLE ... OF <composite type> since that does
not go through StoreCatalogInheritance1().

I missed "or" part of the prologue while writing this. Now that I have
read it carefully, the prologue looks good to me.
--
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers