Обсуждение: ON COMMIT actions and inheritance

Hi,

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

The problem is that when PreCommit_on_commit_actions() executes an ON
COMMIT DROP action on the parent, it will drop its children as well.  It
doesn't however remove the children's own actions, especially ON COMMIT
DELETE ROWS, from the list and when it gets around to executing them, the
children are already gone.  That causes the heap_open in heap_truncate to
fail with an error like this:

ERROR:  XX000: could not open relation with OID 16420
LOCATION:  relation_open, heapam.c:1138

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.

Thanks,
Amit

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

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

While you're there -- I think the CCI after the heap_truncate is not
needed.  Could as well get rid of it ...

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On Tue, Nov 06, 2018 at 09:53:37AM -0300, Alvaro Herrera wrote:
> While you're there -- I think the CCI after the heap_truncate is not
> needed.  Could as well get rid of it ...

Yes, I have noticed the comment saying so.  I did not really want to
bother about that part yet for this patch as that concerns only HEAD
though.
--
Michael

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

Hi,

Thank you updating the patch and adding notes to the documentation about
the points I raised.

On 2018/11/07 9:53, Michael Paquier wrote:
> 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.

I looked at the documentation patch regarding this:

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

How about:

When used on tables with inheritance children (including partitioned
tables), this also drops the children (partitions).

Thanks,
Amit

On Thu, Nov 08, 2018 at 04:46:46PM +0900, Amit Langote wrote:
> How about:
> When used on tables with inheritance children (including partitioned
> tables), this also drops the children (partitions).

Even if the style gets heavier, I have also the following in my box:
When used on a partitioned table, this action drops its partitions and
when used on tables with inheritance children, it drops the depending
children.
--
Michael

On 2018/11/08 18:03, Michael Paquier wrote:
> On Thu, Nov 08, 2018 at 04:46:46PM +0900, Amit Langote wrote:
>> How about:
>> When used on tables with inheritance children (including partitioned
>> tables), this also drops the children (partitions).
> 
> Even if the style gets heavier, I have also the following in my box:
> When used on a partitioned table, this action drops its partitions and
> when used on tables with inheritance children, it drops the depending
> children.

I think we don't need to say "depending" children here, even though I know
the dependency management system is involved internally.

In the end, I will let you go with whatever you think is clearer. :)

Thanks,
Amit

On Thu, Nov 8, 2018 at 4:04 AM Michael Paquier <michael@paquier.xyz> wrote:
> Even if the style gets heavier, I have also the following in my box:
> When used on a partitioned table, this action drops its partitions and
> when used on tables with inheritance children, it drops the depending
> children.

It should be "dependent" not "depending".

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

On Thu, Nov 08, 2018 at 12:53:18PM -0500, Robert Haas wrote:
> On Thu, Nov 8, 2018 at 4:04 AM Michael Paquier <michael@paquier.xyz> wrote:
>> Even if the style gets heavier, I have also the following in my box:
>> When used on a partitioned table, this action drops its partitions and
>> when used on tables with inheritance children, it drops the depending
>> children.
>
> It should be "dependent" not "depending".

Thanks for the feedback!  I have committed and back-patched but actually
I am only back-patching that stuff down to v10 as I have noticed the
following limitation on 9.6 and older versions when cascading ON COMMIT
DROP using this test case:
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;

At commit time, there is an annoying NOTICE message which should not
show up because of the automatic cascading drop:
NOTICE:  drop cascades to table temp_inh_oncommit_test1

The issue is that PERFORM_DELETION_QUIETLY is not available back then,
and this requires a bit of refactoring around reportDependentObjects(),
particularly changing silently one of its arguments which seems risky to
introduce on a stable branch, so I am taking the safest path (this is a
static routine but I am ready to bet that some forks would be unhappy
with the suddent change and that may not trigger a compilation failure).
This bug is also quite old, and there have not been complains until I
discovered it, so it does not seem worth taking any risk.
--
Michael