On Tue, Mar 15, 2016 at 8:08 PM, Vik Fearing <v...@2ndquadrant.fr> wrote: > On 03/08/2016 10:42 PM, Robert Haas wrote: >> On Sun, Jan 31, 2016 at 8:33 AM, Vik Fearing <v...@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 -- Sent via pgsql-hackers mailing list (firstname.lastname@example.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers