Thread: 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
Attachment
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
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
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
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
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
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
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
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.
"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
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
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
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
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
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
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
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
On 4 February 2016 at 09:04, Robert Haas <robertmhaas@gmail.com> wrote:
If you really don't want your session terminated, don't set an idle in transaction session idle timeout (or override it).
> 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.
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
Attachment
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
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 zero > > Probably 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
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 zero > > > > Probably 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
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
Attachment
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 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. 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
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
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
--
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 whileprocessing 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
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