On Mon, Nov 05, 2018 at 02:37:05PM +0900, Amit Langote wrote: > Michael pointed out a problem with specifying different ON COMMIT actions > on a temporary inheritance parent and its children: > > https://www.postgresql.org/message-id/20181102051804.GV1727%40paquier.xyz
Thanks for starting a new thread on the matter. > One way to fix that is to remove the tables that no longer exist from > the list that's passed to heap_truncate(), which the attached patch > implements. I don't find that much elegant as you move the responsibility to do the relation existence checks directly into the ON COMMIT actions, and all this logic exists already when doing object drops as part of dependency.c. Alvaro has suggested using performMultipleDeletions() instead, which is a very good idea in my opinion: https://www.postgresql.org/message-id/20181105193725.4eluxe3xsewr65iu@alvherre.pgsql So I have dug into the issue and I am finishing with the attached, which implements the solution suggested by Alvaro. The approach used is rather close to what is done for on-commit truncation, so as all the to-be-dropped relation OIDs are collected at once, then processed at the same time. One thing is that the truncation needs to happen before dropping the relations as it could be possible that a truncation is attempted on something which has been already dropped because of a previous dependency. This can feel like a waste as it is possible that a relation truncated needs to be dropped afterwards if its parent is dropped, but I think that this keeps the code simple and more understandable. Another interesting behavior is for example the following scenario with partitions: +-- 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) What happens here is that the parent needs to truncate its data, but the child wants to preserve them. This can be made to work but we would need to call again find_all_inheritors() to grab the list of partitions when working on a partition to match what a manual TRUNCATE does when run on a partitioned table. However, there is a point that the partition explicitly wants to *preserve* its rows, which feels also natural to me. This also keeps the code more simple, and users willing to remove roes on it could just specify "on commit delete rows" to remove them. So I would tend to keep the code simple, which makes the behavior of on commit actions less surprising depending on the table kind worked on. This stuff is too late for this week's release, but we could get something into the next one to fix all that. From what I can see, this is handled incorrectly for inheritance trees down to 9.4 (I have not tested 9.3 as it is basically EOL'd this week and I am not planning to do so). Thoughts? -- Michael
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 6048334c75..4c46e66f78 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,65 @@ 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 as all dependencies between
+ * 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.
+ */
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
