Re: idle_in_transaction_timeout

Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> Robert Haas <robertmhaas(at)gmail(dot)com> writes:

>>> I thought the reason why this hasn't been implemented before
>>> now is that sending an ErrorResponse to the client will result
>>> in a loss of protocol sync.
>>
>> Hmm ... you are right that this isn't as simple as an
>> ereport(ERROR), but I'm not sure it's impossible.  We could for
>> instance put the backend into skip-till-Sync state so that it
>> effectively ignored the next command message.  Causing that to
>> happen might be impracticably messy, though.
>
> Another thing we could maybe do is AbortCurrentTransaction() and
> send the client a NoticeResponse saying "hey, expect all of your
> future commands to fail with complaints about the transaction
> being aborted".

Wow, until I read through the thread on this patch and the old
thread that Robert linked to I had forgotten the attempt by Andres
to deal with this three and a half years ago.  That patch died
because of how complicated it was to do the right thing if the
connection was left open.

Personally, where I have seen a use for this, treating it as FATAL
would be better anyway; but was OK with treating it as an ERROR if
that didn't sink the patch.  I guess that puts me in the same camp
as Josh and Robert.

Vik has submitted v1 and v2 of this patch; the only difference
between them is that v1 makes a timeout FATAL and v2 makes it an
ERROR (with appropriate corresponding changes to code comments,
documentation, and message text).  It's not hard to show the
problems with an ERROR under v2, confirming that the simple
approach of just changing the ereport severity is not enough:

test=# set idle_in_transaction_timeout = '3s';
SET
test=# begin;
BEGIN
test=# select 1;
ERROR:  current transaction is aborted due to idle-in-transaction timeout
test=# commit;
ERROR:  current transaction is aborted, commands ignored until end of transaction block
test=# commit;
ROLLBACK

The first thing is that I don't think a delay between the BEGIN and
the SELECT should cause a timeout to trigger, but more importantly
there should not be two ERROR responses to one SELECT statement.

I'm inclined to abandon the ERROR approach as not worth the effort
and fragility, and focus on v1 of the patch.  If we can't get to
consensus on that, I think that this patch should be flagged
"Returned with Feedback", noting that any follow-up version
requires some way to deal with the issues raised regarding multiple
ERROR messages.

Abridged for space. hopefully without losing the main points of the
authors, so far:

Josh Berkus:
> My $0.019999999999998: terminating the connection is the
> preferred behavior.

Tom Lane:
> FWIW, I think aborting the transaction is probably better,
> especially if the patch is designed to do nothing to
> already-aborted transactions.  If the client is still there, it
> will see the abort as a failure in its next query, which is less
> likely to confuse it completely than a connection loss.  (I
> think, anyway.)

Álvaro Herrera:
> I think if we want this at all it should abort the xact, not drop
> the connection.

Andrew Dunstan:
> [quotes Tom's argument]
> Yes, I had the same thought.

David G Johnston:
> Default to dropping the connection but give the
> usersadministrators the capability to decide for themselves?

Robert Haas:
> Assuming that the state of play hasn't changed in some way I'm
> unaware of since 2010, I think the best argument for FATAL here
> is that it's what we can implement.  I happen to think it's
> better anyway, because the cases I've seen where this would
> actually be useful involve badly-written applications that are
> not under the same administrative control as the database and
> supposedly have built-in connection poolers, except sometimes
> they seem to forget about some of the connections they themselves
> opened.  The DBAs can't make the app developers fix the app; they
> just have to try to survive.  Aborting the transaction is a step
> in the right direction but since the guy at the other end of the
> connection is actually or in effect dead, that just leaves you
> with an eternally idle connection.

Tom Lane (reprise):
> I'm not sure whether cancel-transaction behavior is enough better
> than cancel-session to warrant extra effort here.

Andres Freund:
> [quotes Tom's latest (above)]
> I don't think idle_in_transaction_timeout is worth this, but if
> we could implement it we could finally have recovery conflicts
> against idle in txn sessions not use FATAL...

Kevin Grittner:
> Personally, where I have seen a use for this, treating it as
> FATAL would be better anyway

A few ideas were put forward on how a much more complicated patch
could allow transaction rollback with an ERROR work acceptably, but
I think that would probably need to be a new patch in a later CF,
so that is omitted here.

So, can we agree to use FATAL to terminate the connection?

--
Kevin Grittner
EDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company