Re: Idle In Transaction Session Timeout, revived

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