Re: Idle In Transaction Session Timeout, revived

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