On 03/08/2016 10:42 PM, Robert Haas wrote:
> 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.

Attached is version 6 of this patch.

> 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

Revisited.  All idle transactions are now affected, even aborted ones.
I had not thought about subtransactions.

> - I wonder if the documentation should mention potential advantages
> related to holding back xmin.

I guess I kind of punted on this in the new patch.  I briefly mention it
and then link to the routine-vacuuming docs.  I can reword that if
necessary.

> - 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.

There is no better reason for putting it there than "it seemed like a
good idea at the time".  I've looked into it a bit more, and I don't see
any danger of having it there, but I can certainly move it if you think
I should.

> - It would be nice if you reviewed someone else's patch in turn.

I have been reviewing other, small patches.  I am signed up for several
in this commitfest that I will hopefully get to shortly, and I have
reviewed others in recent fests where I had no patch of my own.

I may be playing on the penny slots, but I'm still putting my coins in.

> I'm attaching a lightly-edited version of your patch.

I have incorporated your changes.

I also changed the name IdleInTransactionTimeoutSessionPending to the
thinko-free IdleInTransactionSessionTimeoutPending.
-- 
Vik Fearing                                          +33 6 46 75 15 36
http://2ndQuadrant.fr     PostgreSQL : Expertise, Formation et Support
diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index 6c73fb4..aaa3a71 100644
--- a/doc/src/sgml/config.sgml
+++ b/doc/src/sgml/config.sgml
@@ -6063,6 +6063,27 @@ 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. In particular,
+       it allows tuples visible only to this transaction to be vacuumed.  See
+       <xref linkend="routine-vacuuming"> for more details about this.
+       </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 74ef419..a66e07b 100644
--- a/src/backend/storage/lmgr/proc.c
+++ b/src/backend/storage/lmgr/proc.c
@@ -58,6 +58,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..6d5cc69 100644
--- a/src/backend/tcop/postgres.c
+++ b/src/backend/tcop/postgres.c
@@ -2978,6 +2978,18 @@ ProcessInterrupts(void)
 		}
 	}
 
+	if (IdleInTransactionSessionTimeoutPending)
+	{
+		/* Has the timeout setting changed since last we looked? */
+		if (IdleInTransactionSessionTimeout > 0)
+			ereport(FATAL,
+					(errcode(ERRCODE_IDLE_IN_TRANSACTION_SESSION_TIMEOUT),
+					 errmsg("terminating connection due to idle-in-transaction timeout")));
+		else
+			IdleInTransactionSessionTimeoutPending = false;
+
+	}
+
 	if (ParallelMessagePending)
 		HandleParallelMessages();
 }
@@ -3942,11 +3954,21 @@ PostgresMain(int argc, char *argv[],
 			{
 				set_ps_display("idle in transaction (aborted)", false);
 				pgstat_report_activity(STATE_IDLEINTRANSACTION_ABORTED, NULL);
+
+				/* Start the idle-in-transaction timer */
+				if (IdleInTransactionSessionTimeout > 0)
+					enable_timeout_after(IDLE_IN_TRANSACTION_SESSION_TIMEOUT,
+										 IdleInTransactionSessionTimeout);
 			}
 			else if (IsTransactionOrTransactionBlock())
 			{
 				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 +4009,16 @@ 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
+		 *
+		 * Make sure we do this before we reload the config file; the
+		 * administrator may have turned the feature off.
+		 */
+		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 +4028,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..597dab4 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 IdleInTransactionSessionTimeoutPending = 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..b3f1bc4 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)
+{
+	IdleInTransactionSessionTimeoutPending = 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 f0d4ec1..be9d5ca 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..9200f04 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 IdleInTransactionSessionTimeoutPending;
 
 extern volatile bool ClientConnectionLost;
 
diff --git a/src/include/storage/proc.h b/src/include/storage/proc.h
index 612fa05..c3b462c 100644
--- a/src/include/storage/proc.h
+++ b/src/include/storage/proc.h
@@ -257,6 +257,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 14e9720..f64921e 100644
--- a/src/include/utils/timeout.h
+++ b/src/include/utils/timeout.h
@@ -30,6 +30,7 @@ typedef enum TimeoutId
 	STANDBY_DEADLOCK_TIMEOUT,
 	STANDBY_TIMEOUT,
 	STANDBY_LOCK_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

Reply via email to