Обсуждение: Idle In Transaction Session Timeout, revived

Поиск
Список
Период
Сортировка

Idle In Transaction Session Timeout, revived

От
Vik Fearing
Дата:
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

Вложения

Re: Idle In Transaction Session Timeout, revived

От
Robert Haas
Дата:
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



Re: Idle In Transaction Session Timeout, revived

От
Jim Nasby
Дата:
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



Re: Idle In Transaction Session Timeout, revived

От
Robert Haas
Дата:
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



Re: Idle In Transaction Session Timeout, revived

От
Tom Lane
Дата:
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



Re: Idle In Transaction Session Timeout, revived

От
David Steele
Дата:
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


Re: Idle In Transaction Session Timeout, revived

От
Jim Nasby
Дата:
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



Re: Idle In Transaction Session Timeout, revived

От
Vik Fearing
Дата:
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



Re: Idle In Transaction Session Timeout, revived

От
Robert Haas
Дата:
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



Re: Idle In Transaction Session Timeout, revived

От
"Joshua D. Drake"
Дата:
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.



Re: Idle In Transaction Session Timeout, revived

От
Tom Lane
Дата:
"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



Re: Idle In Transaction Session Timeout, revived

От
Robert Haas
Дата:
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



Re: Idle In Transaction Session Timeout, revived

От
David Steele
Дата:
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


Re: Idle In Transaction Session Timeout, revived

От
Fujii Masao
Дата:
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



Re: Idle In Transaction Session Timeout, revived

От
Alvaro Herrera
Дата:
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



Re: Idle In Transaction Session Timeout, revived

От
Vik Fearing
Дата:
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



Re: Idle In Transaction Session Timeout, revived

От
David Steele
Дата:
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


Re: Idle In Transaction Session Timeout, revived

От
Andres Freund
Дата:
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



Re: [Reveiw] Idle In Transaction Session Timeout, revived

От
Stéphane Schildknecht
Дата:
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



Re: Idle In Transaction Session Timeout, revived

От
Craig Ringer
Дата:
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

Re: Idle In Transaction Session Timeout, revived

От
Robert Haas
Дата:
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

Вложения

Re: Idle In Transaction Session Timeout, revived

От
Andres Freund
Дата:
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



Re: Idle In Transaction Session Timeout, revived

От
Robert Haas
Дата:
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



Re: Idle In Transaction Session Timeout, revived

От
Andres Freund
Дата:
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



Re: Idle In Transaction Session Timeout, revived

От
Vik Fearing
Дата:
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

Вложения

Re: Idle In Transaction Session Timeout, revived

От
Robert Haas
Дата:
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



Re: Idle In Transaction Session Timeout, revived

От
Vik Fearing
Дата:
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



Re: Idle In Transaction Session Timeout, revived

От
Pavel Stehule
Дата:


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

Re: Idle In Transaction Session Timeout, revived

От
Jeff Janes
Дата:
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



Re: Idle In Transaction Session Timeout, revived

От
Robert Haas
Дата:
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