Re: Idle In Transaction Session Timeout, revived

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
> 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