On Sun, Jan 31, 2016 at 8:33 AM, Vik Fearing <v...@2ndquadrant.fr> wrote:
> Attached is a rebased and revised version of my
> idle_in_transaction_session_timeout patch from last year.
>
> This version does not suffer the problems the old one did where it would
> jump out of SSL code thanks to Andres' patch in commit
> 4f85fde8eb860f263384fffdca660e16e77c7f76.
>
> The basic idea is if a session remains idle in a transaction for longer
> than the configured time, that connection will be dropped thus releasing
> the connection slot and any locks that may have been held by the broken
> client.
>
> Added to the March commitfest.
I see this patch has been marked Ready for Committer despite the lack
of any really substantive review. Generally, I think it looks good.
But I have a couple of questions/comments:
- I really wonder if the decision to ignore sessions that are idle in
transaction (aborted) should revisited. Consider this:
rhaas=# begin;
BEGIN
rhaas=# lock table pg_class;
LOCK TABLE
rhaas=# savepoint a;
SAVEPOINT
rhaas=# select 1/0;
ERROR: division by zero
- I wonder if the documentation should mention potential advantages
related to holding back xmin.
- What's the right order of events in PostgresMain? Right now the
patch disables the timeout after checking for interrupts and clearing
DoingCommandRead, but maybe it would be better to disable the timeout
first, so that we can't have it happen that start to execute the
command and then, in medias res, bomb out because of the idle timeout.
Then again, maybe you had some compelling reason for doing it this
way, in which case we should document that in the comments.
- It would be nice if you reviewed someone else's patch in turn.
I'm attaching a lightly-edited version of your patch.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index a09ceb2..cdd5d77 100644
--- a/doc/src/sgml/config.sgml
+++ b/doc/src/sgml/config.sgml
@@ -5976,6 +5976,30 @@ COPY postgres_log FROM '/full/path/to/logfile.csv' WITH csv;
</listitem>
</varlistentry>
+ <varlistentry id="guc-idle-in-transaction-session-timeout" xreflabel="idle_in_transaction_session_timeout">
+ <term><varname>idle_in_transaction_session_timeout</varname> (<type>integer</type>)
+ <indexterm>
+ <primary><varname>idle_in_transaction_session_timeout</> configuration parameter</primary>
+ </indexterm>
+ </term>
+ <listitem>
+ <para>
+ Terminate any session with an open transaction that has been idle for
+ longer than the specified duration in milliseconds. This allows any
+ locks to be released and the connection slot to be reused.
+ </para>
+ <para>
+ Sessions in the state "idle in transaction (aborted)" occupy a
+ connection slot, but because they hold no locks, they are not considered
+ by this parameter.
+ </para>
+ <para>
+ The default value of 0 means that sessions which are idle in transaction
+ will will not be terminated.
+ </para>
+ </listitem>
+ </varlistentry>
+
<varlistentry id="guc-vacuum-freeze-table-age" xreflabel="vacuum_freeze_table_age">
<term><varname>vacuum_freeze_table_age</varname> (<type>integer</type>)
<indexterm>
diff --git a/doc/src/sgml/mvcc.sgml b/doc/src/sgml/mvcc.sgml
index dd3c775..6352e12 100644
--- a/doc/src/sgml/mvcc.sgml
+++ b/doc/src/sgml/mvcc.sgml
@@ -725,7 +725,9 @@ ERROR: could not serialize access due to read/write dependencies among transact
<listitem>
<para>
Don't leave connections dangling <quote>idle in transaction</quote>
- longer than necessary.
+ longer than necessary. The configuration parameter
+ <xref linkend="guc-idle-in-transaction-session-timeout"> may be used to
+ automatically disconnect lingering sessions.
</para>
</listitem>
<listitem>
diff --git a/src/backend/storage/lmgr/proc.c b/src/backend/storage/lmgr/proc.c
index 6453b88..109d090 100644
--- a/src/backend/storage/lmgr/proc.c
+++ b/src/backend/storage/lmgr/proc.c
@@ -57,6 +57,7 @@
int DeadlockTimeout = 1000;
int StatementTimeout = 0;
int LockTimeout = 0;
+int IdleInTransactionSessionTimeout = 0;
bool log_lock_waits = false;
/* Pointer to this process's PGPROC and PGXACT structs, if any */
diff --git a/src/backend/tcop/postgres.c b/src/backend/tcop/postgres.c
index 115166b..8a75dd2 100644
--- a/src/backend/tcop/postgres.c
+++ b/src/backend/tcop/postgres.c
@@ -2978,6 +2978,11 @@ ProcessInterrupts(void)
}
}
+ if (IdleInTransactionTimeoutSessionPending)
+ ereport(FATAL,
+ (errcode(ERRCODE_IDLE_IN_TRANSACTION_SESSION_TIMEOUT),
+ errmsg("terminating connection due to idle-in-transaction timeout")));
+
if (ParallelMessagePending)
HandleParallelMessages();
}
@@ -3947,6 +3952,11 @@ PostgresMain(int argc, char *argv[],
{
set_ps_display("idle in transaction", false);
pgstat_report_activity(STATE_IDLEINTRANSACTION, NULL);
+
+ /* Start the idle-in-transaction timer */
+ if (IdleInTransactionSessionTimeout > 0)
+ enable_timeout_after(IDLE_IN_TRANSACTION_SESSION_TIMEOUT,
+ IdleInTransactionSessionTimeout);
}
else
{
@@ -3987,7 +3997,13 @@ PostgresMain(int argc, char *argv[],
DoingCommandRead = false;
/*
- * (5) check for any other interesting events that happened while we
+ * (5) turn off the idle-in-transaction timeout
+ */
+ if (IdleInTransactionSessionTimeout > 0)
+ disable_timeout(IDLE_IN_TRANSACTION_SESSION_TIMEOUT, false);
+
+ /*
+ * (6) check for any other interesting events that happened while we
* slept.
*/
if (got_SIGHUP)
@@ -3997,7 +4013,7 @@ PostgresMain(int argc, char *argv[],
}
/*
- * (6) process the command. But ignore it if we're skipping till
+ * (7) process the command. But ignore it if we're skipping till
* Sync.
*/
if (ignore_till_sync && firstchar != EOF)
diff --git a/src/backend/utils/errcodes.txt b/src/backend/utils/errcodes.txt
index 04c9c00..1a920e8 100644
--- a/src/backend/utils/errcodes.txt
+++ b/src/backend/utils/errcodes.txt
@@ -229,6 +229,7 @@ Section: Class 25 - Invalid Transaction State
25007 E ERRCODE_SCHEMA_AND_DATA_STATEMENT_MIXING_NOT_SUPPORTED schema_and_data_statement_mixing_not_supported
25P01 E ERRCODE_NO_ACTIVE_SQL_TRANSACTION no_active_sql_transaction
25P02 E ERRCODE_IN_FAILED_SQL_TRANSACTION in_failed_sql_transaction
+25P03 E ERRCODE_IDLE_IN_TRANSACTION_SESSION_TIMEOUT idle_in_transaction_session_timeout
Section: Class 26 - Invalid SQL Statement Name
diff --git a/src/backend/utils/init/globals.c b/src/backend/utils/init/globals.c
index ccd9c8e..0069255 100644
--- a/src/backend/utils/init/globals.c
+++ b/src/backend/utils/init/globals.c
@@ -30,6 +30,7 @@ volatile bool InterruptPending = false;
volatile bool QueryCancelPending = false;
volatile bool ProcDiePending = false;
volatile bool ClientConnectionLost = false;
+volatile bool IdleInTransactionTimeoutSessionPending = false;
volatile uint32 InterruptHoldoffCount = 0;
volatile uint32 QueryCancelHoldoffCount = 0;
volatile uint32 CritSectionCount = 0;
diff --git a/src/backend/utils/init/postinit.c b/src/backend/utils/init/postinit.c
index 6b760d4..0779538 100644
--- a/src/backend/utils/init/postinit.c
+++ b/src/backend/utils/init/postinit.c
@@ -70,6 +70,7 @@ static void InitCommunication(void);
static void ShutdownPostgres(int code, Datum arg);
static void StatementTimeoutHandler(void);
static void LockTimeoutHandler(void);
+static void IdleInTransactionSessionTimeoutHandler(void);
static bool ThereIsAtLeastOneRole(void);
static void process_startup_options(Port *port, bool am_superuser);
static void process_settings(Oid databaseid, Oid roleid);
@@ -597,6 +598,8 @@ InitPostgres(const char *in_dbname, Oid dboid, const char *username,
RegisterTimeout(DEADLOCK_TIMEOUT, CheckDeadLockAlert);
RegisterTimeout(STATEMENT_TIMEOUT, StatementTimeoutHandler);
RegisterTimeout(LOCK_TIMEOUT, LockTimeoutHandler);
+ RegisterTimeout(IDLE_IN_TRANSACTION_SESSION_TIMEOUT,
+ IdleInTransactionSessionTimeoutHandler);
}
/*
@@ -1178,6 +1181,13 @@ LockTimeoutHandler(void)
kill(MyProcPid, SIGINT);
}
+static void
+IdleInTransactionSessionTimeoutHandler(void)
+{
+ IdleInTransactionTimeoutSessionPending = true;
+ InterruptPending = true;
+ SetLatch(MyLatch);
+}
/*
* Returns true if at least one role is defined in this database cluster.
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index ea5a09a..2c3fb45 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -2066,6 +2066,17 @@ static struct config_int ConfigureNamesInt[] =
},
{
+ {"idle_in_transaction_session_timeout", PGC_USERSET, CLIENT_CONN_STATEMENT,
+ gettext_noop("Sets the maximum allowed duration of any idling transaction."),
+ gettext_noop("A value of 0 turns off the timeout."),
+ GUC_UNIT_MS
+ },
+ &IdleInTransactionSessionTimeout,
+ 0, 0, INT_MAX,
+ NULL, NULL, NULL
+ },
+
+ {
{"vacuum_freeze_min_age", PGC_USERSET, CLIENT_CONN_STATEMENT,
gettext_noop("Minimum age at which VACUUM should freeze a table row."),
NULL
diff --git a/src/backend/utils/misc/postgresql.conf.sample b/src/backend/utils/misc/postgresql.conf.sample
index ee3d378..a6bb335 100644
--- a/src/backend/utils/misc/postgresql.conf.sample
+++ b/src/backend/utils/misc/postgresql.conf.sample
@@ -529,6 +529,7 @@
#session_replication_role = 'origin'
#statement_timeout = 0 # in milliseconds, 0 is disabled
#lock_timeout = 0 # in milliseconds, 0 is disabled
+#idle_in_transaction_session_timeout = 0 # in milliseconds, 0 is disabled
#vacuum_freeze_min_age = 50000000
#vacuum_freeze_table_age = 150000000
#vacuum_multixact_freeze_min_age = 5000000
diff --git a/src/include/miscadmin.h b/src/include/miscadmin.h
index cc7833e..3dd70f1 100644
--- a/src/include/miscadmin.h
+++ b/src/include/miscadmin.h
@@ -80,6 +80,7 @@
extern PGDLLIMPORT volatile bool InterruptPending;
extern PGDLLIMPORT volatile bool QueryCancelPending;
extern PGDLLIMPORT volatile bool ProcDiePending;
+extern PGDLLIMPORT volatile bool IdleInTransactionTimeoutSessionPending;
extern volatile bool ClientConnectionLost;
diff --git a/src/include/storage/proc.h b/src/include/storage/proc.h
index dbcdd3f..29d8b77 100644
--- a/src/include/storage/proc.h
+++ b/src/include/storage/proc.h
@@ -255,6 +255,7 @@ extern PGPROC *PreparedXactProcs;
extern int DeadlockTimeout;
extern int StatementTimeout;
extern int LockTimeout;
+extern int IdleInTransactionSessionTimeout;
extern bool log_lock_waits;
diff --git a/src/include/utils/timeout.h b/src/include/utils/timeout.h
index 723b475..59a4afe 100644
--- a/src/include/utils/timeout.h
+++ b/src/include/utils/timeout.h
@@ -29,6 +29,7 @@ typedef enum TimeoutId
STATEMENT_TIMEOUT,
STANDBY_DEADLOCK_TIMEOUT,
STANDBY_TIMEOUT,
+ IDLE_IN_TRANSACTION_SESSION_TIMEOUT,
/* First user-definable timeout reason */
USER_TIMEOUT,
/* Maximum number of timeout reasons */
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
