Hi,
On 2018/11/02 14:27, Michael Paquier wrote:
> On Fri, Nov 02, 2018 at 02:18:04PM +0900, Michael Paquier wrote:
>> This case is funky.
Interesting indeed.
>> The parent gets dropped at commit time, but it does
>> not know that it should drop the child as well per their dependencies.
>> This actually goes into the internals of performDeletion(), which is
>> scary to touch on back-branches just for such cases..
Well, performDeletion *does* drop the child, because when the parent is
dropped due to its ON COMMIT DROP action, it's done using:
/*
* 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 the DROP_CASCADE, which means its children will be deleted as part of
this.
> A bit more fun with inheritance:
> =# begin;
> BEGIN
> =# create temp table aa_p (a int) on commit drop;
> CREATE TABLE
> =# create temp table aa_c (a int) inherits (aa_p) on commit delete rows;
> NOTICE: 00000: merging column "a" with inherited definition
> LOCATION: MergeAttributes, tablecmds.c:2339
> CREATE TABLE
> =# insert into aa_p values (1);
> INSERT 0 1
> =# insert into aa_c values (1);
> INSERT 0 1
> =# commit;
> NOTICE: 00000: drop cascades to table aa_c
> LOCATION: reportDependentObjects, dependency.c:995
> ERROR: XX000: could not open relation with OID 16426
> LOCATION: relation_open, heapam.c:1138
I think the problem here is that the ON COMMIT DROP action on the parent
is executed before the ON COMMIT DELETE ROWS on the child. The first
action drops the child along with its parent, causing the 2nd one to fail
upon not finding the table to apply heap_truncate() to. It's
heap_truncate() that produces "ERROR: could not open relation with OID
xxx". So, the NOTICE comes from the *successful* dropping of the child
and the ERROR comes from failure of heap_truncate() to find it.
By the way, this error is same as in the partitioned case. It's simply
that the dependency type is different for regular inheritance child tables
than it is for partitions. The former requires a user command to specify
CASCADE when dropping via parent, whereas the latter doesn't. So, you see
the NOTICE in the regular inheritance case, whereas you don't in the
partitioning case. The error itself is same in both cases.
> Let's treat that as a separate issue, as this happens also with an
> unpatched build. The only reason why you cannot trigger it with
> partitions is that ON COMMIT is currently broken for them, so we should
> fix the reported case first. In consequence, I would tend to commit the
> patch proposed and take care of the first, except if of course anybody
> has an objection.
Agreed that they're two independent issues, although it wouldn't be such a
bad idea to fix them in one go, as they're both issues related to the
handling of ON COMMIT actions on tables in inheritance trees.
I've created a patch for the other issue and attached both.
Thanks,
Amit
>From 5d1cb1beda9a4d3c020933e85715cdd2423e5ba7 Mon Sep 17 00:00:00 2001
From: amit <amitlangot...@gmail.com>
Date: Fri, 2 Nov 2018 15:26:37 +0900
Subject: [PATCH v5 2/2] Check relation still exists before applying ON COMMIT
action
---
src/backend/commands/tablecmds.c | 17 +++++++++++++++++
src/test/regress/expected/temp.out | 23 +++++++++++++++++++++++
src/test/regress/sql/temp.sql | 15 +++++++++++++++
3 files changed, 55 insertions(+)
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index ba76c3f9b9..7f742dba7b 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -13291,6 +13291,23 @@ PreCommit_on_commit_actions(void)
}
if (oids_to_truncate != NIL)
{
+ List *tmp = oids_to_truncate;
+
+ /*
+ * Check that relations we're about to truncate still exist.
Some of
+ * them might be inheritance children, which would be gone as a
result
+ * of their parent being dropped due to ONCOMMIT_DROP action of
its
+ * own.
+ */
+ oids_to_truncate = NIL;
+ foreach(l, tmp)
+ {
+ Oid relid = lfirst_oid(l);
+
+ if (SearchSysCacheExists1(RELOID,
ObjectIdGetDatum(relid)))
+ oids_to_truncate =
lappend_oid(oids_to_truncate, relid);
+ }
+
heap_truncate(oids_to_truncate);
CommandCounterIncrement(); /* XXX needed? */
}
diff --git a/src/test/regress/expected/temp.out
b/src/test/regress/expected/temp.out
index ed2c591fc6..2cfc5debf1 100644
--- a/src/test/regress/expected/temp.out
+++ b/src/test/regress/expected/temp.out
@@ -213,3 +213,26 @@ select * from temp_parted_oncommit_test;
(0 rows)
drop table temp_parted_oncommit_test;
+-- another case where a child table is gone by the time it's ON COMMIT
+-- action is executed (whatever it is) due to the ON COMMIT DROP action
+-- on its parent
+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;
+insert into temp_parted_oncommit_test values (1);
+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;
+NOTICE: drop cascades to table temp_inh_oncommit_test1
+-- zero rows in both cases
+select relname from pg_class where relname like 'temp_parted_oncommit_test%';
+ relname
+---------
+(0 rows)
+
+select relname from pg_class where relname like 'temp_inh_oncommit_test%';
+ relname
+---------
+(0 rows)
+
diff --git a/src/test/regress/sql/temp.sql b/src/test/regress/sql/temp.sql
index 0274bdb13f..59b11c4894 100644
--- a/src/test/regress/sql/temp.sql
+++ b/src/test/regress/sql/temp.sql
@@ -162,3 +162,18 @@ commit;
-- temp_parted_oncommit_test1 must've been emptied during the commit
select * from temp_parted_oncommit_test;
drop table temp_parted_oncommit_test;
+
+-- another case where a child table is gone by the time it's ON COMMIT
+-- action is executed (whatever it is) due to the ON COMMIT DROP action
+-- on its parent
+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;
+insert into temp_parted_oncommit_test values (1);
+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;
+-- zero rows in both cases
+select relname from pg_class where relname like 'temp_parted_oncommit_test%';
+select relname from pg_class where relname like 'temp_inh_oncommit_test%';
--
2.11.0
>From 710f3a0830a4cb58ca7d1e7968d613d144c0fdda Mon Sep 17 00:00:00 2001
From: amit <amitlangot...@gmail.com>
Date: Fri, 2 Nov 2018 15:28:57 +0900
Subject: [PATCH v5 1/2] Ignore partitioned tables during ON COMMIT DELETE ROWS
---
src/backend/catalog/heap.c | 11 ++++++++---
src/test/regress/expected/temp.out | 14 ++++++++++++++
src/test/regress/sql/temp.sql | 11 +++++++++++
3 files changed, 33 insertions(+), 3 deletions(-)
diff --git a/src/backend/catalog/heap.c b/src/backend/catalog/heap.c
index 064cf9dbf6..2c2d287f0e 100644
--- a/src/backend/catalog/heap.c
+++ b/src/backend/catalog/heap.c
@@ -3121,7 +3121,8 @@ RelationTruncateIndexes(Relation heapRelation)
/*
* heap_truncate
*
- * This routine deletes all data within all the specified relations.
+ * This routine deletes all data within all the specified relations,
+ * ignoring any that don't have any storage to begin with
*
* This is not transaction-safe! There is another, transaction-safe
* implementation in commands/tablecmds.c. We now use this only for
@@ -3151,8 +3152,12 @@ heap_truncate(List *relids)
{
Relation rel = lfirst(cell);
- /* Truncate the relation */
- heap_truncate_one_rel(rel);
+ /*
+ * Truncate the relation. Regular and partitioned tables can be
+ * temporary relations, but only regular tables have storage.
+ */
+ if (rel->rd_rel->relkind == RELKIND_RELATION)
+ heap_truncate_one_rel(rel);
/* Close the relation, but keep exclusive lock on it until
commit */
heap_close(rel, NoLock);
diff --git a/src/test/regress/expected/temp.out
b/src/test/regress/expected/temp.out
index addf1ec444..ed2c591fc6 100644
--- a/src/test/regress/expected/temp.out
+++ b/src/test/regress/expected/temp.out
@@ -199,3 +199,17 @@ select pg_temp.whoami();
(1 row)
drop table public.whereami;
+-- for partitioned temp tables, ON COMMIT action should ignore storage-less
+-- partitioned parent table
+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 delete rows;
+insert into temp_parted_oncommit_test values (1);
+commit;
+-- temp_parted_oncommit_test1 must've been emptied during the commit
+select * from temp_parted_oncommit_test;
+ a
+---
+(0 rows)
+
+drop table temp_parted_oncommit_test;
diff --git a/src/test/regress/sql/temp.sql b/src/test/regress/sql/temp.sql
index 5183c727f5..0274bdb13f 100644
--- a/src/test/regress/sql/temp.sql
+++ b/src/test/regress/sql/temp.sql
@@ -151,3 +151,14 @@ select whoami();
select pg_temp.whoami();
drop table public.whereami;
+
+-- for partitioned temp tables, ON COMMIT action should ignore storage-less
+-- partitioned parent table
+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 delete rows;
+insert into temp_parted_oncommit_test values (1);
+commit;
+-- temp_parted_oncommit_test1 must've been emptied during the commit
+select * from temp_parted_oncommit_test;
+drop table temp_parted_oncommit_test;
--
2.11.0
