Idle In Transaction Session Timeout, revived
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.
--
Vik Fearing +33 6 46 75 15 36
http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support
Attachments:
iitt.v4.patchtext/x-patch; name=iitt.v4.patchDownload
diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index 392eb70..9cccf0e 100644
--- a/doc/src/sgml/config.sgml
+++ b/doc/src/sgml/config.sgml
@@ -5908,6 +5908,29 @@ 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 such sessions 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 3690753..b711da4 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 390816b..7f03d8e 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 e22d4db..66c1fea 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,7 @@ 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 +1180,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 38ba82f..d7322c2 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -2052,6 +2052,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 029114f..c7044da 100644
--- a/src/backend/utils/misc/postgresql.conf.sample
+++ b/src/backend/utils/misc/postgresql.conf.sample
@@ -527,6 +527,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 3441288..d31273c 100644
--- a/src/include/storage/proc.h
+++ b/src/include/storage/proc.h
@@ -241,6 +241,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 */
On Sun, Jan 31, 2016 at 8:33 AM, Vik Fearing <vik@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.
+1 for doing something like this. Great idea!
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 2/3/16 2:30 PM, Robert Haas wrote:
On Sun, Jan 31, 2016 at 8:33 AM, Vik Fearing <vik@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.
+1 for doing something like this. Great idea!
Wouldn't it be more sensible to just roll the transaction back and not
disconnect?
--
Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX
Experts in Analytics, Data Architecture and PostgreSQL
Data in Trouble? Get it in Treble! http://BlueTreble.com
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Wed, Feb 3, 2016 at 3:41 PM, Jim Nasby <Jim.Nasby@bluetreble.com> wrote:
On 2/3/16 2:30 PM, Robert Haas wrote:
On Sun, Jan 31, 2016 at 8:33 AM, Vik Fearing <vik@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.
+1 for doing something like this. Great idea!
Wouldn't it be more sensible to just roll the transaction back and not
disconnect?
It would be nice to be able to do that, but the client-server protocol
can't handle it without losing sync. Basically, if you send an error
to an idle client, you have to kill the session. This has come up
many times before.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Robert Haas <robertmhaas@gmail.com> writes:
On Wed, Feb 3, 2016 at 3:41 PM, Jim Nasby <Jim.Nasby@bluetreble.com> wrote:
Wouldn't it be more sensible to just roll the transaction back and not
disconnect?
It would be nice to be able to do that, but the client-server protocol
can't handle it without losing sync. Basically, if you send an error
to an idle client, you have to kill the session. This has come up
many times before.
Well, you can't just spit out an unprompted error message and go back to
waiting for the next command; as Robert says, that would leave the wire
protocol state out of sync. But in principle we could kill the
transaction and not say anything to the client right then. Instead set
some state that causes the next command from the client to get an error.
(This would not be much different from what happens when you send a
command in an already-reported-failed transaction; though we'd want to
issue a different error message than for that case.)
I'm not sure how messy this would be in practice. But if we think that
killing the whole session is not desirable but something we're doing for
expediency, then it would be worth looking into that approach.
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 2/3/16 4:25 PM, Tom Lane wrote:
Robert Haas <robertmhaas@gmail.com> writes:
On Wed, Feb 3, 2016 at 3:41 PM, Jim Nasby <Jim.Nasby@bluetreble.com> wrote:
Wouldn't it be more sensible to just roll the transaction back and not
disconnect?I'm not sure how messy this would be in practice. But if we think that
killing the whole session is not desirable but something we're doing for
expediency, then it would be worth looking into that approach.
I think killing the session is a perfectly sensible thing to do in this
case. Everything meaningful that was done in the session will be rolled
back - no need to waste resources keeping the connection open.
--
-David
david@pgmasters.net
On 2/3/16 4:05 PM, David Steele wrote:
On 2/3/16 4:25 PM, Tom Lane wrote:
Robert Haas <robertmhaas@gmail.com> writes:
On Wed, Feb 3, 2016 at 3:41 PM, Jim Nasby <Jim.Nasby@bluetreble.com> wrote:
Wouldn't it be more sensible to just roll the transaction back and not
disconnect?I'm not sure how messy this would be in practice. But if we think that
killing the whole session is not desirable but something we're doing for
expediency, then it would be worth looking into that approach.I think killing the session is a perfectly sensible thing to do in this
case. Everything meaningful that was done in the session will be rolled
back - no need to waste resources keeping the connection open.
Except you end up losing stuff like every GUC you've set, existing temp
tables, etc. For an application that presumably doesn't matter, but for
a user connection it would be a PITA.
I wouldn't put a bunch of effort into it though. Dropping the connection
is certainly better than nothing.
--
Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX
Experts in Analytics, Data Architecture and PostgreSQL
Data in Trouble? Get it in Treble! http://BlueTreble.com
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 02/03/2016 11:36 PM, Jim Nasby wrote:
On 2/3/16 4:05 PM, David Steele wrote:
On 2/3/16 4:25 PM, Tom Lane wrote:
Robert Haas <robertmhaas@gmail.com> writes:
On Wed, Feb 3, 2016 at 3:41 PM, Jim Nasby <Jim.Nasby@bluetreble.com>
wrote:Wouldn't it be more sensible to just roll the transaction back and not
disconnect?I'm not sure how messy this would be in practice. But if we think that
killing the whole session is not desirable but something we're doing for
expediency, then it would be worth looking into that approach.I think killing the session is a perfectly sensible thing to do in this
case. Everything meaningful that was done in the session will be rolled
back - no need to waste resources keeping the connection open.
That was the consensus last time I presented this bikeshed for painting.
Except you end up losing stuff like every GUC you've set, existing temp
tables, etc. For an application that presumably doesn't matter, but for
a user connection it would be a PITA.I wouldn't put a bunch of effort into it though. Dropping the connection
is certainly better than nothing.
You could always put SET idle_in_transaction_session_timeout = 0; in
your .psqlrc file to exempt your manual sessions from it. Or change it
just for your user or something.
--
Vik Fearing +33 6 46 75 15 36
http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Wed, Feb 3, 2016 at 5:36 PM, Jim Nasby <Jim.Nasby@bluetreble.com> wrote:
I think killing the session is a perfectly sensible thing to do in this
case. Everything meaningful that was done in the session will be rolled
back - no need to waste resources keeping the connection open.Except you end up losing stuff like every GUC you've set, existing temp
tables, etc. For an application that presumably doesn't matter, but for a
user connection it would be a PITA.I wouldn't put a bunch of effort into it though. Dropping the connection is
certainly better than nothing.
Well, my view is that if somebody wants an alternative behavior
besides dropping the connection, they can write a patch to provide
that as an additional option. That, too, has been discussed before.
But the fact that somebody might want that doesn't make this a bad or
useless behavior. Indeed, I'd venture that more people would want
this than would want that.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 02/03/2016 02:52 PM, Robert Haas wrote:
On Wed, Feb 3, 2016 at 5:36 PM, Jim Nasby <Jim.Nasby@bluetreble.com> wrote:
I think killing the session is a perfectly sensible thing to do in this
case. Everything meaningful that was done in the session will be rolled
back - no need to waste resources keeping the connection open.Except you end up losing stuff like every GUC you've set, existing temp
tables, etc. For an application that presumably doesn't matter, but for a
user connection it would be a PITA.I wouldn't put a bunch of effort into it though. Dropping the connection is
certainly better than nothing.Well, my view is that if somebody wants an alternative behavior
besides dropping the connection, they can write a patch to provide
that as an additional option. That, too, has been discussed before.
But the fact that somebody might want that doesn't make this a bad or
useless behavior. Indeed, I'd venture that more people would want
this than would want that.
Something feels wrong about just dropping the connection. I can see
doing what connection poolers do (DISCARD ALL) + a rollback but the idea
that we are going to destroy a connection to the database due to an idle
transaction seems like a potential foot gun. Unfortunately, outside of a
feeling I can not provide a good example.
Sincerely,
JD
--
Command Prompt, Inc. http://the.postgres.company/
+1-503-667-4564
PostgreSQL Centered full stack support, consulting and development.
Everyone appreciates your honesty, until you are honest with them.
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
"Joshua D. Drake" <jd@commandprompt.com> writes:
On 02/03/2016 02:52 PM, Robert Haas wrote:
Well, my view is that if somebody wants an alternative behavior
besides dropping the connection, they can write a patch to provide
that as an additional option. That, too, has been discussed before.
But the fact that somebody might want that doesn't make this a bad or
useless behavior. Indeed, I'd venture that more people would want
this than would want that.
Something feels wrong about just dropping the connection.
I have the same uneasy feeling about it as JD. However, you could
certainly argue that if the client application has lost its marbles
to the extent of allowing a transaction to time out, there's no good
reason to suppose that it will wake up any time soon, and then we are
definitely wasting resources by letting it monopolize a backend. Not as
many resources as if the xact were still open, but waste none the less.
My design sketch wherein we do nothing to notify the client certainly
doesn't do anything to help the client wake up, either. So maybe it's
fine and we should just go forward with the kill-the-connection approach.
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Wed, Feb 3, 2016 at 6:16 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
"Joshua D. Drake" <jd@commandprompt.com> writes:
On 02/03/2016 02:52 PM, Robert Haas wrote:
Well, my view is that if somebody wants an alternative behavior
besides dropping the connection, they can write a patch to provide
that as an additional option. That, too, has been discussed before.
But the fact that somebody might want that doesn't make this a bad or
useless behavior. Indeed, I'd venture that more people would want
this than would want that.Something feels wrong about just dropping the connection.
I have the same uneasy feeling about it as JD. However, you could
certainly argue that if the client application has lost its marbles
to the extent of allowing a transaction to time out, there's no good
reason to suppose that it will wake up any time soon, ...
That's exactly what I think. If you imagine a user who starts a
transaction and then leaves for lunch, aborting the transaction seems
nicer than killing the connection. But what I think really happens is
some badly-written Java application loses track of a connection
someplace and just never finds it again. Again, I'm not averse to
having both behavior someday, but my gut feeling is that killing the
connection will be the more useful one.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 2/3/16 8:04 PM, Robert Haas wrote:
On Wed, Feb 3, 2016 at 6:16 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
"Joshua D. Drake" <jd@commandprompt.com> writes:
On 02/03/2016 02:52 PM, Robert Haas wrote:
Well, my view is that if somebody wants an alternative behavior
besides dropping the connection, they can write a patch to provide
that as an additional option. That, too, has been discussed before.
But the fact that somebody might want that doesn't make this a bad or
useless behavior. Indeed, I'd venture that more people would want
this than would want that.Something feels wrong about just dropping the connection.
I have the same uneasy feeling about it as JD. However, you could
certainly argue that if the client application has lost its marbles
to the extent of allowing a transaction to time out, there's no good
reason to suppose that it will wake up any time soon, ...<...> But what I think really happens is
some badly-written Java application loses track of a connection
someplace and just never finds it again. <...>
That's what I've seen over and over again. And then sometimes it's not
a badly-written Java application, but me, and in that case I definitely
want the connection killed. Without logging, if you please.
--
-David
david@pgmasters.net
David Steele wrote:
<...> But what I think really happens is
some badly-written Java application loses track of a connection
someplace and just never finds it again. <...>
I've seen that also, plenty of times.
That's what I've seen over and over again. And then sometimes it's not
a badly-written Java application, but me, and in that case I definitely
want the connection killed. Without logging, if you please.
So the way to escape audit logging is to open a transaction, steal some
data, then leave the connection open so that it's not logged when it's
killed?
--
�lvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Sun, Jan 31, 2016 at 10:33 PM, Vik Fearing <vik@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.
+1
But, IIRC, one of the problems that prevent the adoption of this feature is
the addition of gettimeofday() call after every SQL command receipt.
Have you already resolved that problem? Or we don't need to care about
it because it's almost harmless?
Regards,
--
Fujii Masao
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 02/04/2016 02:24 PM, Fujii Masao wrote:
On Sun, Jan 31, 2016 at 10:33 PM, Vik Fearing <vik@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.+1
But, IIRC, one of the problems that prevent the adoption of this feature is
the addition of gettimeofday() call after every SQL command receipt.
Have you already resolved that problem? Or we don't need to care about
it because it's almost harmless?
I guess it would be possible to look at MyBEEntry somehow and pull
st_state_start_timestamp from it to replace the call to
GetCurrentTimestamp(), but I don't know if it's worth doing that.
The extra call only happens if the timeout is enabled anyway, so I don't
think it matters enough to be a blocker.
--
Vik Fearing +33 6 46 75 15 36
http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 2/4/16 5:00 AM, Alvaro Herrera wrote:
David Steele wrote:
<...> But what I think really happens is
some badly-written Java application loses track of a connection
someplace and just never finds it again. <...>I've seen that also, plenty of times.
That's what I've seen over and over again. And then sometimes it's not
a badly-written Java application, but me, and in that case I definitely
want the connection killed. Without logging, if you please.So the way to escape audit logging is to open a transaction, steal some
data, then leave the connection open so that it's not logged when it's
killed?
Well, of course I was joking, but even so I only meant the disconnect
shouldn't be logged to save me embarrassment.
But you are probably joking as well. Oh, what a tangled web.
--
-David
david@pgmasters.net
On 2016-02-04 22:24:50 +0900, Fujii Masao wrote:
But, IIRC, one of the problems that prevent the adoption of this feature is
the addition of gettimeofday() call after every SQL command receipt.
Have you already resolved that problem? Or we don't need to care about
it because it's almost harmless?
Well, it only happens when the feature is enabled, not
unconditionally. So I don't think that's particularly bad, as long as
the feature is not enabled by default.
If we feel we need to something about the gettimeofday() call at some
point, I think it'd made a lot of sense to introduce a more centralize
"statement stop" time, and an extended pgstat_report_activity() that
allows to specify the timestamp. Because right now we'll essentially do
gettimeofday() calls successively when starting a command, starting a
transaction, committing a transaction, and finishing a comment. That's
pretty pointless.
Andres
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 31/01/2016 14:33, Vik Fearing 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.
Hello,
I've looked at this patch, which I'd be able to review as a user, probably not
at a code level.
It seems to me this is a need in a huge number of badly handled idle in
transaction sessions (at application level).
This feature works as I expected it to.
My question would be regarding the value 0 assigned to the GUC parameter to
disable it. Wouldn't be -1 a better value, similar to
log_min_duration_statement or similar GUC parameter?
(I understand you can't put a 0ms timeout duration, but -1 seems more
understandable).
Best regards,
--
Stéphane Schildknecht
Contact régional PostgreSQL pour l'Europe francophone
Loxodata - Conseil, expertise et formations
06.17.11.37.42
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 4 February 2016 at 09:04, Robert Haas <robertmhaas@gmail.com> wrote:
I have the same uneasy feeling about it as JD. However, you could
certainly argue that if the client application has lost its marbles
to the extent of allowing a transaction to time out, there's no good
reason to suppose that it will wake up any time soon, ...That's exactly what I think. If you imagine a user who starts a
transaction and then leaves for lunch, aborting the transaction seems
nicer than killing the connection. But what I think really happens is
some badly-written Java application loses track of a connection
someplace and just never finds it again. Again, I'm not averse to
having both behavior someday, but my gut feeling is that killing the
connection will be the more useful one.
Applications - and users - must be prepared for the fact that uncommitted
data and session state may be lost at any time. The fact that PostgreSQL
tries not to lose it is quite nice, but gives people a false sense of
security too. Someone trips over a cable, a carrier has a bit of a BGP
hiccup, a NAT conntrack timeout occurs, there's an ECC parity check error
causing a proc kill ... your state can go away.
If you really don't want your session terminated, don't set an idle in
transaction session idle timeout (or override it).
(In some ways I think we're too good at this; I really should write an
extension that randomly aborts some low percentage of xacts with fake
deadlocks or serialization failures and randomly kills occasional
connections so that apps actually use their retry paths...)
Sure, it'd be *nice* to just terminate the xact and have a separate param
for timing out idle sessions whether or not they're in an xact. Cleaner -
terminate the xact if there's an xact-related timeout, terminate the
session if there's a session-related timeout. But nobody's written that
patch and this proposal solves a real world problem well enough.
Terminating the xact without terminating the session is a little tricky as
noted earlier so it's not a simple change to switch to that.
I'd be happy to have this. I won't mind having it if we eventually add an
idle_xact_timeout and idle_session_timeout in 9.something too.
--
Craig Ringer http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
On Sun, Jan 31, 2016 at 8:33 AM, Vik Fearing <vik@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
Attachments:
iitt.v5.patchapplication/x-patch; name=iitt.v5.patchDownload
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 */
On 2016-03-08 16:42:37 -0500, Robert Haas wrote:
- 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
Probably only if the toplevel transaction is also aborted. Might not be
entirely trivial to determine.
- 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.
Hm, we should never bomb out of the middle of anything with this, right?
I mean all the itmeout handler does is set a volatile var and set a
latch, the rest is done in the interrupt handler? Which is not called in
the signal handler.
I'm no targuing for the current order, just that one argument ;). FWIW,
I think Vik wrote this after discussing with me, and IIRC there was not
a lot of reasoning going into the specific location for doing this.
Greetings,
Andres Freund
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Tue, Mar 8, 2016 at 6:08 PM, Andres Freund <andres@anarazel.de> wrote:
On 2016-03-08 16:42:37 -0500, Robert Haas wrote:
- 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 zeroProbably only if the toplevel transaction is also aborted. Might not be
entirely trivial to determine.
Yes, that would be one way to do it - or just ignore whether it's
aborted or not and make the timeout always apply. That seems pretty
reasonable, too, because a transaction that's idle in transaction and
aborted could easily be one that the client has forgotten about, even
if it's not hanging onto any resources other than a connection slot.
And, if it turns out that the client didn't forget about it, well,
they don't lose anything by retrying the transaction on a new
connection anyway.
On a procedural note, I'm happy to push this patch through to commit
if it gets updated in the near future. If not, we should mark it
Returned with Feedback and Vik can resubmit for 9.7. Tempus fugit.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 2016-03-15 14:21:34 -0400, Robert Haas wrote:
On Tue, Mar 8, 2016 at 6:08 PM, Andres Freund <andres@anarazel.de> wrote:
On 2016-03-08 16:42:37 -0500, Robert Haas wrote:
- 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 zeroProbably only if the toplevel transaction is also aborted. Might not be
entirely trivial to determine.Yes, that would be one way to do it - or just ignore whether it's
aborted or not and make the timeout always apply. That seems pretty
reasonable, too, because a transaction that's idle in transaction and
aborted could easily be one that the client has forgotten about, even
if it's not hanging onto any resources other than a connection slot.
And, if it turns out that the client didn't forget about it, well,
they don't lose anything by retrying the transaction on a new
connection anyway.
I'm fine with both.
Andres
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 03/08/2016 10:42 PM, Robert Haas wrote:
On Sun, Jan 31, 2016 at 8:33 AM, Vik Fearing <vik@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
Attachments:
iitt.v6.patchtext/x-patch; name=iitt.v6.patchDownload
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 */
On Tue, Mar 15, 2016 at 8:08 PM, Vik Fearing <vik@2ndquadrant.fr> wrote:
On 03/08/2016 10:42 PM, Robert Haas wrote:
On Sun, Jan 31, 2016 at 8:33 AM, Vik Fearing <vik@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 zeroRevisited. 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.
Committed with slight changes to the docs, and I added a flag variable
instead of relying on IdleInTransactionSessionTimeout not changing at
an inopportune time.
Thanks.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 03/16/2016 05:32 PM, Robert Haas wrote:
Committed with slight changes to the docs, and I added a flag variable
instead of relying on IdleInTransactionSessionTimeout not changing at
an inopportune time.
Thank you!
--
Vik Fearing +33 6 46 75 15 36
http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
2016-03-16 17:54 GMT+01:00 Vik Fearing <vik@2ndquadrant.fr>:
On 03/16/2016 05:32 PM, Robert Haas wrote:
Committed with slight changes to the docs, and I added a flag variable
instead of relying on IdleInTransactionSessionTimeout not changing at
an inopportune time.Thank you!
great, important feature
Thank you
Pavel
Show quoted text
--
Vik Fearing +33 6 46 75 15 36
http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Wed, Mar 16, 2016 at 8:32 AM, Robert Haas <robertmhaas@gmail.com> wrote:
Committed with slight changes to the docs, and I added a flag variable
instead of relying on IdleInTransactionSessionTimeout not changing at
an inopportune time.
Thanks for committing, this should be a useful feature.
I get a pretty strange error message:
jjanes=# set idle_in_transaction_session_timeout = "1s";
jjanes=# begin;
-- wait for more than 1 second.
jjanes=# select count(*) from pgbench_accounts;
FATAL: terminating connection due to idle-in-transaction timeout
server closed the connection unexpectedly
This probably means the server terminated abnormally
before or while processing the request.
The connection to the server was lost. Attempting reset: Succeeded.
First it tells me exactly why the connection was killed, then it tells
me it doesn't know why the connection was killed and probably the
server has crashed.
I can't find the spot in the code where the FATAL message is getting
printed. I suppose it will not be easy to pass a
"dont_plead_ignorance" flag over to the part that prints the follow-up
message?
Thanks.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Fri, Mar 18, 2016 at 10:08 PM, Jeff Janes <jeff.janes@gmail.com> wrote:
On Wed, Mar 16, 2016 at 8:32 AM, Robert Haas <robertmhaas@gmail.com> wrote:
Committed with slight changes to the docs, and I added a flag variable
instead of relying on IdleInTransactionSessionTimeout not changing at
an inopportune time.Thanks for committing, this should be a useful feature.
I get a pretty strange error message:
jjanes=# set idle_in_transaction_session_timeout = "1s";
jjanes=# begin;
-- wait for more than 1 second.
jjanes=# select count(*) from pgbench_accounts;
FATAL: terminating connection due to idle-in-transaction timeout
server closed the connection unexpectedly
This probably means the server terminated abnormally
before or while processing the request.
The connection to the server was lost. Attempting reset: Succeeded.First it tells me exactly why the connection was killed, then it tells
me it doesn't know why the connection was killed and probably the
server has crashed.I can't find the spot in the code where the FATAL message is getting
printed. I suppose it will not be easy to pass a
"dont_plead_ignorance" flag over to the part that prints the follow-up
message?
The "This probably means the server terminated abnormally" message is
coming from libpq, while the FATAL error is coming from the server.
One might think that libpq should be prepared for the connection to be
closed if the server has sent a FATAL error, but I think the fact that
it isn't is a general problem, not related to this particular patch.
It would be good to think about how we might fix that...
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers