Prepare Transaction support for ON COMMIT DROP temporary tables
Hi,
Please find attached a patch to enable support for temporary tables in
prepared transactions when ON COMMIT DROP has been specified.
The comment in the existing code around this idea reads:
* Don't allow PREPARE TRANSACTION if we've accessed a temporary table in
* this transaction.
[ ... ]
* XXX In principle this could be relaxed to allow some useful special
* cases, such as a temp table created and dropped all within the
* transaction. That seems to require much more bookkeeping though.
In the attached patch I have added this paragraph, and of course the
implementation of it:
* A special case of this situation is using ON COMMIT DROP, where the
* call to PreCommit_on_commit_actions() is then responsible for
* performing the DROP table within the transaction and before we get
* here.
Regards,
--
dim
Attachments:
oncommitdrop-px.patchtext/x-patchDownload
commit 7dd834a2fb57ba617d70abf7a23eb5cc84dadca5
Author: Dimitri Fontaine <dim@tapoueh.org>
Date: Thu Dec 27 12:22:56 2018 +0100
Add support for on-commit-drop temp tables to prepared transactions.
As the bookkeeping is all done within PreCommit_on_commit_actions() we can
prepare a transaction that have been accessing temporary tables when all of
them as marked ON COMMIT DROP.
diff --git a/doc/src/sgml/ref/prepare_transaction.sgml b/doc/src/sgml/ref/prepare_transaction.sgml
index d958f7a06f..c1ef0707cf 100644
--- a/doc/src/sgml/ref/prepare_transaction.sgml
+++ b/doc/src/sgml/ref/prepare_transaction.sgml
@@ -97,8 +97,9 @@ PREPARE TRANSACTION <replaceable class="parameter">transaction_id</replaceable>
</para>
<para>
- It is not currently allowed to <command>PREPARE</command> a transaction that
- has executed any operations involving temporary tables,
+ It is not currently allowed to <command>PREPARE</command> a transaction
+ that has executed any operations involving temporary tables (except when
+ all involved temporary tables are <literal>ON COMMIT DROP</literal>),
created any cursors <literal>WITH HOLD</literal>, or executed
<command>LISTEN</command>, <command>UNLISTEN</command>, or
<command>NOTIFY</command>.
diff --git a/src/backend/access/transam/xact.c b/src/backend/access/transam/xact.c
index d967400384..1d7f3017ad 100644
--- a/src/backend/access/transam/xact.c
+++ b/src/backend/access/transam/xact.c
@@ -2272,11 +2272,19 @@ PrepareTransaction(void)
* XXX In principle this could be relaxed to allow some useful special
* cases, such as a temp table created and dropped all within the
* transaction. That seems to require much more bookkeeping though.
+ *
+ * A special case of this situation is using ON COMMIT DROP, where the
+ * call to PreCommit_on_commit_actions() is then responsible for
+ * performing the DROP table within the transaction and before we get
+ * here.
*/
- if ((MyXactFlags & XACT_FLAGS_ACCESSEDTEMPREL))
+ if ((MyXactFlags & XACT_FLAGS_ACCESSEDTEMPREL)
+ && !every_on_commit_is_on_commit_drop())
+ {
ereport(ERROR,
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
- errmsg("cannot PREPARE a transaction that has operated on temporary tables")));
+ errmsg("cannot PREPARE a transaction that has operated on temporary tables that are not ON COMMIT DROP")));
+ }
/*
* Likewise, don't allow PREPARE after pg_export_snapshot. This could be
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index c8c50e8c98..2060f3e68e 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -13139,6 +13139,26 @@ remove_on_commit_action(Oid relid)
}
}
+/*
+ * Return true when every temp relation known in the current session is marked
+ * ON COMMIT DROP. This allows to then PREPARE TRANSACTION, for instance,
+ * because we know that at prepare time the temp table is dropped already.
+ */
+bool
+every_on_commit_is_on_commit_drop()
+{
+ ListCell *l;
+
+ foreach(l, on_commits)
+ {
+ OnCommitItem *oc = (OnCommitItem *) lfirst(l);
+
+ if (oc->oncommit != ONCOMMIT_DROP)
+ return false;
+ }
+ return true;
+}
+
/*
* Perform ON COMMIT actions.
*
diff --git a/src/include/commands/tablecmds.h b/src/include/commands/tablecmds.h
index 2afcd5be44..2f5731a9f6 100644
--- a/src/include/commands/tablecmds.h
+++ b/src/include/commands/tablecmds.h
@@ -82,6 +82,7 @@ extern void createForeignKeyTriggers(Relation rel, Oid refRelOid,
extern void register_on_commit_action(Oid relid, OnCommitAction action);
extern void remove_on_commit_action(Oid relid);
+extern bool every_on_commit_is_on_commit_drop(void);
extern void PreCommit_on_commit_actions(void);
extern void AtEOXact_on_commit_actions(bool isCommit);
diff --git a/src/test/regress/expected/prepared_xacts.out b/src/test/regress/expected/prepared_xacts.out
index eb77c18788..a50ca764c5 100644
--- a/src/test/regress/expected/prepared_xacts.out
+++ b/src/test/regress/expected/prepared_xacts.out
@@ -252,3 +252,95 @@ DROP TABLE pxtest2;
DROP TABLE pxtest3; -- will still be there if prepared xacts are disabled
ERROR: table "pxtest3" does not exist
DROP TABLE pxtest4;
+-- we should be able to prepare a transaction with on commit drop temporary
+-- tables
+begin;
+create temp table pxtesttemp(id serial, f1 text) on commit drop;
+insert into pxtesttemp(f1) values('regress'), ('temp') returning *;
+ id | f1
+----+---------
+ 1 | regress
+ 2 | temp
+(2 rows)
+
+prepare transaction 'regress temp';
+rollback prepared 'regress temp';
+-- There should be no prepared transactions
+SELECT gid FROM pg_prepared_xacts;
+ gid
+-----
+(0 rows)
+
+-- other kinds of temporary tables are not supported though.
+begin;
+create temp table pxtesttemp(id serial, f1 text) on commit delete rows;
+insert into pxtesttemp(f1) values('regress'), ('temp') returning *;
+ id | f1
+----+---------
+ 1 | regress
+ 2 | temp
+(2 rows)
+
+prepare transaction 'regress temp';
+ERROR: cannot PREPARE a transaction that has operated on temporary tables
+-- There should be no prepared transactions
+SELECT gid FROM pg_prepared_xacts;
+ gid
+-----
+(0 rows)
+
+begin;
+create temp table pxtesttemp1(id serial, f1 text) on commit drop;
+create temp table pxtesttemp2(id serial, f1 text) on commit delete rows;
+insert into pxtesttemp1(f1) values('regress'), ('temp') returning *;
+ id | f1
+----+---------
+ 1 | regress
+ 2 | temp
+(2 rows)
+
+prepare transaction 'regress temp';
+ERROR: cannot PREPARE a transaction that has operated on temporary tables
+-- There should be no prepared transactions
+SELECT gid FROM pg_prepared_xacts;
+ gid
+-----
+(0 rows)
+
+-- Test with two temporary tables on commit drop now
+begin;
+create temp table pxtesttemp1(id serial, f1 text) on commit drop;
+create temp table pxtesttemp2(id serial, f1 text) on commit drop;
+insert into pxtesttemp1(f1) values('regress'), ('temp');
+insert into pxtesttemp2(f1) values('regress'), ('temp');
+prepare transaction 'regress temp';
+rollback prepared 'regress temp';
+-- There should be no prepared transactions
+SELECT gid FROM pg_prepared_xacts;
+ gid
+-----
+(0 rows)
+
+-- Now test with a temp table that exists in the session, and a temp table
+-- that exists only within the prepared transaction.
+create temp table pxtesttemp1(id serial, f1 text);
+insert into pxtesttemp1(f1) values('regress'), ('temp') returning *;
+ id | f1
+----+---------
+ 1 | regress
+ 2 | temp
+(2 rows)
+
+begin;
+create temp table pxtesttemp2(id serial, f1 text) on commit drop;
+insert into pxtesttemp2(f1) values('regress'), ('temp');
+prepare transaction 'regress temp';
+rollback prepared 'regress temp';
+-- cleanup
+drop table pxtesttemp1;
+-- There should be no prepared transactions
+SELECT gid FROM pg_prepared_xacts;
+ gid
+-----
+(0 rows)
+
diff --git a/src/test/regress/sql/prepared_xacts.sql b/src/test/regress/sql/prepared_xacts.sql
index d8249a27dc..31ab2c9513 100644
--- a/src/test/regress/sql/prepared_xacts.sql
+++ b/src/test/regress/sql/prepared_xacts.sql
@@ -156,3 +156,61 @@ SELECT gid FROM pg_prepared_xacts;
DROP TABLE pxtest2;
DROP TABLE pxtest3; -- will still be there if prepared xacts are disabled
DROP TABLE pxtest4;
+
+-- we should be able to prepare a transaction with on commit drop temporary
+-- tables
+begin;
+create temp table pxtesttemp(id serial, f1 text) on commit drop;
+insert into pxtesttemp(f1) values('regress'), ('temp') returning *;
+prepare transaction 'regress temp';
+rollback prepared 'regress temp';
+
+-- There should be no prepared transactions
+SELECT gid FROM pg_prepared_xacts;
+
+-- other kinds of temporary tables are not supported though.
+begin;
+create temp table pxtesttemp(id serial, f1 text) on commit delete rows;
+insert into pxtesttemp(f1) values('regress'), ('temp') returning *;
+prepare transaction 'regress temp';
+
+-- There should be no prepared transactions
+SELECT gid FROM pg_prepared_xacts;
+
+begin;
+create temp table pxtesttemp1(id serial, f1 text) on commit drop;
+create temp table pxtesttemp2(id serial, f1 text) on commit delete rows;
+insert into pxtesttemp1(f1) values('regress'), ('temp') returning *;
+prepare transaction 'regress temp';
+
+-- There should be no prepared transactions
+SELECT gid FROM pg_prepared_xacts;
+
+-- Test with two temporary tables on commit drop now
+begin;
+create temp table pxtesttemp1(id serial, f1 text) on commit drop;
+create temp table pxtesttemp2(id serial, f1 text) on commit drop;
+insert into pxtesttemp1(f1) values('regress'), ('temp');
+insert into pxtesttemp2(f1) values('regress'), ('temp');
+prepare transaction 'regress temp';
+rollback prepared 'regress temp';
+
+-- There should be no prepared transactions
+SELECT gid FROM pg_prepared_xacts;
+
+-- Now test with a temp table that exists in the session, and a temp table
+-- that exists only within the prepared transaction.
+create temp table pxtesttemp1(id serial, f1 text);
+insert into pxtesttemp1(f1) values('regress'), ('temp') returning *;
+
+begin;
+create temp table pxtesttemp2(id serial, f1 text) on commit drop;
+insert into pxtesttemp2(f1) values('regress'), ('temp');
+prepare transaction 'regress temp';
+rollback prepared 'regress temp';
+
+-- cleanup
+drop table pxtesttemp1;
+
+-- There should be no prepared transactions
+SELECT gid FROM pg_prepared_xacts;
On 28/12/2018 12:46, Dimitri Fontaine wrote:
Hi,
Please find attached a patch to enable support for temporary tables in
prepared transactions when ON COMMIT DROP has been specified.
The comments I made on IRC have been addressed in this version of the
patch, so it looks good to me.
--
Vik Fearing +33 6 46 75 15 36
http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support
Hi Dimitri
On 2018-Dec-28, Dimitri Fontaine wrote:
Please find attached a patch to enable support for temporary tables in
prepared transactions when ON COMMIT DROP has been specified.
Glad to see you submitting patches again.
I suggest to add in your regression tests a case where the prepared
transaction commits, and ensuring that the temp table is gone from
catalogs.
--
�lvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Alvaro Herrera <alvherre@2ndquadrant.com> writes:
Glad to see you submitting patches again.
Thanks!
I suggest to add in your regression tests a case where the prepared
transaction commits, and ensuring that the temp table is gone from
catalogs.
Please find such a revision attached.
Regards,
--
dim
Attachments:
oncommitdrop-px-2.patchtext/x-patchDownload
commit 0b2b2d4b2ec4cd6cd2d26ef229a3764ab9ad2e78
Author: Dimitri Fontaine <dim@tapoueh.org>
Date: Thu Dec 27 12:22:56 2018 +0100
Add support for on-commit-drop temp tables to prepared transactions.
As the bookkeeping is all done within PreCommit_on_commit_actions() we can
prepare a transaction that have been accessing temporary tables when all of
them as marked ON COMMIT DROP.
diff --git a/doc/src/sgml/ref/prepare_transaction.sgml b/doc/src/sgml/ref/prepare_transaction.sgml
index d958f7a06f..c1ef0707cf 100644
--- a/doc/src/sgml/ref/prepare_transaction.sgml
+++ b/doc/src/sgml/ref/prepare_transaction.sgml
@@ -97,8 +97,9 @@ PREPARE TRANSACTION <replaceable class="parameter">transaction_id</replaceable>
</para>
<para>
- It is not currently allowed to <command>PREPARE</command> a transaction that
- has executed any operations involving temporary tables,
+ It is not currently allowed to <command>PREPARE</command> a transaction
+ that has executed any operations involving temporary tables (except when
+ all involved temporary tables are <literal>ON COMMIT DROP</literal>),
created any cursors <literal>WITH HOLD</literal>, or executed
<command>LISTEN</command>, <command>UNLISTEN</command>, or
<command>NOTIFY</command>.
diff --git a/src/backend/access/transam/xact.c b/src/backend/access/transam/xact.c
index d967400384..1d7f3017ad 100644
--- a/src/backend/access/transam/xact.c
+++ b/src/backend/access/transam/xact.c
@@ -2272,11 +2272,19 @@ PrepareTransaction(void)
* XXX In principle this could be relaxed to allow some useful special
* cases, such as a temp table created and dropped all within the
* transaction. That seems to require much more bookkeeping though.
+ *
+ * A special case of this situation is using ON COMMIT DROP, where the
+ * call to PreCommit_on_commit_actions() is then responsible for
+ * performing the DROP table within the transaction and before we get
+ * here.
*/
- if ((MyXactFlags & XACT_FLAGS_ACCESSEDTEMPREL))
+ if ((MyXactFlags & XACT_FLAGS_ACCESSEDTEMPREL)
+ && !every_on_commit_is_on_commit_drop())
+ {
ereport(ERROR,
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
- errmsg("cannot PREPARE a transaction that has operated on temporary tables")));
+ errmsg("cannot PREPARE a transaction that has operated on temporary tables that are not ON COMMIT DROP")));
+ }
/*
* Likewise, don't allow PREPARE after pg_export_snapshot. This could be
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index c8c50e8c98..2060f3e68e 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -13139,6 +13139,26 @@ remove_on_commit_action(Oid relid)
}
}
+/*
+ * Return true when every temp relation known in the current session is marked
+ * ON COMMIT DROP. This allows to then PREPARE TRANSACTION, for instance,
+ * because we know that at prepare time the temp table is dropped already.
+ */
+bool
+every_on_commit_is_on_commit_drop()
+{
+ ListCell *l;
+
+ foreach(l, on_commits)
+ {
+ OnCommitItem *oc = (OnCommitItem *) lfirst(l);
+
+ if (oc->oncommit != ONCOMMIT_DROP)
+ return false;
+ }
+ return true;
+}
+
/*
* Perform ON COMMIT actions.
*
diff --git a/src/include/commands/tablecmds.h b/src/include/commands/tablecmds.h
index 2afcd5be44..2f5731a9f6 100644
--- a/src/include/commands/tablecmds.h
+++ b/src/include/commands/tablecmds.h
@@ -82,6 +82,7 @@ extern void createForeignKeyTriggers(Relation rel, Oid refRelOid,
extern void register_on_commit_action(Oid relid, OnCommitAction action);
extern void remove_on_commit_action(Oid relid);
+extern bool every_on_commit_is_on_commit_drop(void);
extern void PreCommit_on_commit_actions(void);
extern void AtEOXact_on_commit_actions(bool isCommit);
diff --git a/src/test/regress/expected/prepared_xacts.out b/src/test/regress/expected/prepared_xacts.out
index eb77c18788..d2e81461d0 100644
--- a/src/test/regress/expected/prepared_xacts.out
+++ b/src/test/regress/expected/prepared_xacts.out
@@ -252,3 +252,101 @@ DROP TABLE pxtest2;
DROP TABLE pxtest3; -- will still be there if prepared xacts are disabled
ERROR: table "pxtest3" does not exist
DROP TABLE pxtest4;
+-- we should be able to prepare a transaction with on commit drop temporary
+-- tables
+begin;
+create temp table pxtesttemp(id serial, f1 text) on commit drop;
+insert into pxtesttemp(f1) values('regress'), ('temp') returning *;
+ id | f1
+----+---------
+ 1 | regress
+ 2 | temp
+(2 rows)
+
+prepare transaction 'regress temp';
+commit prepared 'regress temp';
+-- There should be no prepared transactions
+SELECT gid FROM pg_prepared_xacts;
+ gid
+-----
+(0 rows)
+
+-- The temporary table should be gone now
+SELECT relname FROM pg_class WHERE relname = 'pxtesttemp';
+ relname
+---------
+(0 rows)
+
+-- other kinds of temporary tables are not supported though.
+begin;
+create temp table pxtesttemp(id serial, f1 text) on commit delete rows;
+insert into pxtesttemp(f1) values('regress'), ('temp') returning *;
+ id | f1
+----+---------
+ 1 | regress
+ 2 | temp
+(2 rows)
+
+prepare transaction 'regress temp';
+ERROR: cannot PREPARE a transaction that has operated on temporary tables that are not ON COMMIT DROP
+-- There should be no prepared transactions
+SELECT gid FROM pg_prepared_xacts;
+ gid
+-----
+(0 rows)
+
+begin;
+create temp table pxtesttemp1(id serial, f1 text) on commit drop;
+create temp table pxtesttemp2(id serial, f1 text) on commit delete rows;
+insert into pxtesttemp1(f1) values('regress'), ('temp') returning *;
+ id | f1
+----+---------
+ 1 | regress
+ 2 | temp
+(2 rows)
+
+prepare transaction 'regress temp';
+ERROR: cannot PREPARE a transaction that has operated on temporary tables that are not ON COMMIT DROP
+-- There should be no prepared transactions
+SELECT gid FROM pg_prepared_xacts;
+ gid
+-----
+(0 rows)
+
+-- Test with two temporary tables on commit drop now
+begin;
+create temp table pxtesttemp1(id serial, f1 text) on commit drop;
+create temp table pxtesttemp2(id serial, f1 text) on commit drop;
+insert into pxtesttemp1(f1) values('regress'), ('temp');
+insert into pxtesttemp2(f1) values('regress'), ('temp');
+prepare transaction 'regress temp';
+rollback prepared 'regress temp';
+-- There should be no prepared transactions
+SELECT gid FROM pg_prepared_xacts;
+ gid
+-----
+(0 rows)
+
+-- Now test with a temp table that exists in the session, and a temp table
+-- that exists only within the prepared transaction.
+create temp table pxtesttemp1(id serial, f1 text);
+insert into pxtesttemp1(f1) values('regress'), ('temp') returning *;
+ id | f1
+----+---------
+ 1 | regress
+ 2 | temp
+(2 rows)
+
+begin;
+create temp table pxtesttemp2(id serial, f1 text) on commit drop;
+insert into pxtesttemp2(f1) values('regress'), ('temp');
+prepare transaction 'regress temp';
+rollback prepared 'regress temp';
+-- cleanup
+drop table pxtesttemp1;
+-- There should be no prepared transactions
+SELECT gid FROM pg_prepared_xacts;
+ gid
+-----
+(0 rows)
+
diff --git a/src/test/regress/sql/prepared_xacts.sql b/src/test/regress/sql/prepared_xacts.sql
index d8249a27dc..dd8655facf 100644
--- a/src/test/regress/sql/prepared_xacts.sql
+++ b/src/test/regress/sql/prepared_xacts.sql
@@ -156,3 +156,64 @@ SELECT gid FROM pg_prepared_xacts;
DROP TABLE pxtest2;
DROP TABLE pxtest3; -- will still be there if prepared xacts are disabled
DROP TABLE pxtest4;
+
+-- we should be able to prepare a transaction with on commit drop temporary
+-- tables
+begin;
+create temp table pxtesttemp(id serial, f1 text) on commit drop;
+insert into pxtesttemp(f1) values('regress'), ('temp') returning *;
+prepare transaction 'regress temp';
+commit prepared 'regress temp';
+
+-- There should be no prepared transactions
+SELECT gid FROM pg_prepared_xacts;
+
+-- The temporary table should be gone now
+SELECT relname FROM pg_class WHERE relname = 'pxtesttemp';
+
+-- other kinds of temporary tables are not supported though.
+begin;
+create temp table pxtesttemp(id serial, f1 text) on commit delete rows;
+insert into pxtesttemp(f1) values('regress'), ('temp') returning *;
+prepare transaction 'regress temp';
+
+-- There should be no prepared transactions
+SELECT gid FROM pg_prepared_xacts;
+
+begin;
+create temp table pxtesttemp1(id serial, f1 text) on commit drop;
+create temp table pxtesttemp2(id serial, f1 text) on commit delete rows;
+insert into pxtesttemp1(f1) values('regress'), ('temp') returning *;
+prepare transaction 'regress temp';
+
+-- There should be no prepared transactions
+SELECT gid FROM pg_prepared_xacts;
+
+-- Test with two temporary tables on commit drop now
+begin;
+create temp table pxtesttemp1(id serial, f1 text) on commit drop;
+create temp table pxtesttemp2(id serial, f1 text) on commit drop;
+insert into pxtesttemp1(f1) values('regress'), ('temp');
+insert into pxtesttemp2(f1) values('regress'), ('temp');
+prepare transaction 'regress temp';
+rollback prepared 'regress temp';
+
+-- There should be no prepared transactions
+SELECT gid FROM pg_prepared_xacts;
+
+-- Now test with a temp table that exists in the session, and a temp table
+-- that exists only within the prepared transaction.
+create temp table pxtesttemp1(id serial, f1 text);
+insert into pxtesttemp1(f1) values('regress'), ('temp') returning *;
+
+begin;
+create temp table pxtesttemp2(id serial, f1 text) on commit drop;
+insert into pxtesttemp2(f1) values('regress'), ('temp');
+prepare transaction 'regress temp';
+rollback prepared 'regress temp';
+
+-- cleanup
+drop table pxtesttemp1;
+
+-- There should be no prepared transactions
+SELECT gid FROM pg_prepared_xacts;
On Fri, Dec 28, 2018 at 08:32:15PM +0100, Dimitri Fontaine wrote:
Alvaro Herrera <alvherre@2ndquadrant.com> writes:
I suggest to add in your regression tests a case where the prepared
transaction commits, and ensuring that the temp table is gone from
catalogs.Please find such a revision attached.
Being able to relax a bit the case is better than nothing, so that's
nice to see incremental improvements. Thanks Dimitri.
I just had a very quick glance, so that's far from being a detailed
review, but could it be possible to add test cases involving
inheritance trees and/or partitions if that makes sense? The ON
COMMIT action handling is designed to make such cases work properly.
--
Michael
Michael Paquier <michael@paquier.xyz> writes:
Being able to relax a bit the case is better than nothing, so that's
nice to see incremental improvements. Thanks Dimitri.
I'm afraid that this patch is likely to be, if not completely broken,
at least very much less useful than one could wish by the time we get
done closing the holes discussed in this other thread:
/messages/by-id/5d910e2e-0db8-ec06-dd5f-baec420513c3@imap.cc
For instance, if we're going to have to reject the case where the
session's temporary schema was created during the current transaction,
then that puts a very weird constraint on whether this case works.
Also, even without worrying about new problems that that discussion
may lead to, I don't think that the patch works as-is. The function
every_on_commit_is_on_commit_drop() does what it says, but that is
NOT sufficient to conclude that every temp table the transaction has
touched is on-commit-drop. This logic will successfully reject cases
with on-commit-delete-rows temp tables, but not cases where the temp
table(s) lack any ON COMMIT spec at all.
regards, tom lane
Hi,
Tom Lane <tgl@sss.pgh.pa.us> writes:
I'm afraid that this patch is likely to be, if not completely broken,
at least very much less useful than one could wish by the time we get
done closing the holes discussed in this other thread:/messages/by-id/5d910e2e-0db8-ec06-dd5f-baec420513c3@imap.cc
Thanks for the review here Tom, and for linking with the other
discussion (Álvaro did that too, thanks!). I've been reviewing it too.
I didn't think about the pg_temp_NN namespaces in my approach, and I
think it might be possible to make it work, but it's getting quite
involved now.
One idea would be that if every temp table in the session belongs to the
transaction, and their namespace too (we could check through pg_depend
that the namespace doesn't contain anything else beside the
transaction's tables), then we could dispose of the temp schema and
on-commit-drop tables at PREPARE COMMIT time.
Otherwise, as before, prevent the transaction to be a 2PC one.
For instance, if we're going to have to reject the case where the
session's temporary schema was created during the current transaction,
then that puts a very weird constraint on whether this case works.
Yeah. The goal of my approach is to transparently get back temp table
support in 2PC when that makes sense, which is most use cases I've been
confronted to. We use 2PC in Citus, and it would be nice to be able to
use transaction local temp tables on worker nodes when doing data
injestion and roll-ups.
Also, even without worrying about new problems that that discussion
may lead to, I don't think that the patch works as-is. The function
every_on_commit_is_on_commit_drop() does what it says, but that is
NOT sufficient to conclude that every temp table the transaction has
touched is on-commit-drop. This logic will successfully reject cases
with on-commit-delete-rows temp tables, but not cases where the temp
table(s) lack any ON COMMIT spec at all.
Thanks! I missed that the lack of ON COMMIT spec would have that impact
in the code. We could add tracking of that I suppose, and will have a
look at how to implement it provided that the other points find an
acceptable solution.
Regards,
--
dim
On Mon, Jan 14, 2019 at 07:41:18PM +0100, Dimitri Fontaine wrote:
Thanks for the review here Tom, and for linking with the other
discussion (Álvaro did that too, thanks!). I've been reviewing it
too.
If you can look at the patch, reviews are welcome. There are quite a
couple of patterns I spotted on the way.
I didn't think about the pg_temp_NN namespaces in my approach, and I
think it might be possible to make it work, but it's getting quite
involved now.One idea would be that if every temp table in the session belongs to the
transaction, and their namespace too (we could check through pg_depend
that the namespace doesn't contain anything else beside the
transaction's tables), then we could dispose of the temp schema and
on-commit-drop tables at PREPARE COMMIT time.
Hm. A strong assumption that we rely on in the code is that the
temporary namespace drop only happens when the session ends, so you
would need to complicate the logic so as the namespace is created in a
given transaction, which is something that can be done (at least
that's what my patch on the other thread adds control for), and that
no other objects than ON COMMIT tables are created, which is more
tricky to track (still things would get weird with a LOCK on ON COMMIT
DROP tables?). The root of the problem is that the objects' previous
versions would still be around between the PREPARE TRANSACTION and
COMMIT PREPARED, and that both queries can be perfectly run
transparently across multiple sessions.
Back in the time, one thing that we did in Postgres-XC was to enforce
2PC to not be used and use a direct commit instead of failing, which
was utterly wrong. Postgres-XL may be reusing some of that :(
Yeah. The goal of my approach is to transparently get back temp table
support in 2PC when that makes sense, which is most use cases I've been
confronted to. We use 2PC in Citus, and it would be nice to be able to
use transaction local temp tables on worker nodes when doing data
injestion and roll-ups.
You have not considered the case of inherited tables and partitioned
mixing ON COMMIT actions of different types as well. For inherited
tables this does not matter much I think, perhaps for partitions it
does (see tests in 52ea6a8, which you would need to mix with 2PC).
--
Michael
On Mon, Jan 14, 2019 at 1:41 PM Dimitri Fontaine <dimitri@citusdata.com> wrote:
One idea would be that if every temp table in the session belongs to the
transaction, and their namespace too (we could check through pg_depend
that the namespace doesn't contain anything else beside the
transaction's tables), then we could dispose of the temp schema and
on-commit-drop tables at PREPARE COMMIT time.
Why not just drop any on-commit-drop tables at PREPARE TRANSACTION
time and leave the schema alone? If there are any temp tables touched
by the transaction which are not on-commit-drop then we'd have to
fail, but as long as all the tables we've got are on-commit-drop then
it seems fine to just nuke them at PREPARE time. Such tables must've
been created in the current transaction, because otherwise the
creating transaction aborted and they're gone for that reason, or it
committed and they're gone because they're on-commit-drop. And
regardless of whether the transaction we are preparing goes on to
commit or abort, those tables will be gone afterwards for the same
reasons. So there doesn't in this case seem to be any reason to keep
them around until the transaction's fate is known.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
On 16/01/2019 17:44, Robert Haas wrote:
On Mon, Jan 14, 2019 at 1:41 PM Dimitri Fontaine <dimitri@citusdata.com> wrote:
One idea would be that if every temp table in the session belongs to the
transaction, and their namespace too (we could check through pg_depend
that the namespace doesn't contain anything else beside the
transaction's tables), then we could dispose of the temp schema and
on-commit-drop tables at PREPARE COMMIT time.Why not just drop any on-commit-drop tables at PREPARE TRANSACTION
time and leave the schema alone? If there are any temp tables touched
by the transaction which are not on-commit-drop then we'd have to
fail, but as long as all the tables we've got are on-commit-drop then
it seems fine to just nuke them at PREPARE time. Such tables must've
been created in the current transaction, because otherwise the
creating transaction aborted and they're gone for that reason, or it
committed and they're gone because they're on-commit-drop. And
regardless of whether the transaction we are preparing goes on to
commit or abort, those tables will be gone afterwards for the same
reasons. So there doesn't in this case seem to be any reason to keep
them around until the transaction's fate is known.
Isn't that what happens already? PrepareTransaction() calls
PreCommit_on_commit_actions() from what I can tell.
--
Vik Fearing +33 6 46 75 15 36
http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support
On Fri, Jan 18, 2019 at 4:50 AM Vik Fearing <vik.fearing@2ndquadrant.com> wrote:
Isn't that what happens already? PrepareTransaction() calls
PreCommit_on_commit_actions() from what I can tell.
Huh. Well, in that case, I'm not sure I understand we really need to
do beyond removing the error checks for the case where all tables are
on-commit-drop.
It could be useful to do something about the issue with pg_temp
creation that Tom linked to in the other thread. But even if you
didn't do that, it'd be pretty easy to work around this in application
code -- just issue a dummy CREATE TEMP TABLE .. ON COMMIT DROP
statement the first time you use a connection, so that the temp schema
definitely exists. So I'm not sure I'd view that as a blocker for
this patch, even though it's kind of a sucky limitation.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
On Fri, Jan 18, 2019 at 10:39:46AM -0500, Robert Haas wrote:
Huh. Well, in that case, I'm not sure I understand we really need to
do beyond removing the error checks for the case where all tables are
on-commit-drop.
I have not looked at the patch in details, but we should really be
careful that if we do that the namespace does not remain behind when
performing such transactions so as it cannot be dropped. On my very
recent lookups of this class of problems you can easily finish by
blocking a backend from shutting down when dropping its temporary
schema, with the client, say psql, already able to disconnect. So as
long as the 2PC transaction is not COMMIT PREPARED the backend-side
wait will not be able to complete, blocking a backend slot in shared
memory. PREPARE TRANSACTION is very close to a simple commit in terms
of its semantics, while COMMIT PREPARED is just here to finish
releasing resources.
It could be useful to do something about the issue with pg_temp
creation that Tom linked to in the other thread. But even if you
didn't do that, it'd be pretty easy to work around this in application
code -- just issue a dummy CREATE TEMP TABLE .. ON COMMIT DROP
statement the first time you use a connection, so that the temp schema
definitely exists. So I'm not sure I'd view that as a blocker for
this patch, even though it's kind of a sucky limitation.
That's not really user-friendly, still workable. Or you could just
call current_schema() ;)
--
Michael
On Sat, Jan 19, 2019 at 10:39:43AM +0900, Michael Paquier wrote:
I have not looked at the patch in details, but we should really be
careful that if we do that the namespace does not remain behind when
performing such transactions so as it cannot be dropped. On my very
recent lookups of this class of problems you can easily finish by
blocking a backend from shutting down when dropping its temporary
schema, with the client, say psql, already able to disconnect. So as
long as the 2PC transaction is not COMMIT PREPARED the backend-side
wait will not be able to complete, blocking a backend slot in shared
memory. PREPARE TRANSACTION is very close to a simple commit in terms
of its semantics, while COMMIT PREPARED is just here to finish
releasing resources.
I have been looking at this patch, which conflicts on HEAD by the way
(Sorry!) still it is easy enough to get rid of the conflict, and from
what I can see it does not completely do its job. Simply take the
following example:
=# begin;
BEGIN
=# create temp table aa (a int ) on commit drop;
CREATE TABLE
=# prepare transaction 'ad';
PREPARE TRANSACTION
=# \q
This causes the client to think that the session is finished, but if
you look closer at the backend it is still pending to close until the
transaction is COMMIT PREPARED:
michael 22126 0.0 0.0 218172 15788 ? Ss 15:59 0:00
postgres: michael michael [local] idle waiting
Here is a backtrace:
#7 0x00005616900d0462 in LockAcquireExtended (locktag=0x7ffdd6bd5390,
lockmode=8, sessionLock=false, dontWait=false,
reportMemoryError=true, locallockp=0x0)
at lock.c:1050
#8 0x00005616900cf9ab in LockAcquire (locktag=0x7ffdd6bd5390,
lockmode=8, sessionLock=false, dontWait=false) at lock.c:713
#9 0x00005616900ced07 in LockDatabaseObject (classid=2615,
objid=16385, objsubid=0, lockmode=8) at lmgr.c:934
#10 0x000056168fd8cace in AcquireDeletionLock (object=0x7ffdd6bd5414,
flags=0) at dependency.c:1389
#11 0x000056168fd8b398 in performDeletion (object=0x7ffdd6bd5414,
behavior=DROP_CASCADE, flags=29) at dependency.c:325
#12 0x000056168fda103a in RemoveTempRelations (tempNamespaceId=16385)
at namespace.c:4142
#13 0x000056168fda106d in RemoveTempRelationsCallback (code=0, arg=0)
at namespace.c:4161
If you really intend to drop any trace of the objects at PREPARE
phase, that does not seem completely impossible to me, still you would
also need handling for the case where the temp table created also
creates the temporary schema for the session.
--
Michael
On Mon, Jan 28, 2019 at 04:06:11PM +0900, Michael Paquier wrote:
If you really intend to drop any trace of the objects at PREPARE
phase, that does not seem completely impossible to me, still you would
also need handling for the case where the temp table created also
creates the temporary schema for the session.
More work needs to be done, and the patch has problems, so I am
marking this patch as returned with feedback.
--
Michael