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