Discarding DISCARD ALL
Currently, session poolers operating in transaction mode need to send
a "server_reset_query" which is mostly DISCARD ALL.
It seems strange to me that we put this work onto the pooler, forcing
poolers to repeatedly issue the same command, at some cost in
performance. Measuring the overhead with pgbench might miss the points
that poolers are frequently configured on different network hosts and
that monitoring tools used in production will record the DISCARD
statement. YMMV, but the overhead is measurably non-zero.
Proposal is to have a simple new parameter:
transaction_cleanup = off (default) | on
A setting of "on" will issue the equivalent of a DISCARD ALL as soon
as the transaction has been ended by a COMMIT, ROLLBACK or PREPARE.
Poolers such as pgbouncer would then be able to connect transaction
mode pools by setting transaction_cleanup=on at time of connection,
avoiding any need to issue a server_reset_query, removing the DISCARD
ALL command from the normal execution path, while still achieving the
same thing.
This has an additional side benefit: if we know we will clean up at
the end of the transaction, then all temp tables become effectively ON
COMMIT DROP and we are able to allow temp tables in prepared
transactions. There are likely other side benefits from this
knowledge, allowing us to further tune the PostgreSQL server to the
common use case of transaction session poolers. I think it should be
possible to avoid looking for holdable portals if we are dropping them
all anyway.
Patch attached, passes make check with new tests added.
Comments welcome.
--
Simon Riggs http://www.EnterpriseDB.com/
Attachments:
transaction_cleanup.v4.patchapplication/octet-stream; name=transaction_cleanup.v4.patchDownload
diff --git a/src/backend/access/transam/xact.c b/src/backend/access/transam/xact.c
index 9cd0b7c11b..6db7b37dad 100644
--- a/src/backend/access/transam/xact.c
+++ b/src/backend/access/transam/xact.c
@@ -35,6 +35,8 @@
#include "catalog/pg_enum.h"
#include "catalog/storage.h"
#include "commands/async.h"
+#include "commands/prepare.h"
+#include "commands/sequence.h"
#include "commands/tablecmds.h"
#include "commands/trigger.h"
#include "executor/spi.h"
@@ -80,6 +82,8 @@ bool XactReadOnly;
bool DefaultXactDeferrable = false;
bool XactDeferrable;
+bool transaction_cleanup = false;
+
int synchronous_commit = SYNCHRONOUS_COMMIT_ON;
/*
@@ -336,6 +340,8 @@ static void CommitTransaction(void);
static TransactionId RecordTransactionAbort(bool isSubXact);
static void StartTransaction(void);
+static void PostXactSessionCleanup(void);
+
static void StartSubTransaction(void);
static void CommitSubTransaction(void);
static void AbortSubTransaction(void);
@@ -2132,11 +2138,23 @@ CommitTransaction(void)
/* Shut down the deferred-trigger manager */
AfterTriggerEndXact(true);
- /*
- * Let ON COMMIT management do its thing (must happen after closing
- * cursors, to avoid dangling-reference problems)
- */
- PreCommit_on_commit_actions();
+ /* Apply cleanup consistently across all end of xact actions */
+ if (transaction_cleanup && !IsBootstrapProcessingMode())
+ {
+ MyXactFlags |= XACT_FLAGS_CLEANUP_AT_XACT_END;
+
+ /* Closing portals might run user-defined code, so do that first. */
+ PortalHashTableDeleteAll();
+ ResetTempTableNamespace();
+ }
+ else
+ {
+ /*
+ * Let ON COMMIT management do its thing (must happen after closing
+ * cursors, to avoid dangling-reference problems)
+ */
+ PreCommit_on_commit_actions();
+ }
/*
* Synchronize files that are created and not WAL-logged during this
@@ -2372,11 +2390,23 @@ PrepareTransaction(void)
/* Shut down the deferred-trigger manager */
AfterTriggerEndXact(true);
- /*
- * Let ON COMMIT management do its thing (must happen after closing
- * cursors, to avoid dangling-reference problems)
- */
- PreCommit_on_commit_actions();
+ /* Apply cleanup consistently across all end of xact actions */
+ if (transaction_cleanup && !IsBootstrapProcessingMode())
+ {
+ MyXactFlags |= XACT_FLAGS_CLEANUP_AT_XACT_END;
+
+ /* Closing portals might run user-defined code, so do that first. */
+ PortalHashTableDeleteAll();
+ ResetTempTableNamespace();
+ }
+ else
+ {
+ /*
+ * Let ON COMMIT management do its thing (must happen after closing
+ * cursors, to avoid dangling-reference problems)
+ */
+ PreCommit_on_commit_actions();
+ }
/*
* Synchronize files that are created and not WAL-logged during this
@@ -2413,11 +2443,12 @@ PrepareTransaction(void)
* We must check this after executing any ON COMMIT actions, because they
* might still access a temp relation.
*
- * 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.
+ * This can be relaxed if we are using transaction_cleanup since we know
+ * that any temp tables will have been created and dropped all within the
+ * transaction.
*/
- if ((MyXactFlags & XACT_FLAGS_ACCESSEDTEMPNAMESPACE))
+ if ((MyXactFlags & XACT_FLAGS_ACCESSEDTEMPNAMESPACE) &&
+ !(MyXactFlags & XACT_FLAGS_CLEANUP_AT_XACT_END))
ereport(ERROR,
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
errmsg("cannot PREPARE a transaction that has operated on temporary objects")));
@@ -2840,6 +2871,25 @@ CleanupTransaction(void)
s->state = TRANS_DEFAULT;
}
+/*
+ * Perform same actions as DISCARD ALL, but don't try to execute that
+ * directly since it expects to be inside a new transaction.
+ */
+static void
+PostXactSessionCleanup(void)
+{
+ if ((MyXactFlags & XACT_FLAGS_CLEANUP_AT_XACT_END))
+ {
+ SetPGVariable("session_authorization", NIL, false);
+ ResetAllOptions();
+ DropAllPreparedStatements();
+ LockReleaseAll(USER_LOCKMETHOD, true);
+ ResetPlanCache();
+ ResetSequenceCaches();
+ }
+}
+
+
/*
* StartTransactionCommand
*/
@@ -3010,6 +3060,8 @@ CommitTransactionCommand(void)
s->chain = false;
RestoreTransactionCharacteristics();
}
+ else
+ PostXactSessionCleanup();
break;
/*
@@ -3036,6 +3088,8 @@ CommitTransactionCommand(void)
s->chain = false;
RestoreTransactionCharacteristics();
}
+ else
+ PostXactSessionCleanup();
break;
/*
@@ -3054,6 +3108,8 @@ CommitTransactionCommand(void)
s->chain = false;
RestoreTransactionCharacteristics();
}
+ else
+ PostXactSessionCleanup();
break;
/*
@@ -3062,6 +3118,7 @@ CommitTransactionCommand(void)
*/
case TBLOCK_PREPARE:
PrepareTransaction();
+ PostXactSessionCleanup();
s->blockState = TBLOCK_DEFAULT;
break;
diff --git a/src/backend/commands/async.c b/src/backend/commands/async.c
index e04afd9963..ee7e5dba59 100644
--- a/src/backend/commands/async.c
+++ b/src/backend/commands/async.c
@@ -1018,6 +1018,12 @@ AtCommit_Notify(void)
}
}
+ /*
+ * Remove all listenChannels directly, if we need to cleanup
+ */
+ if (amRegisteredListener && transaction_cleanup)
+ Exec_UnlistenAllCommit();
+
/* If no longer listening to anything, get out of listener array */
if (amRegisteredListener && listenChannels == NIL)
asyncQueueUnregister();
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index dabcbb0736..d24a3ff285 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -1633,6 +1633,15 @@ static struct config_bool ConfigureNamesBool[] =
false,
NULL, NULL, NULL
},
+ {
+ {"transaction_cleanup", PGC_USERSET, CLIENT_CONN_STATEMENT,
+ gettext_noop("Whether session cleanup occurs immediately following transaction completion."),
+ NULL
+ },
+ &transaction_cleanup,
+ false,
+ NULL, NULL, NULL
+ },
{
{"transaction_deferrable", PGC_USERSET, CLIENT_CONN_STATEMENT,
gettext_noop("Whether to defer a read-only serializable transaction until it can be executed with no possible serialization failures."),
diff --git a/src/include/access/xact.h b/src/include/access/xact.h
index 7320de345c..b1bbac2cb8 100644
--- a/src/include/access/xact.h
+++ b/src/include/access/xact.h
@@ -86,6 +86,8 @@ extern int synchronous_commit;
extern PGDLLIMPORT TransactionId CheckXidAlive;
extern PGDLLIMPORT bool bsysscan;
+extern bool transaction_cleanup;
+
/*
* Miscellaneous flag bits to record events which occur on the top level
* transaction. These flags are only persisted in MyXactFlags and are intended
@@ -107,6 +109,8 @@ extern int MyXactFlags;
*/
#define XACT_FLAGS_ACQUIREDACCESSEXCLUSIVELOCK (1U << 1)
+#define XACT_FLAGS_CLEANUP_AT_XACT_END (1U << 2)
+
/*
* start- and end-of-transaction callbacks for dynamically loaded modules
*/
diff --git a/src/test/regress/expected/guc.out b/src/test/regress/expected/guc.out
index 811f80a097..1c5552495c 100644
--- a/src/test/regress/expected/guc.out
+++ b/src/test/regress/expected/guc.out
@@ -530,13 +530,13 @@ SELECT relname FROM pg_class WHERE relname = 'reset_test';
--
-- Test DISCARD ALL
--
+CREATE ROLE regress_guc_user;
-- do changes
DECLARE foo CURSOR WITH HOLD FOR SELECT 1;
PREPARE foo AS SELECT 1;
LISTEN foo_event;
SET vacuum_cost_delay = 13;
CREATE TEMP TABLE tmp_foo (data text) ON COMMIT DELETE ROWS;
-CREATE ROLE regress_guc_user;
SET SESSION AUTHORIZATION regress_guc_user;
-- look changes
SELECT pg_listening_channels();
@@ -610,6 +610,81 @@ SELECT current_user = 'regress_guc_user';
f
(1 row)
+-- do changes, with cleanup
+SET transaction_cleanup = on;
+BEGIN;
+DECLARE foo CURSOR WITH HOLD FOR SELECT 1;
+PREPARE foo AS SELECT 1;
+LISTEN foo_event;
+SET vacuum_cost_delay = 13;
+CREATE TEMP TABLE tmp_foo (data text) ON COMMIT DELETE ROWS;
+SET SESSION AUTHORIZATION regress_guc_user;
+-- look changes
+SELECT name FROM pg_prepared_statements;
+ name
+------
+ foo
+(1 row)
+
+SELECT name FROM pg_cursors;
+ name
+------
+ foo
+(1 row)
+
+SHOW vacuum_cost_delay;
+ vacuum_cost_delay
+-------------------
+ 13ms
+(1 row)
+
+SELECT relname from pg_class where relname = 'tmp_foo';
+ relname
+---------
+ tmp_foo
+(1 row)
+
+SELECT current_user = 'regress_guc_user';
+ ?column?
+----------
+ t
+(1 row)
+
+COMMIT;
+SET transaction_cleanup = off;
+-- look again, should be same as DISCARD ALL
+SELECT pg_listening_channels();
+ pg_listening_channels
+-----------------------
+(0 rows)
+
+SELECT name FROM pg_prepared_statements;
+ name
+------
+(0 rows)
+
+SELECT name FROM pg_cursors;
+ name
+------
+(0 rows)
+
+SHOW vacuum_cost_delay;
+ vacuum_cost_delay
+-------------------
+ 0
+(1 row)
+
+SELECT relname from pg_class where relname = 'tmp_foo';
+ relname
+---------
+(0 rows)
+
+SELECT current_user = 'regress_guc_user';
+ ?column?
+----------
+ f
+(1 row)
+
DROP ROLE regress_guc_user;
--
-- search_path should react to changes in pg_namespace
diff --git a/src/test/regress/sql/guc.sql b/src/test/regress/sql/guc.sql
index 43dbba3775..e7f4095f82 100644
--- a/src/test/regress/sql/guc.sql
+++ b/src/test/regress/sql/guc.sql
@@ -160,13 +160,14 @@ SELECT relname FROM pg_class WHERE relname = 'reset_test';
-- Test DISCARD ALL
--
+CREATE ROLE regress_guc_user;
+
-- do changes
DECLARE foo CURSOR WITH HOLD FOR SELECT 1;
PREPARE foo AS SELECT 1;
LISTEN foo_event;
SET vacuum_cost_delay = 13;
CREATE TEMP TABLE tmp_foo (data text) ON COMMIT DELETE ROWS;
-CREATE ROLE regress_guc_user;
SET SESSION AUTHORIZATION regress_guc_user;
-- look changes
SELECT pg_listening_channels();
@@ -184,6 +185,32 @@ SELECT name FROM pg_cursors;
SHOW vacuum_cost_delay;
SELECT relname from pg_class where relname = 'tmp_foo';
SELECT current_user = 'regress_guc_user';
+
+-- do changes, with cleanup
+SET transaction_cleanup = on;
+BEGIN;
+DECLARE foo CURSOR WITH HOLD FOR SELECT 1;
+PREPARE foo AS SELECT 1;
+LISTEN foo_event;
+SET vacuum_cost_delay = 13;
+CREATE TEMP TABLE tmp_foo (data text) ON COMMIT DELETE ROWS;
+SET SESSION AUTHORIZATION regress_guc_user;
+-- look changes
+SELECT name FROM pg_prepared_statements;
+SELECT name FROM pg_cursors;
+SHOW vacuum_cost_delay;
+SELECT relname from pg_class where relname = 'tmp_foo';
+SELECT current_user = 'regress_guc_user';
+COMMIT;
+SET transaction_cleanup = off;
+-- look again, should be same as DISCARD ALL
+SELECT pg_listening_channels();
+SELECT name FROM pg_prepared_statements;
+SELECT name FROM pg_cursors;
+SHOW vacuum_cost_delay;
+SELECT relname from pg_class where relname = 'tmp_foo';
+SELECT current_user = 'regress_guc_user';
+
DROP ROLE regress_guc_user;
--
Simon>It seems strange to me that we put this work onto the pooler, forcing
Simon>poolers to repeatedly issue the same command
What if poolers learn to manage connections and prepared statements better?
Then poolers won't have to reset the session every time, and everyone wins.
Simon>This has an additional side benefit: if we know we will clean up at
Simon>the end of the transaction, then all temp tables become effectively ON
Simon>COMMIT DROP
The ability to use the temporary tables sounds cool, however,
server-prepared statements
allow improve performance significantly (both at frontend and backend), so
I would not treat
"server_reset_query=discard all" as a best practice.
That is why transaction_cleanup=on (discard everything at transaction
finish) seems to be a workaround
rather than a best practice for the future.
Just to clarify: the patch seems to be small, so it looks harmless,
however, I would not be that
enthusiastic with tying new features with transaction_cleanup=on.
---
What do you mean by "session cleanup"?
Does that always have to be the same as "discard all"?
What if the user wants "deallocate all" behavior?
Should transaction_cleanup be an enum rather than on/off?
Vladimir
On 12/23/20 3:47 PM, Vladimir Sitnikov wrote:
Simon>It seems strange to me that we put this work onto the pooler, forcing
Simon>poolers to repeatedly issue the same commandWhat if poolers learn to manage connections and prepared statements better?
Then poolers won't have to reset the session every time, and everyone wins.
While that is be possible to implement since some client libraries
implement this in their pools (e.g. Sequel for Ruby) this patch would
help connection poolers which are not aware of prepared statements, for
example PgBouncer, so it is worthwhile as long as there are connection
poolers out there which are not aware of prepared statements. And even
the connection poolers which are aware might want to automatically drop
temporary tables and reset GUCs. So I do not think that this feature
would become pointless even if people write a patch for PgBouncer.
Simon>This has an additional side benefit: if we know we will clean up at
Simon>the end of the transaction, then all temp tables become effectively ON
Simon>COMMIT DROPThe ability to use the temporary tables sounds cool, however,
server-prepared statements
allow improve performance significantly (both at frontend and backend),
so I would not treat
"server_reset_query=discard all" as a best practice.
That is why transaction_cleanup=on (discard everything at transaction
finish) seems to be a workaround
rather than a best practice for the future.
While I know how to add support for prepared statements to a connection
pooler it is unclear to me how one would add support for temporary
tables which are not at least ON COMMIT DELETE ROWS since rows inserted
on one connection will only be visible at the connection.
Andreas
On Wed, 23 Dec 2020 at 15:19, Andreas Karlsson <andreas@proxel.se> wrote:
On 12/23/20 3:47 PM, Vladimir Sitnikov wrote:
Simon>It seems strange to me that we put this work onto the pooler, forcing
Simon>poolers to repeatedly issue the same commandWhat if poolers learn to manage connections and prepared statements better?
Then poolers won't have to reset the session every time, and everyone wins.While that is be possible to implement since some client libraries
implement this in their pools (e.g. Sequel for Ruby) this patch would
help connection poolers which are not aware of prepared statements, for
example PgBouncer, so it is worthwhile as long as there are connection
poolers out there which are not aware of prepared statements. And even
the connection poolers which are aware might want to automatically drop
temporary tables and reset GUCs. So I do not think that this feature
would become pointless even if people write a patch for PgBouncer.
The whole premise of the patch is tighter integration, with the server
providing the facilities that poolers need.
The patch can be enhanced to do whatever else we agree is desirable.
Do we need something like DISCARD ALL EXCEPT PREPARED STATEMENTS; ??
If there are different requirements for each pooler, what are they?
--
Simon Riggs http://www.EnterpriseDB.com/
On 12/23/20 6:49 PM, Simon Riggs wrote:
The whole premise of the patch is tighter integration, with the server
providing the facilities that poolers need.
I am all for that. Ideally I would want builtin connection pooling but
short term I think the way forward is most likely tighter integration.
The patch can be enhanced to do whatever else we agree is desirable.
Do we need something like DISCARD ALL EXCEPT PREPARED STATEMENTS; ??
If there are different requirements for each pooler, what are they?
If someone adds prepared statement support to e.g. PgBouncer that might
be a nice feature to have. Plus maybe something like "SET
transaction_cleanup = 'except_prepared'". But right now I think the
pools which support prepared statements do not use DISCARD ALL and
instead just trust the end user to not run SET or use temporary tables
which outlive transactions.
Andreas
On 2020-12-23 15:33, Simon Riggs wrote:
Poolers such as pgbouncer would then be able to connect transaction
mode pools by setting transaction_cleanup=on at time of connection,
avoiding any need to issue a server_reset_query, removing the DISCARD
ALL command from the normal execution path, while still achieving the
same thing.
PgBouncer does not send DISCARD ALL in transaction mode. There is a
separate setting to do that, but it's not the default, and it's more of
a workaround for bad client code. So I don't know if this feature would
be of much use for PgBouncer. Other connection poolers might have other
opinions.
I hope to do further review of the patch later this week, but I wanted
to at least comment on this piece:
On Wed, Jan 20, 2021 at 2:48 AM Peter Eisentraut
<peter.eisentraut@enterprisedb.com> wrote:
On 2020-12-23 15:33, Simon Riggs wrote:
Poolers such as pgbouncer would then be able to connect transaction
mode pools by setting transaction_cleanup=on at time of connection,
avoiding any need to issue a server_reset_query, removing the DISCARD
ALL command from the normal execution path, while still achieving the
same thing.PgBouncer does not send DISCARD ALL in transaction mode. There is a
separate setting to do that, but it's not the default, and it's more of
a workaround for bad client code. So I don't know if this feature would
be of much use for PgBouncer. Other connection poolers might have other
opinions.
Yes, to have server_reset_query apply in transaction pooling mode you
have to additionally configure pgbouncer with
server_reset_query_always enabled.
I'd mildly take issue with "a workaround for bad client code". Yes,
clients in transaction pooling mode shouldn't issue (for example) `SET
...`, but there's no way I'm aware of in Postgres to prevent
session-specific items like those GUCs from being set by a given user,
so I view it more like a safeguard than a workaround.
In our setup we have server_reset_query_always=1 as such a safeguard,
because it's too easy for application code to update, for example,
statement_timeout to disastrous results. But we also work to make sure
those don't happen (or get cleaned up if they happen to slip in).
An alternative approach that occurred to me while typing this reply: a
setting in Postgres that would disallow setting session level GUCs
(i.e., enforce `SET LOCAL` transaction level usage instead) would
remove a large chunk of our need to set server_reset_query_always=1
(and more interestingly it'd highlight when broken code gets pushed).
But even with that, I see some value in the proposed setting since
there is additional session state beyond GUCs.
James
On Wed, 20 Jan 2021 at 14:21, James Coleman <jtc331@gmail.com> wrote:
An alternative approach that occurred to me while typing this reply: a
setting in Postgres that would disallow setting session level GUCs
(i.e., enforce `SET LOCAL` transaction level usage instead) would
remove a large chunk of our need to set server_reset_query_always=1
(and more interestingly it'd highlight when broken code gets pushed).
But even with that, I see some value in the proposed setting since
there is additional session state beyond GUCs.
With transaction_cleanup=on we could force all SETs to be SET LOCAL.
The point is that if we declare ahead of time that the transaction
will be reset then we can act differently and more easily for various
circumstances, for SETs, for Temp tables and others.
--
Simon Riggs http://www.EnterpriseDB.com/
On Wed, Jan 20, 2021 at 9:58 AM Simon Riggs <simon@2ndquadrant.com> wrote:
On Wed, 20 Jan 2021 at 14:21, James Coleman <jtc331@gmail.com> wrote:
An alternative approach that occurred to me while typing this reply: a
setting in Postgres that would disallow setting session level GUCs
(i.e., enforce `SET LOCAL` transaction level usage instead) would
remove a large chunk of our need to set server_reset_query_always=1
(and more interestingly it'd highlight when broken code gets pushed).
But even with that, I see some value in the proposed setting since
there is additional session state beyond GUCs.With transaction_cleanup=on we could force all SETs to be SET LOCAL.
The point is that if we declare ahead of time that the transaction
will be reset then we can act differently and more easily for various
circumstances, for SETs, for Temp tables and others.
Right, I agree it's independently useful. My "alternative" is a subset
of that functionality and doesn't cover as many cases.
James
On Wed, Jan 20, 2021 at 3:53 PM James Coleman <jtc331@gmail.com> wrote:
On Wed, Jan 20, 2021 at 9:58 AM Simon Riggs <simon@2ndquadrant.com> wrote:
On Wed, 20 Jan 2021 at 14:21, James Coleman <jtc331@gmail.com> wrote:
An alternative approach that occurred to me while typing this reply: a
setting in Postgres that would disallow setting session level GUCs
(i.e., enforce `SET LOCAL` transaction level usage instead) would
remove a large chunk of our need to set server_reset_query_always=1
(and more interestingly it'd highlight when broken code gets pushed).
But even with that, I see some value in the proposed setting since
there is additional session state beyond GUCs.With transaction_cleanup=on we could force all SETs to be SET LOCAL.
The point is that if we declare ahead of time that the transaction
will be reset then we can act differently and more easily for various
circumstances, for SETs, for Temp tables and others.Right, I agree it's independently useful. My "alternative" is a subset
of that functionality and doesn't cover as many cases.
So if we go for that option, would we call it?
session_state = 'session' (default) | 'local_set'
If you use 'local' then you find that all state is transaction only
* SET defaults to meaning SET LOCAL
* SET SESSION returns ERROR
--
Simon Riggs http://www.EnterpriseDB.com/