On Tue, Nov 06, 2018 at 07:04:17PM +0900, Amit Langote wrote: > Agree with keeping it simple. Maybe, we could (should?) document that the > only ON COMMIT action that works when specified with partitioned parent > table is DROP (other actions are essentially ignored)?
I have been thinking about this one, and here are some ideas: - for ON COMMIT DROP: When used on a partitioned table or a table with inheritance children, this drops the depending partitions and children. - for ON DELETE ROWS: When used on a partitioned table, this is not cascaded to its partitions. > Seems fine to me. Thanks for looking at the patch. > + > + /* > + * Truncate relations before dropping so as all dependencies between > > so as -> so that Fixed. > + * relations are removed after they are worked on. This is a waste as it > + * could be possible that a relation truncated needs also to be removed > + * per dependency games but this makes the whole logic more robust and > + * there is no need to re-check that a relation exists afterwards. > + */ > > "afterwords" seems redundant, but may I suggest rewriting the 2nd sentence as: > > Doing it like this might be a waste as it's possible that a relation being > truncated will be dropped anyway due to it's parent being dropped, but > this makes the code more robust because of not having to re-check that the > relation exists. Your comment sounds better, so added. -- Michael
diff --git a/doc/src/sgml/ref/create_table.sgml b/doc/src/sgml/ref/create_table.sgml
index 10428f8ff0..98c5a01416 100644
--- a/doc/src/sgml/ref/create_table.sgml
+++ b/doc/src/sgml/ref/create_table.sgml
@@ -1215,7 +1215,8 @@ WITH ( MODULUS <replaceable class="parameter">numeric_literal</replaceable>, REM
All rows in the temporary table will be deleted at the end
of each transaction block. Essentially, an automatic <xref
linkend="sql-truncate"/> is done
- at each commit.
+ at each commit. When used on a partitioned table, this
+ is not cascaded to its partitions.
</para>
</listitem>
</varlistentry>
@@ -1225,7 +1226,9 @@ WITH ( MODULUS <replaceable class="parameter">numeric_literal</replaceable>, REM
<listitem>
<para>
The temporary table will be dropped at the end of the current
- transaction block.
+ transaction block. When used on a partitioned table or a
+ table with inheritance children, this drops the depending
+ partitions and children.
</para>
</listitem>
</varlistentry>
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 6048334c75..f966f5c431 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -13300,6 +13300,7 @@ PreCommit_on_commit_actions(void)
{
ListCell *l;
List *oids_to_truncate = NIL;
+ List *oids_to_drop = NIL;
foreach(l, on_commits)
{
@@ -13326,36 +13327,66 @@ PreCommit_on_commit_actions(void)
oids_to_truncate = lappend_oid(oids_to_truncate, oc->relid);
break;
case ONCOMMIT_DROP:
- {
- ObjectAddress object;
-
- object.classId = RelationRelationId;
- object.objectId = oc->relid;
- object.objectSubId = 0;
-
- /*
- * Since this is an automatic drop, rather than one
- * directly initiated by the user, we pass the
- * PERFORM_DELETION_INTERNAL flag.
- */
- performDeletion(&object,
- DROP_CASCADE, PERFORM_DELETION_INTERNAL);
-
- /*
- * Note that table deletion will call
- * remove_on_commit_action, so the entry should get marked
- * as deleted.
- */
- Assert(oc->deleting_subid != InvalidSubTransactionId);
- break;
- }
+ oids_to_drop = lappend_oid(oids_to_drop, oc->relid);
+ break;
}
}
+
+ /*
+ * Truncate relations before dropping so that all dependencies between
+ * relations are removed after they are worked on. Doing it like this
+ * might be a waste as it is possible that a relation being truncated
+ * will be dropped anyway due to its parent being dropped, but this
+ * makes the code more robust because of not having to re-check that the
+ * relation exists at truncation time.
+ */
if (oids_to_truncate != NIL)
{
heap_truncate(oids_to_truncate);
CommandCounterIncrement(); /* XXX needed? */
}
+ if (oids_to_drop != NIL)
+ {
+ ObjectAddresses *targetObjects = new_object_addresses();
+ ListCell *l;
+
+ foreach(l, oids_to_drop)
+ {
+ ObjectAddress object;
+
+ object.classId = RelationRelationId;
+ object.objectId = lfirst_oid(l);
+ object.objectSubId = 0;
+
+ Assert(!object_address_present(&object, targetObjects));
+
+ add_exact_object_address(&object, targetObjects);
+ }
+
+ /*
+ * Since this is an automatic drop, rather than one directly initiated
+ * by the user, we pass the PERFORM_DELETION_INTERNAL flag.
+ */
+ performMultipleDeletions(targetObjects, DROP_CASCADE,
+ PERFORM_DELETION_INTERNAL | PERFORM_DELETION_QUIETLY);
+
+#ifdef USE_ASSERT_CHECKING
+
+ /*
+ * Note that table deletion will call remove_on_commit_action, so the
+ * entry should get marked as deleted.
+ */
+ foreach(l, on_commits)
+ {
+ OnCommitItem *oc = (OnCommitItem *) lfirst(l);
+
+ if (oc->oncommit != ONCOMMIT_DROP)
+ continue;
+
+ Assert(oc->deleting_subid != InvalidSubTransactionId);
+ }
+#endif
+ }
}
/*
diff --git a/src/test/regress/expected/temp.out b/src/test/regress/expected/temp.out
index a769abe9bb..f018f17ca0 100644
--- a/src/test/regress/expected/temp.out
+++ b/src/test/regress/expected/temp.out
@@ -216,3 +216,88 @@ select * from temp_parted_oncommit;
(0 rows)
drop table temp_parted_oncommit;
+-- Check dependencies between ON COMMIT actions with a partitioned
+-- table and its partitions. Using ON COMMIT DROP on a parent removes
+-- the whole set.
+begin;
+create temp table temp_parted_oncommit_test (a int)
+ partition by list (a) on commit drop;
+create temp table temp_parted_oncommit_test1
+ partition of temp_parted_oncommit_test
+ for values in (1) on commit delete rows;
+create temp table temp_parted_oncommit_test2
+ partition of temp_parted_oncommit_test
+ for values in (2) on commit drop;
+insert into temp_parted_oncommit_test values (1), (2);
+commit;
+-- no relations remain in this case.
+select relname from pg_class where relname like 'temp_parted_oncommit_test%';
+ relname
+---------
+(0 rows)
+
+-- Using ON COMMIT DELETE on a partitioned table does not remove
+-- all rows if partitions preserve their data.
+begin;
+create temp table temp_parted_oncommit_test (a int)
+ partition by list (a) on commit delete rows;
+create temp table temp_parted_oncommit_test1
+ partition of temp_parted_oncommit_test
+ for values in (1) on commit preserve rows;
+create temp table temp_parted_oncommit_test2
+ partition of temp_parted_oncommit_test
+ for values in (2) on commit drop;
+insert into temp_parted_oncommit_test values (1), (2);
+commit;
+-- Data from the remaining partition is still here as its rows are
+-- preserved.
+select * from temp_parted_oncommit_test;
+ a
+---
+ 1
+(1 row)
+
+-- two relations remain in this case.
+select relname from pg_class where relname like 'temp_parted_oncommit_test%';
+ relname
+----------------------------
+ temp_parted_oncommit_test
+ temp_parted_oncommit_test1
+(2 rows)
+
+drop table temp_parted_oncommit_test;
+-- Check dependencies between ON COMMIT actions with inheritance trees.
+-- Using ON COMMIT DROP on a parent removes the whole set.
+begin;
+create temp table temp_inh_oncommit_test (a int) on commit drop;
+create temp table temp_inh_oncommit_test1 ()
+ inherits(temp_inh_oncommit_test) on commit delete rows;
+insert into temp_inh_oncommit_test1 values (1);
+commit;
+-- no relations remain in this case
+select relname from pg_class where relname like 'temp_inh_oncommit_test%';
+ relname
+---------
+(0 rows)
+
+-- Data on the parent is removed, and the child goes away.
+begin;
+create temp table temp_inh_oncommit_test (a int) on commit delete rows;
+create temp table temp_inh_oncommit_test1 ()
+ inherits(temp_inh_oncommit_test) on commit drop;
+insert into temp_inh_oncommit_test1 values (1);
+insert into temp_inh_oncommit_test values (1);
+commit;
+select * from temp_inh_oncommit_test;
+ a
+---
+(0 rows)
+
+-- one relation remains
+select relname from pg_class where relname like 'temp_inh_oncommit_test%';
+ relname
+------------------------
+ temp_inh_oncommit_test
+(1 row)
+
+drop table temp_inh_oncommit_test;
diff --git a/src/test/regress/sql/temp.sql b/src/test/regress/sql/temp.sql
index 1074c7cfac..1beccc6ceb 100644
--- a/src/test/regress/sql/temp.sql
+++ b/src/test/regress/sql/temp.sql
@@ -165,3 +165,62 @@ commit;
-- partitions are emptied by the previous commit
select * from temp_parted_oncommit;
drop table temp_parted_oncommit;
+
+-- Check dependencies between ON COMMIT actions with a partitioned
+-- table and its partitions. Using ON COMMIT DROP on a parent removes
+-- the whole set.
+begin;
+create temp table temp_parted_oncommit_test (a int)
+ partition by list (a) on commit drop;
+create temp table temp_parted_oncommit_test1
+ partition of temp_parted_oncommit_test
+ for values in (1) on commit delete rows;
+create temp table temp_parted_oncommit_test2
+ partition of temp_parted_oncommit_test
+ for values in (2) on commit drop;
+insert into temp_parted_oncommit_test values (1), (2);
+commit;
+-- no relations remain in this case.
+select relname from pg_class where relname like 'temp_parted_oncommit_test%';
+-- Using ON COMMIT DELETE on a partitioned table does not remove
+-- all rows if partitions preserve their data.
+begin;
+create temp table temp_parted_oncommit_test (a int)
+ partition by list (a) on commit delete rows;
+create temp table temp_parted_oncommit_test1
+ partition of temp_parted_oncommit_test
+ for values in (1) on commit preserve rows;
+create temp table temp_parted_oncommit_test2
+ partition of temp_parted_oncommit_test
+ for values in (2) on commit drop;
+insert into temp_parted_oncommit_test values (1), (2);
+commit;
+-- Data from the remaining partition is still here as its rows are
+-- preserved.
+select * from temp_parted_oncommit_test;
+-- two relations remain in this case.
+select relname from pg_class where relname like 'temp_parted_oncommit_test%';
+drop table temp_parted_oncommit_test;
+
+-- Check dependencies between ON COMMIT actions with inheritance trees.
+-- Using ON COMMIT DROP on a parent removes the whole set.
+begin;
+create temp table temp_inh_oncommit_test (a int) on commit drop;
+create temp table temp_inh_oncommit_test1 ()
+ inherits(temp_inh_oncommit_test) on commit delete rows;
+insert into temp_inh_oncommit_test1 values (1);
+commit;
+-- no relations remain in this case
+select relname from pg_class where relname like 'temp_inh_oncommit_test%';
+-- Data on the parent is removed, and the child goes away.
+begin;
+create temp table temp_inh_oncommit_test (a int) on commit delete rows;
+create temp table temp_inh_oncommit_test1 ()
+ inherits(temp_inh_oncommit_test) on commit drop;
+insert into temp_inh_oncommit_test1 values (1);
+insert into temp_inh_oncommit_test values (1);
+commit;
+select * from temp_inh_oncommit_test;
+-- one relation remains
+select relname from pg_class where relname like 'temp_inh_oncommit_test%';
+drop table temp_inh_oncommit_test;
signature.asc
Description: PGP signature
