|From:||Vik Fearing <vik(at)2ndquadrant(dot)fr>|
|To:||Robert Haas <robertmhaas(at)gmail(dot)com>|
|Cc:||PostgreSQL-development <pgsql-hackers(at)postgresql(dot)org>, Andres Freund <andres(at)anarazel(dot)de>|
|Subject:||Re: Idle In Transaction Session Timeout, revived|
|Views:||Raw Message | Whole Thread | Download mbox | Resend email|
On 03/08/2016 10:42 PM, Robert Haas wrote:
> On Sun, Jan 31, 2016 at 8:33 AM, Vik Fearing <vik(at)2ndquadrant(dot)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
>> 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
>> 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;
> rhaas=# lock table pg_class;
> LOCK TABLE
> rhaas=# savepoint a;
> 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
> - 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
> - 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
Vik Fearing +33 6 46 75 15 36
http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support
|Next Message||Constantin S. Pan||2016-03-16 00:11:15||Re: [WIP] speeding up GIN build with parallel workers|
|Previous Message||David Rowley||2016-03-16 00:04:34||Re: Parallel Aggregate|