Re: ON COMMIT actions and inheritance - Mailing list pgsql-hackers

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