Re: ON COMMIT actions and inheritance

Hi,

On 2018/11/06 12:03, Michael Paquier wrote:
> 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

Agreed that Alvaro's idea is better.

> 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.

Thanks for rewriting the patch.

> 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.

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)?

> 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).

Seems fine to me.

I only have cosmetic comments on the patch.

+
+ /*
+ * Truncate relations before dropping so as all dependencies between

so as -> so that

+ * 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.

Thanks,
Amit