On 2018/11/02 10:51, Michael Paquier wrote:
> On Thu, Nov 01, 2018 at 01:04:43PM +0900, Michael Paquier wrote:
>> On Thu, Nov 01, 2018 at 12:39:16PM +0900, Amit Langote wrote:
>>> Rajkumar pointed out off-list that the patch still remains to be applied.
>>> Considering that there is a planned point release on Nov 8, maybe we
>>> should do something about this?
>>
>> Yes doing something about that very soon would be a good idea. Tom,
>> are you planning to look at it or should I jump in?
>
> And so I am looking at v3 now...
Thanks for looking.
> Adding a test case in temp.sql would be nice.
Good idea, done.
When writing the test, I noticed something to be pointed out. As of
1c7c317cd9d, partitions of a temporary partition table themselves must be
temporary, but the ON COMMIT action has to be specified for each table
separately. Maybe, there is nothing to be concerned about here, because
there's nowhere to record what's specified for the parent to use it on the
children. So, children's CREATE TABLE commands must specify the ON COMMIT
action for themselves.
> Would it make sense to support TRUNCATE on a materialized as well in the
> future? It seems to me that it is dangerous to assume that only
> relations make use of heap_truncate_one_rel() anyway as modules or
> external code could perfectly call it. And the thing is documented
> to work on a relation, including materialized views, not just an
> ordinary table which is what RELKIND_RELATION only mentions. On the
> contrary we know that heap_truncate() works only on temporary
> relations. It is documented to do so and does so.
>
> So it seems to me that Tom correctly mentioned to add the check in
> heap_truncate, not heap_truncate_one_rel(), so v3 looks incorrect to
> me.
Okay, I agree that adding the preventive check in heap_truncate is a good
idea.
Updated patch attached.
Thanks,
Amit
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;
