Cancelling idle in transaction state
Currently SIGINT is ignored during <IDLE> in transaction, but we have
recently agreed to allow this to cancel the transaction. We said we
would do this in all cases, so this is a separate feature/patch (though
Hot Standby requires it).
A simple change allows the transaction to be cancelled, but there are
some loose ends that I wish to discuss.
If we are running a statement and a cancel is received, then we return
the ERROR to the client, who is expecting it. If we cancel a transaction
while the connection is idle, we have no way of signalling to the client
program this has occurred. So the client finds out about this much
later, not in fact until the next message is sent.
Is there a mechanism for communicating the state back to the client?
Will this be handled correctly with existing code? psql appears to be
confused by a cancelled backend.
I'm not familiar with these aspects of the code, so some clear
suggestions are needed to allow me to work this out. I'm worried that
this will delay things further otherwise.
--
Simon Riggs www.2ndQuadrant.com
PostgreSQL Training, Services and Support
Added to TODO:
Allow administrators to cancel multi-statement idle
transactions
This allows locks to be released, but it is complex to report the
cancellation back to the client.
* http://archives.postgresql.org/pgsql-hackers/2008-12/msg01340.php
---------------------------------------------------------------------------
Simon Riggs wrote:
Currently SIGINT is ignored during <IDLE> in transaction, but we have
recently agreed to allow this to cancel the transaction. We said we
would do this in all cases, so this is a separate feature/patch (though
Hot Standby requires it).A simple change allows the transaction to be cancelled, but there are
some loose ends that I wish to discuss.If we are running a statement and a cancel is received, then we return
the ERROR to the client, who is expecting it. If we cancel a transaction
while the connection is idle, we have no way of signalling to the client
program this has occurred. So the client finds out about this much
later, not in fact until the next message is sent.Is there a mechanism for communicating the state back to the client?
Will this be handled correctly with existing code? psql appears to be
confused by a cancelled backend.I'm not familiar with these aspects of the code, so some clear
suggestions are needed to allow me to work this out. I'm worried that
this will delay things further otherwise.--
Simon Riggs www.2ndQuadrant.com
PostgreSQL Training, Services and Support--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
--
Bruce Momjian <bruce@momjian.us> http://momjian.us
EnterpriseDB http://enterprisedb.com
+ If your life is a hard drive, Christ can be your backup. +
On Wed, 2009-01-21 at 15:22 -0500, Bruce Momjian wrote:
Added to TODO:
Allow administrators to cancel multi-statement idle
transactionsThis allows locks to be released, but it is complex to report the
cancellation back to the client.* http://archives.postgresql.org/pgsql-hackers/2008-12/msg01340.php
This is part of Hot Standby.
The bug is on the TODO list.
--
Simon Riggs www.2ndQuadrant.com
PostgreSQL Training, Services and Support
Simon Riggs wrote:
On Wed, 2009-01-21 at 15:22 -0500, Bruce Momjian wrote:
Added to TODO:
Allow administrators to cancel multi-statement idle
transactionsThis allows locks to be released, but it is complex to report the
cancellation back to the client.* http://archives.postgresql.org/pgsql-hackers/2008-12/msg01340.php
This is part of Hot Standby.
The bug is on the TODO list.
Well, if it gets done for 8.4 then we can mark it completed; it not it
will be there for 8.5. The behavior is useful independent of hot
standby.
--
Bruce Momjian <bruce@momjian.us> http://momjian.us
EnterpriseDB http://enterprisedb.com
+ If your life is a hard drive, Christ can be your backup. +
On Wed, 2009-01-21 at 15:46 -0500, Bruce Momjian wrote:
Simon Riggs wrote:
On Wed, 2009-01-21 at 15:22 -0500, Bruce Momjian wrote:
Added to TODO:
Allow administrators to cancel multi-statement idle
transactionsThis allows locks to be released, but it is complex to report the
cancellation back to the client.* http://archives.postgresql.org/pgsql-hackers/2008-12/msg01340.php
This is part of Hot Standby.
The bug is on the TODO list.
Well, if it gets done for 8.4 then we can mark it completed; it not it
will be there for 8.5. The behavior is useful independent of hot
standby.
At one time there was also a positive discussion on having something
like:
idle_in_transaction_timeout
Does this play along with that?
Joshua D.D rake
--
Bruce Momjian <bruce@momjian.us> http://momjian.us
EnterpriseDB http://enterprisedb.com+ If your life is a hard drive, Christ can be your backup. +
--
PostgreSQL - XMPP: jdrake@jabber.postgresql.org
Consulting, Development, Support, Training
503-667-4564 - http://www.commandprompt.com/
The PostgreSQL Company, serving since 1997
Joshua D. Drake wrote:
On Wed, 2009-01-21 at 15:46 -0500, Bruce Momjian wrote:
Simon Riggs wrote:
On Wed, 2009-01-21 at 15:22 -0500, Bruce Momjian wrote:
Added to TODO:
Allow administrators to cancel multi-statement idle
transactionsThis allows locks to be released, but it is complex to report the
cancellation back to the client.* http://archives.postgresql.org/pgsql-hackers/2008-12/msg01340.php
This is part of Hot Standby.
The bug is on the TODO list.
Well, if it gets done for 8.4 then we can mark it completed; it not it
will be there for 8.5. The behavior is useful independent of hot
standby.At one time there was also a positive discussion on having something
like:idle_in_transaction_timeout
Yep, and already a TODO:
Add idle_in_transaction_timeout GUC so locks are not held for
long periods of time
--
Bruce Momjian <bruce@momjian.us> http://momjian.us
EnterpriseDB http://enterprisedb.com
+ If your life is a hard drive, Christ can be your backup. +
I'd be very grateful to any hackers out there who are looking for a
project before close of 8.5 to consider working on this. It's dang
useful, both for Hot Standby and normal processing.
(You'll have the added bonus of proving you're smarter than me!)
On Wed, 2009-01-21 at 15:22 -0500, Bruce Momjian wrote:
Added to TODO:
Allow administrators to cancel multi-statement idle
transactionsThis allows locks to be released, but it is complex to report the
cancellation back to the client.* http://archives.postgresql.org/pgsql-hackers/2008-12/msg01340.php
---------------------------------------------------------------------------
Simon Riggs wrote:
Currently SIGINT is ignored during <IDLE> in transaction, but we have
recently agreed to allow this to cancel the transaction. We said we
would do this in all cases, so this is a separate feature/patch (though
Hot Standby requires it).A simple change allows the transaction to be cancelled, but there are
some loose ends that I wish to discuss.If we are running a statement and a cancel is received, then we return
the ERROR to the client, who is expecting it. If we cancel a transaction
while the connection is idle, we have no way of signalling to the client
program this has occurred. So the client finds out about this much
later, not in fact until the next message is sent.Is there a mechanism for communicating the state back to the client?
Will this be handled correctly with existing code? psql appears to be
confused by a cancelled backend.I'm not familiar with these aspects of the code, so some clear
suggestions are needed to allow me to work this out. I'm worried that
this will delay things further otherwise.--
Simon Riggs www.2ndQuadrant.com
PostgreSQL Training, Services and Support
--
Bruce Momjian <bruce@momjian.us> http://momjian.us
EnterpriseDB http://enterprisedb.com+ If your life is a hard drive, Christ can be your backup. +
--
Simon Riggs www.2ndQuadrant.com
On Dec 5, 2009, at 12:25 PM, Simon Riggs wrote:
...
I'm not volunteering here, but having worked with the protocol, I do have a suggestion:
This allows locks to be released, but it is complex to report the
cancellation back to the client.
I think the answer here is that the server should *not* report the cancellation.
Rather, it should mark the transaction as failed and let the client eventually sync its state on subsequent requests that will result in InFailedTransaction ERRORs.
With such a solution, COMMITs issued to administrator cancelled transactions should result in an ERROR. Well, I suppose that would only be a requirement when:
BEGIN;
... some work ...
<idle>
<admin zapped this transaction>
<more idle>
COMMIT; <-- client needs to know that this failed,
and it should be something louder than
a "ROLLBACK" tag. :P
So, if a command were issued to a cancelled transaction prior to a COMMIT:
BEGIN;
... some work ...
<idle>
<admin zapped this transaction>
SELECT * FROM something; -- fails, IFX ERROR emitted to client
COMMIT; <-- client was already notified of
the xact failure by a prior command's error,
so the normal "ROLLBACK" would be fine.
Also, if immediate notification is seen as a necessity, a WARNING with a special code could be leveraged. Oh, or maybe use a dedicated LISTEN/NOTIFY channel? "LISTEN pg_darn_admin_zapped_my_xact;" to opt-in for transaction cancellation events that occur in *this* backend.. [Note: this is in addition to COMMITs emitting ERRORs]
I can't see immediate notification being useful excepting some rather strange situations where the client left the transaction idle to go do other expensive operations that "should" be immediately interrupted if this particular transaction were to be cancelled for some reason.. Such a situation might even make sense if those "expensive operations" somehow depended on the locks held by the transaction, but I think that's a stretch. Not to mention that the client could just occasionally poll the transaction with 'SELECT 1's; no special WARNING or NOTIFY's would be necessary.
On Sat, Dec 5, 2009 at 8:13 PM, James Pye <lists@jwp.name> wrote:
I think the answer here is that the server should *not* report the cancellation.
Rather, it should mark the transaction as failed and let the client eventually sync its state on subsequent requests that will result in InFailedTransaction ERRORs.
[...]
Also, if immediate notification is seen as a necessity, a WARNING with a special code could be leveraged. Oh, or maybe use a dedicated LISTEN/NOTIFY channel? "LISTEN pg_darn_admin_zapped_my_xact;" to opt-in for transaction cancellation events that occur in *this* backend.. [Note: this is in addition to COMMITs emitting ERRORs]
I think this line of thinking is on the right track. The server
should certainly not send back an immediate ERROR response, because
that will definitely confuse the client. Of course, any subsequent
commands will report ERRORs until the client rolls back. But it also
seems highly desirable for the server to send some sort of immediate,
asynchronous notification, so that a sufficiently smart client can
immediately report the error back to the user or take such other
action as it deems appropriate.
Currently, it appears that the only messages that the server can send
back asynchronously are ParameterStatus and NotificationResponse. So
we need to decide whether it's feasible/better to shoehorn this
functionality into one of those message types, or whether we should
bump the protocol version and add a new message type (cue: panic in
the streets). On first examination (and I am not an expert in this
area), ParameterStatus would seem to be the better choice, because it
appears to me that all clients must be prepared to cope with such
messages, whereas in theory a client might be unprepared for a
NotificationResponse if it never executes LISTEN. (It seems clearly
preferable not to require clients to issue an explicit LISTEN in order
to enable this feature.)
Going with that theory, we could pick a "magical" parameter status
value, either something like __transaction_cancelled, or maybe even
something that contains a character that isn't even legal in a normal
parameter, like $transaction_cancelled, if we don't think that will
break any clients. Then we could just report a value change for this
whenever an idle transaction is cancelled. Clients who ignore this
will find out when they next issue a query; others will know
immediately.
...Robert
Robert Haas <robertmhaas@gmail.com> writes:
I think this line of thinking is on the right track. The server
should certainly not send back an immediate ERROR response, because
that will definitely confuse the client. Of course, any subsequent
commands will report ERRORs until the client rolls back. But it also
seems highly desirable for the server to send some sort of immediate,
asynchronous notification, so that a sufficiently smart client can
immediately report the error back to the user or take such other
action as it deems appropriate.
If you must have that, send a NOTICE. I don't actually see the point
though. If the client was as smart and well-coded as all that, it
wouldn't be sitting on an open transaction in the first place.
Currently, it appears that the only messages that the server can send
back asynchronously are ParameterStatus and NotificationResponse.
Using either of those is completely inappropriate.
regards, tom lane
On Sat, Dec 5, 2009 at 10:15 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Robert Haas <robertmhaas@gmail.com> writes:
I think this line of thinking is on the right track. The server
should certainly not send back an immediate ERROR response, because
that will definitely confuse the client. Of course, any subsequent
commands will report ERRORs until the client rolls back. But it also
seems highly desirable for the server to send some sort of immediate,
asynchronous notification, so that a sufficiently smart client can
immediately report the error back to the user or take such other
action as it deems appropriate.If you must have that, send a NOTICE.
Ah ha! I missed that one. That's perfect.
I don't actually see the point
though. If the client was as smart and well-coded as all that, it
wouldn't be sitting on an open transaction in the first place.
Think about an interactive client. It's not the client's fault that
the user has chosen to begin a transaction and then sit there
cogitating, but the client would like to let the user know right away
that their current transaction is defunct.
...Robert
On Sat, 2009-12-05 at 18:13 -0700, James Pye wrote:
On Dec 5, 2009, at 12:25 PM, Simon Riggs wrote:
...
I'm not volunteering here, but having worked with the protocol, I do have a suggestion:
Thanks. Looks like good input. With the further clarification that we
use NOTIFY it seems a solution is forming.
Any other takers?
--
Simon Riggs www.2ndQuadrant.com
On Sun, 2009-12-06 at 07:58 +0000, Simon Riggs wrote:
On Sat, 2009-12-05 at 18:13 -0700, James Pye wrote:
On Dec 5, 2009, at 12:25 PM, Simon Riggs wrote:
...
I'm not volunteering here, but having worked with the protocol, I do have a suggestion:
Thanks. Looks like good input. With the further clarification that we
use NOTIFY it seems a solution is forming.
If we use notify, then "the sufficiently smart client" (tm) should
probably declared that it is waiting for such notify , no ?
That would mean, that it should have issued either
"LISTEN CANCEL_IDLE_TRX_<pid>"
or with the new payload enabled NOTIFY just
"LISTEN CANCEL_IDLE_TRX"
and then the NOTIFY would include the pid of rolled back backend and
possibly some other extra info.
Otoh, we could also come up with something that looks like a NOTIFY from
client end, but is sent only to one connection that is canceled instead
of all listeners.
--
Hannu Krosing http://www.2ndQuadrant.com
PostgreSQL Scalability and Availability
Services, Consulting and Training
On Dec 6, 2009, at 2:58 AM, Simon Riggs <simon@2ndQuadrant.com> wrote:
On Sat, 2009-12-05 at 18:13 -0700, James Pye wrote:
On Dec 5, 2009, at 12:25 PM, Simon Riggs wrote:
...
I'm not volunteering here, but having worked with the protocol, I
do have a suggestion:Thanks. Looks like good input. With the further clarification that we
use NOTIFY it seems a solution is forming.
Notice, not NOTIFY.
...Robert
Hannu Krosing <hannu@2ndquadrant.com> writes:
On Sun, 2009-12-06 at 07:58 +0000, Simon Riggs wrote:
Thanks. Looks like good input. With the further clarification that we
use NOTIFY it seems a solution is forming.
If we use notify, then "the sufficiently smart client" (tm) should
probably declared that it is waiting for such notify , no ?
We are using NOTICE, not NOTIFY, assuming that we use anything at all
(which I still regard as unnecessary). Please stop injecting confusion
into the discussion.
regards, tom lane
On Sun, Dec 6, 2009 at 4:23 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
We are using NOTICE, not NOTIFY, assuming that we use anything at all
(which I still regard as unnecessary). Please stop injecting confusion
into the discussion.
Attached is a minimal POC patch that allows to cancel an idle
transaction with SIGINT. The HS patch also allows this in its current
form but as Simon points out the client gets out of sync with it.
The proposal is to send an additional NOTICE to the client and abort
all open transactions and subtransactions (this is what I got from the
previous discussion).
I had to write an additional function AbortAnyTransaction() which
aborts all transactions and subtransactions and leaves the transaction
in the aborted state, is there an existing function to do this?
We'd probably want to add a timeout for idle transactions also (which
is a wishlist item since quite some time) and could also offer user
functions like pg_cancel_idle_transaction(). Along this we might need
to add internal reasons like we do for SIGUSR1 because we are now
multiplexing different functionality onto the SIGINT signal. One might
want to cancel an idle transaction only and not a running query,
without keeping track of internal reasons one risks to cancel a
legitimate query if that backend has started to work on a query again.
Comments?
Joachim
Attachments:
idletxn.difftext/x-diff; charset=US-ASCII; name=idletxn.diffDownload
diff -cr cvs/src/backend/access/transam/xact.c cvs.build/src/backend/access/transam/xact.c
*** cvs/src/backend/access/transam/xact.c 2009-12-24 13:55:12.000000000 +0100
--- cvs.build/src/backend/access/transam/xact.c 2009-12-24 20:55:17.000000000 +0100
***************
*** 2692,2697 ****
--- 2692,2735 ----
}
/*
+ * AbortAnyTransaction
+ */
+ void
+ AbortAnyTransaction(void)
+ {
+ TransactionState s = CurrentTransactionState;
+
+ switch (s->blockState)
+ {
+ case TBLOCK_DEFAULT:
+ case TBLOCK_STARTED:
+ case TBLOCK_BEGIN:
+ case TBLOCK_INPROGRESS:
+ case TBLOCK_END:
+ case TBLOCK_ABORT:
+ case TBLOCK_SUBABORT:
+ case TBLOCK_ABORT_END:
+ case TBLOCK_ABORT_PENDING:
+ case TBLOCK_PREPARE:
+ case TBLOCK_SUBABORT_END:
+ case TBLOCK_SUBABORT_RESTART:
+ AbortCurrentTransaction();
+ break;
+
+ case TBLOCK_SUBINPROGRESS:
+ case TBLOCK_SUBBEGIN:
+ case TBLOCK_SUBEND:
+ case TBLOCK_SUBABORT_PENDING:
+ case TBLOCK_SUBRESTART:
+ AbortSubTransaction();
+ CleanupSubTransaction();
+ AbortAnyTransaction();
+ break;
+ }
+ }
+
+
+ /*
* PreventTransactionChain
*
* This routine is to be called by statements that must not run inside
diff -cr cvs/src/backend/tcop/postgres.c cvs.build/src/backend/tcop/postgres.c
*** cvs/src/backend/tcop/postgres.c 2009-12-24 13:55:18.000000000 +0100
--- cvs.build/src/backend/tcop/postgres.c 2009-12-24 20:55:17.000000000 +0100
***************
*** 2637,2643 ****
if (!proc_exit_inprogress)
{
InterruptPending = true;
! QueryCancelPending = true;
/*
* If it's safe to interrupt, and we're waiting for a lock, service
--- 2637,2647 ----
if (!proc_exit_inprogress)
{
InterruptPending = true;
!
! if (DoingCommandRead)
! TransactionCancelPending = true;
! else
! QueryCancelPending = true;
/*
* If it's safe to interrupt, and we're waiting for a lock, service
***************
*** 2789,2794 ****
--- 2793,2821 ----
errmsg("canceling statement due to user request")));
}
}
+ if (TransactionCancelPending)
+ {
+ QueryCancelPending = false;
+ ImmediateInterruptOK = false; /* not idle anymore */
+
+ if (!IsTransactionOrTransactionBlock())
+ return;
+
+ if (IsAbortedTransactionBlockState())
+ return;
+
+ DisableNotifyInterrupt();
+ DisableCatchupInterrupt();
+
+ ereport(NOTICE,
+ (errcode(ERRCODE_QUERY_CANCELED),
+ errmsg("canceling transaction due to user request")));
+
+ AbortAnyTransaction();
+
+ set_ps_display("idle in transaction (aborted)", false);
+ pgstat_report_activity("<IDLE> in transaction (aborted)");
+ }
/* If we get here, do nothing (probably, QueryCancelPending was reset) */
}
diff -cr cvs/src/backend/utils/init/globals.c cvs.build/src/backend/utils/init/globals.c
*** cvs/src/backend/utils/init/globals.c 2009-12-09 11:24:42.000000000 +0100
--- cvs.build/src/backend/utils/init/globals.c 2009-12-24 20:55:17.000000000 +0100
***************
*** 27,32 ****
--- 27,33 ----
volatile bool InterruptPending = false;
volatile bool QueryCancelPending = false;
+ volatile bool TransactionCancelPending = false;
volatile bool ProcDiePending = false;
volatile bool ImmediateInterruptOK = false;
volatile uint32 InterruptHoldoffCount = 0;
diff -cr cvs/src/include/access/xact.h cvs.build/src/include/access/xact.h
*** cvs/src/include/access/xact.h 2009-12-24 13:55:28.000000000 +0100
--- cvs.build/src/include/access/xact.h 2009-12-24 20:55:17.000000000 +0100
***************
*** 204,209 ****
--- 204,210 ----
extern bool IsTransactionOrTransactionBlock(void);
extern char TransactionBlockStatusCode(void);
extern void AbortOutOfAnyTransaction(void);
+ extern void AbortAnyTransaction(void);
extern void PreventTransactionChain(bool isTopLevel, const char *stmtType);
extern void RequireTransactionChain(bool isTopLevel, const char *stmtType);
extern bool IsInTransactionChain(bool isTopLevel);
diff -cr cvs/src/include/miscadmin.h cvs.build/src/include/miscadmin.h
*** cvs/src/include/miscadmin.h 2009-12-24 13:55:28.000000000 +0100
--- cvs.build/src/include/miscadmin.h 2009-12-24 20:55:37.000000000 +0100
***************
*** 68,73 ****
--- 68,74 ----
/* these are marked volatile because they are set by signal handlers: */
extern PGDLLIMPORT volatile bool InterruptPending;
extern volatile bool QueryCancelPending;
+ extern volatile bool TransactionCancelPending;
extern volatile bool ProcDiePending;
/* these are marked volatile because they are examined by signal handlers: */
On Thu, 2009-12-24 at 21:38 +0100, Joachim Wieland wrote:
On Sun, Dec 6, 2009 at 4:23 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
We are using NOTICE, not NOTIFY, assuming that we use anything at all
(which I still regard as unnecessary). Please stop injecting confusion
into the discussion.Attached is a minimal POC patch that allows to cancel an idle
transaction with SIGINT. The HS patch also allows this in its current
form but as Simon points out the client gets out of sync with it.
Thanks for working on this.
The proposal is to send an additional NOTICE to the client and abort
all open transactions and subtransactions (this is what I got from the
previous discussion).
(Adding Kris into the discussion here.)
Would this work with JDBC driver and/or general protocol clients?
I had to write an additional function AbortAnyTransaction() which
aborts all transactions and subtransactions and leaves the transaction
in the aborted state, is there an existing function to do this?
AbortOutOfAnyTransaction()
We'd probably want to add a timeout for idle transactions also (which
is a wishlist item since quite some time) and could also offer user
functions like pg_cancel_idle_transaction(). Along this we might need
to add internal reasons like we do for SIGUSR1 because we are now
multiplexing different functionality onto the SIGINT signal. One might
want to cancel an idle transaction only and not a running query,
without keeping track of internal reasons one risks to cancel a
legitimate query if that backend has started to work on a query again.
Next project, not both at once.
--
Simon Riggs www.2ndQuadrant.com
On Tue, 29 Dec 2009, Simon Riggs wrote:
The proposal is to send an additional NOTICE to the client and abort
all open transactions and subtransactions (this is what I got from the
previous discussion).Would this work with JDBC driver and/or general protocol clients?
A Notice would be easy to overlook. The JDBC driver wraps that as a
SQLWarning which callers need to explicitly check for (and rarely do in my
experience). So when they run their next statement they'll get an error
saying that the current transaction is aborted, but they'll have no idea
why as the warning was silently eaten. I'd prefer the transaction
cancellation to come as an Error because that's what it really is.
The only downside I can see is that a client would get confused if:
1) Transaction starts.
2) Idle transaction is killed and error message is given.
3) Client issues rollback
4) Client gets error message from saying the transaction was cancelled.
Kris Jurka
On Wed, Dec 30, 2009 at 12:28 AM, Simon Riggs <simon@2ndquadrant.com> wrote:
I had to write an additional function AbortAnyTransaction() which
aborts all transactions and subtransactions and leaves the transaction
in the aborted state, is there an existing function to do this?AbortOutOfAnyTransaction()
But this would clean up completely and not leave the transaction in
the aborted state. Subsequent commands will be executed just fine
instead of being refused with the error message that the transaction
is already aborted... Right?
Joachim
On Wed, 2009-12-30 at 05:02 -0500, Kris Jurka wrote:
On Tue, 29 Dec 2009, Simon Riggs wrote:
The proposal is to send an additional NOTICE to the client and abort
all open transactions and subtransactions (this is what I got from the
previous discussion).Would this work with JDBC driver and/or general protocol clients?
A Notice would be easy to overlook. The JDBC driver wraps that as a
SQLWarning which callers need to explicitly check for (and rarely do in my
experience). So when they run their next statement they'll get an error
saying that the current transaction is aborted, but they'll have no idea
why as the warning was silently eaten. I'd prefer the transaction
cancellation to come as an Error because that's what it really is.
I'm not certain of all of these points, but here goes:
AFAIK, NOTICE was suggested because it can be sent at any time, whereas
ERRORs are only associated with statements.
http://developer.postgresql.org/pgdocs/postgres/protocol-flow.html#PROTOCOL-ASYNC
"It is possible for NoticeResponse messages to be generated due to
outside activity; for example, if the database administrator commands a
"fast" database shutdown, the backend will send a NoticeResponse
indicating this fact before closing the connection. Accordingly,
frontends should always be prepared to accept and display NoticeResponse
messages, even when the connection is nominally idle."
Can JDBC accept a NOTICE, yet throw an error? NOTICEs have a SQLState
field just like ERRORs do, so you should be able to special case that.
I understand that this will mean that we are enhancing the protocol for
this release, but I don't have a better suggestion.
The only downside I can see is that a client would get confused if:
1) Transaction starts.
2) Idle transaction is killed and error message is given.
3) Client issues rollback
4) Client gets error message from saying the transaction was cancelled.
Are you saying that the client should send rollback and that it should
generate no message?
--
Simon Riggs www.2ndQuadrant.com
On Wed, 2009-12-30 at 11:43 +0100, Joachim Wieland wrote:
On Wed, Dec 30, 2009 at 12:28 AM, Simon Riggs <simon@2ndquadrant.com> wrote:
I had to write an additional function AbortAnyTransaction() which
aborts all transactions and subtransactions and leaves the transaction
in the aborted state, is there an existing function to do this?AbortOutOfAnyTransaction()
But this would clean up completely and not leave the transaction in
the aborted state.
True
Subsequent commands will be executed just fine
instead of being refused with the error message that the transaction
is already aborted...
True, but it is a subsequent transaction, not the same one. (I've
checked).
--
Simon Riggs www.2ndQuadrant.com
Simon Riggs wrote:
On Wed, 2009-12-30 at 05:02 -0500, Kris Jurka wrote:
On Tue, 29 Dec 2009, Simon Riggs wrote:
The proposal is to send an additional NOTICE to the client and abort
all open transactions and subtransactions (this is what I got from the
previous discussion).Would this work with JDBC driver and/or general protocol clients?
A Notice would be easy to overlook. The JDBC driver wraps that as a
SQLWarning which callers need to explicitly check for (and rarely do in my
experience). So when they run their next statement they'll get an error
saying that the current transaction is aborted, but they'll have no idea
why as the warning was silently eaten. I'd prefer the transaction
cancellation to come as an Error because that's what it really is.I'm not certain of all of these points, but here goes:
AFAIK, NOTICE was suggested because it can be sent at any time, whereas
ERRORs are only associated with statements.http://developer.postgresql.org/pgdocs/postgres/protocol-flow.html#PROTOCOL-ASYNC
"It is possible for NoticeResponse messages to be generated due to
outside activity; for example, if the database administrator commands a
"fast" database shutdown, the backend will send a NoticeResponse
indicating this fact before closing the connection. Accordingly,
frontends should always be prepared to accept and display NoticeResponse
messages, even when the connection is nominally idle."
Could we send an asynchronous notification immediately when the
transaction is cancelled, but also change the error message you get in
the subsequent commands. Clients that ignore the async notification
would still see a proper error message at the ERROR.
Something like:
ERROR: current transaction was aborted because of conflict with
recovery, commands ignored until end of transaction block
instead of the usual "current transaction is aborted, commands ignored
until end of transaction block" message.
--
Heikki Linnakangas
EnterpriseDB http://www.enterprisedb.com
On Wed, 30 Dec 2009, Simon Riggs wrote:
http://developer.postgresql.org/pgdocs/postgres/protocol-flow.html#PROTOCOL-ASYNC
"It is possible for NoticeResponse messages to be generated due to
outside activity; for example, if the database administrator commands a
"fast" database shutdown, the backend will send a NoticeResponse
indicating this fact before closing the connection. Accordingly,
frontends should always be prepared to accept and display NoticeResponse
messages, even when the connection is nominally idle."
The problem is that frontends won't check the backend connection until
they've already been given the next command to execute at which point it's
too late. I think a lot of the discussion on this thread is wishful
thinking about when a frontend will see the message and what they'll do
with it. You would either need a multithreaded frontend that had some
type of callback mechanism for these notices, or you'd need to poll the
socket every so often to see if you'd received a notice. I don't think
that describes most applications or client libraries.
Can JDBC accept a NOTICE, yet throw an error? NOTICEs have a SQLState
field just like ERRORs do, so you should be able to special case that.
Yes, that's possible.
The only downside I can see is that a client would get confused if:
1) Transaction starts.
2) Idle transaction is killed and error message is given.
3) Client issues rollback
4) Client gets error message from saying the transaction was cancelled.Are you saying that the client should send rollback and that it should
generate no message?
No, I'm saying if for some business logic reason the client decided it
needed to rollback as it hadn't seen the error message yet.
Kris Jurka
On Wed, 30 Dec 2009, Heikki Linnakangas wrote:
Could we send an asynchronous notification immediately when the
transaction is cancelled, but also change the error message you get in
the subsequent commands. Clients that ignore the async notification
would still see a proper error message at the ERROR.
+1
Kris Jurka
On 30/12/2009 7:37 PM, Simon Riggs wrote:
Can JDBC accept a NOTICE, yet throw an error? NOTICEs have a SQLState
field just like ERRORs do, so you should be able to special case that.
The JDBC driver would have to throw when the app code next interacted
with the connection object anyway. It can't asynchronously throw an
exception. Since the next interaction that can throw SQLException() is
likely to be setup for or execution of a query, I'm not sure it makes
any difference to the JDBC user whether query cancellation is reported
as a NOTICE or an ERROR behind the scenes.
Since the proposed patch leaves cancelled transactions in the error
state, rather than closing them and leaving the connection clean and
idle, it doesn't matter much if a client doesn't understand or check for
the NOTICE. The app code will try to do work on the connection and that
work will fail because the transaction is aborted, resulting in a normal
SQLException reporting that the "current transaction is aborted ...".
JDBC-using code has to be prepared to handle exceptions at any point of
interaction with the JDBC driver anyway, and any code that isn't is
buggy. Consequently there's LOTS of buggy JDBC code out there :-( as
people often ignore exceptions thrown during operations they think
"can't fail". However, such buggy code is already broken by
pg_cancel_backend() and pg_terminate_backend(), and won't be broken any
more or differently by the proposed change, so I don't see a problem
with it.
Personally, I'd be happy to leave the JDBC driver as it was. It might be
kind of handy if I could getWarnings() on the connection object without
blocking so I could call it before I executed a statement on the
connection ... but that'd always introduce a race between transaction
cancellation/timeout and statement execution, so code must always be
prepared to handle timeout/cancellation related failure anyway.
As you say, the driver can special-case connection cancelled NOTICE
mesages as errors and throw them at next user interaction it wants. But
I'm not sure that's anything more than a kind of nice-to-have cosmetic
feature. If the JDBC driver handled the NOTICE and threw a more
informative SQLException to tell the app why the transaction was dead,
that'd be nice, but hardly vital. It'd want to preserve the notice as an
SQLWarning as well.
I understand that this will mean that we are enhancing the protocol for
this release, but I don't have a better suggestion.
Only in an extremely backward compatible way - and it's more of a
behavior change for the backend than a protocol change. Pg's backends
change behaviour a whole lot more than that in a typical release...
The only downside I can see is that a client would get confused if:
1) Transaction starts.
2) Idle transaction is killed and error message is given.
3) Client issues rollback
4) Client gets error message from saying the transaction was cancelled.
For JDBC users, "there is no transaction in progress" is only reported
as a SQLWarning via getWarnings(), so I'd be surprised if anything used
it for more than logging or debugging purposes.
Are you saying that the client should send rollback and that it should
generate no message?
--
Craig Ringer
Craig Ringer <craig@postnewspapers.com.au> wrote:
It might be kind of handy if I could getWarnings() on the
connection object without blocking so I could call it before I
executed a statement on the connection ... but that'd always
introduce a race between transaction cancellation/timeout and
statement execution, so code must always be prepared to handle
timeout/cancellation related failure anyway.
+1 (I think)
If I'm understanding this, it sounds to me like it would be most
appropriate for the NOTICE to generate a warning at the connection
level and for the next request to throw an exception in the format
suggested by Heikki -- which I think is what Craig is suggesting.
-Kevin
On Wed, 2009-12-30 at 14:15 +0200, Heikki Linnakangas wrote:
Simon Riggs wrote:
AFAIK, NOTICE was suggested because it can be sent at any time, whereas
ERRORs are only associated with statements.http://developer.postgresql.org/pgdocs/postgres/protocol-flow.html#PROTOCOL-ASYNC
"It is possible for NoticeResponse messages to be generated due to
outside activity; for example, if the database administrator commands a
"fast" database shutdown, the backend will send a NoticeResponse
indicating this fact before closing the connection. Accordingly,
frontends should always be prepared to accept and display NoticeResponse
messages, even when the connection is nominally idle."Could we send an asynchronous notification immediately when the
transaction is cancelled, but also change the error message you get in
the subsequent commands. Clients that ignore the async notification
would still see a proper error message at the ERROR.Something like:
ERROR: current transaction was aborted because of conflict with
recovery, commands ignored until end of transaction blockinstead of the usual "current transaction is aborted, commands ignored
until end of transaction block" message.
This is possible, yes.
I have an added complication, hinted at by Joachim, currently
investigating.
--
Simon Riggs www.2ndQuadrant.com
On Thu, 2009-12-24 at 21:38 +0100, Joachim Wieland wrote:
On Sun, Dec 6, 2009 at 4:23 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
We are using NOTICE, not NOTIFY, assuming that we use anything at all
(which I still regard as unnecessary). Please stop injecting confusion
into the discussion.Attached is a minimal POC patch that allows to cancel an idle
transaction with SIGINT. The HS patch also allows this in its current
form but as Simon points out the client gets out of sync with it.The proposal is to send an additional NOTICE to the client and abort
all open transactions and subtransactions (this is what I got from the
previous discussion).
This all works and I'm looking to post a reviewed patch soon.
I had to write an additional function AbortAnyTransaction() which
aborts all transactions and subtransactions and leaves the transaction
in the aborted state, is there an existing function to do this?
My use of AbortOutOfAnyTransaction() was what caused the
problem-I-couldn't-solve. It aborted too far, confusing clients.
Joachim's function does the right thing and leaves the transaction state
correctly, so that clients don't get confused.
Problem solved, thanks Joachim.
--
Simon Riggs www.2ndQuadrant.com
On Thu, 2009-12-31 at 11:10 +0000, Simon Riggs wrote:
On Thu, 2009-12-24 at 21:38 +0100, Joachim Wieland wrote:
On Sun, Dec 6, 2009 at 4:23 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
We are using NOTICE, not NOTIFY, assuming that we use anything at all
(which I still regard as unnecessary). Please stop injecting confusion
into the discussion.Attached is a minimal POC patch that allows to cancel an idle
transaction with SIGINT. The HS patch also allows this in its current
form but as Simon points out the client gets out of sync with it.The proposal is to send an additional NOTICE to the client and abort
all open transactions and subtransactions (this is what I got from the
previous discussion).This all works and I'm looking to post a reviewed patch soon.
Attached is the patch I intend to commit, barring objections.
This patch extends SIGINT to allow cancellation of transactions while
idle in both HS and normal mode. It also changes the standard message
reported on an idle transaction in aborted state to '<IDLE> in
transaction (aborted)', so that once aborted we keep the message even if
the user tries to issue further statements other than ROLLBACK or
COMMIT.
This also solves the bug reported by Kris Jurka.
Joachim, credit will be to you, so please re-check.
(Further changes pending on HS side, so not all issues resolved by this.
I intend to use this mechanism for HS cancellations when
CONFLICT_MODE_ERROR, and another mechanism for CONFLICT_MODE_FATAL.)
--
Simon Riggs www.2ndQuadrant.com
Attachments:
cancel_idle_in_transaction.v2.patchtext/x-patch; charset=UTF-8; name=cancel_idle_in_transaction.v2.patchDownload
*** a/doc/src/sgml/func.sgml
--- b/doc/src/sgml/func.sgml
***************
*** 12899,12905 **** SELECT set_config('log_statement_stats', 'off', false);
<literal><function>pg_cancel_backend</function>(<parameter>pid</parameter> <type>int</>)</literal>
</entry>
<entry><type>boolean</type></entry>
! <entry>Cancel a backend's current query</entry>
</row>
<row>
<entry>
--- 12899,12905 ----
<literal><function>pg_cancel_backend</function>(<parameter>pid</parameter> <type>int</>)</literal>
</entry>
<entry><type>boolean</type></entry>
! <entry>Cancel a backend's current query or abort an idle transaction</entry>
</row>
<row>
<entry>
*** a/doc/src/sgml/monitoring.sgml
--- b/doc/src/sgml/monitoring.sgml
***************
*** 55,60 **** postgres 965 0.0 1.1 6152 1512 pts/1 SN 13:17 0:00 postgres: stats c
--- 55,61 ----
postgres 998 0.0 2.3 6532 2992 pts/1 SN 13:18 0:00 postgres: tgl runbug 127.0.0.1 idle
postgres 1003 0.0 2.4 6532 3128 pts/1 SN 13:19 0:00 postgres: tgl regression [local] SELECT waiting
postgres 1016 0.1 2.4 6532 3080 pts/1 SN 13:19 0:00 postgres: tgl regression [local] idle in transaction
+ postgres 1066 0.1 2.4 6532 3080 pts/1 SN 13:19 0:00 postgres: tgl regression [local] idle in transaction (aborted)
</screen>
(The appropriate invocation of <command>ps</> varies across different
***************
*** 77,82 **** postgres: <replaceable>user</> <replaceable>database</> <replaceable>host</> <re
--- 78,84 ----
the life of the client connection, but the activity indicator changes.
The activity can be <literal>idle</> (i.e., waiting for a client command),
<literal>idle in transaction</> (waiting for client inside a <command>BEGIN</> block),
+ <literal>idle in transaction (aborted)</> (waiting for client to <command>ROLLBACK</>),
or a command type name such as <literal>SELECT</>. Also,
<literal>waiting</> is attached if the server process is presently waiting
on a lock held by another server process. In the above example we can infer
*** a/src/backend/access/transam/xact.c
--- b/src/backend/access/transam/xact.c
***************
*** 313,320 **** IsTransactionState(void)
/*
* IsAbortedTransactionBlockState
*
! * This returns true if we are currently running a query
! * within an aborted transaction block.
*/
bool
IsAbortedTransactionBlockState(void)
--- 313,319 ----
/*
* IsAbortedTransactionBlockState
*
! * This returns true if we are within an aborted transaction block.
*/
bool
IsAbortedTransactionBlockState(void)
***************
*** 2692,2697 **** AbortCurrentTransaction(void)
--- 2691,2737 ----
}
/*
+ * AbortTransactionAndAnySubtransactions
+ *
+ * Similar to AbortCurrentTransaction but if any subtransactions
+ * in progress we abort them and all of their parents. So this is
+ * used when the caller wishes to make the abort untrappable by the user.
+ */
+ void
+ AbortTransactionAndAnySubtransactions(void)
+ {
+ TransactionState s = CurrentTransactionState;
+
+ switch (s->blockState)
+ {
+ case TBLOCK_DEFAULT:
+ case TBLOCK_STARTED:
+ case TBLOCK_BEGIN:
+ case TBLOCK_INPROGRESS:
+ case TBLOCK_END:
+ case TBLOCK_ABORT:
+ case TBLOCK_SUBABORT:
+ case TBLOCK_ABORT_END:
+ case TBLOCK_ABORT_PENDING:
+ case TBLOCK_PREPARE:
+ case TBLOCK_SUBABORT_END:
+ case TBLOCK_SUBABORT_RESTART:
+ AbortCurrentTransaction();
+ break;
+
+ case TBLOCK_SUBINPROGRESS:
+ case TBLOCK_SUBBEGIN:
+ case TBLOCK_SUBEND:
+ case TBLOCK_SUBABORT_PENDING:
+ case TBLOCK_SUBRESTART:
+ AbortSubTransaction();
+ CleanupSubTransaction();
+ AbortTransactionAndAnySubtransactions();
+ break;
+ }
+ }
+
+ /*
* PreventTransactionChain
*
* This routine is to be called by statements that must not run inside
*** a/src/backend/tcop/postgres.c
--- b/src/backend/tcop/postgres.c
***************
*** 2637,2651 **** StatementCancelHandler(SIGNAL_ARGS)
if (!proc_exit_inprogress)
{
InterruptPending = true;
! QueryCancelPending = true;
/*
! * If it's safe to interrupt, and we're waiting for a lock, service
! * the interrupt immediately. No point in interrupting if we're
! * waiting for input, however.
*/
! if (InterruptHoldoffCount == 0 && CritSectionCount == 0 &&
! (DoingCommandRead || ImmediateInterruptOK))
{
/* bump holdoff count to make ProcessInterrupts() a no-op */
/* until we are done getting ready for it */
--- 2637,2653 ----
if (!proc_exit_inprogress)
{
InterruptPending = true;
!
! if (!DoingCommandRead)
! QueryCancelPending = true;
! else if (IsTransactionOrTransactionBlock())
! TransactionCancelPending = true;
/*
! * If it's safe to interrupt, service the interrupt immediately.
*/
! if (InterruptHoldoffCount == 0 && CritSectionCount == 0 && ImmediateInterruptOK &&
! (TransactionCancelPending || QueryCancelPending))
{
/* bump holdoff count to make ProcessInterrupts() a no-op */
/* until we are done getting ready for it */
***************
*** 2789,2794 **** ProcessInterrupts(void)
--- 2791,2820 ----
errmsg("canceling statement due to user request")));
}
}
+ if (TransactionCancelPending)
+ {
+ TransactionCancelPending = false;
+ ImmediateInterruptOK = false; /* not idle anymore */
+
+ if (IsAbortedTransactionBlockState())
+ return;
+
+ DisableNotifyInterrupt();
+ DisableCatchupInterrupt();
+
+ ereport(NOTICE,
+ (errcode(ERRCODE_QUERY_CANCELED),
+ errmsg("canceling idle transaction due to administrator request")));
+
+ AbortTransactionAndAnySubtransactions();
+
+ /*
+ * Set the display correctly now, rather than wait for client wait loop
+ * to cycle round and reset display at some later time.
+ */
+ set_ps_display("idle in transaction (aborted)", false);
+ pgstat_report_activity("<IDLE> in transaction (aborted)");
+ }
/* If we get here, do nothing (probably, QueryCancelPending was reset) */
}
***************
*** 3603,3609 **** PostgresMain(int argc, char *argv[], const char *username)
*/
if (send_ready_for_query)
{
! if (IsTransactionOrTransactionBlock())
{
set_ps_display("idle in transaction", false);
pgstat_report_activity("<IDLE> in transaction");
--- 3629,3640 ----
*/
if (send_ready_for_query)
{
! if (IsAbortedTransactionBlockState())
! {
! set_ps_display("idle in transaction (aborted)", false);
! pgstat_report_activity("<IDLE> in transaction (aborted)");
! }
! else if (IsTransactionOrTransactionBlock())
{
set_ps_display("idle in transaction", false);
pgstat_report_activity("<IDLE> in transaction");
*** a/src/backend/utils/init/globals.c
--- b/src/backend/utils/init/globals.c
***************
*** 27,32 **** ProtocolVersion FrontendProtocol;
--- 27,33 ----
volatile bool InterruptPending = false;
volatile bool QueryCancelPending = false;
+ volatile bool TransactionCancelPending = false;
volatile bool ProcDiePending = false;
volatile bool ImmediateInterruptOK = false;
volatile uint32 InterruptHoldoffCount = 0;
*** a/src/include/access/xact.h
--- b/src/include/access/xact.h
***************
*** 204,209 **** extern bool IsTransactionBlock(void);
--- 204,210 ----
extern bool IsTransactionOrTransactionBlock(void);
extern char TransactionBlockStatusCode(void);
extern void AbortOutOfAnyTransaction(void);
+ extern void AbortTransactionAndAnySubtransactions(void);
extern void PreventTransactionChain(bool isTopLevel, const char *stmtType);
extern void RequireTransactionChain(bool isTopLevel, const char *stmtType);
extern bool IsInTransactionChain(bool isTopLevel);
*** a/src/include/miscadmin.h
--- b/src/include/miscadmin.h
***************
*** 68,73 ****
--- 68,74 ----
/* these are marked volatile because they are set by signal handlers: */
extern PGDLLIMPORT volatile bool InterruptPending;
extern volatile bool QueryCancelPending;
+ extern volatile bool TransactionCancelPending;
extern volatile bool ProcDiePending;
/* these are marked volatile because they are examined by signal handlers: */
On Thu, Dec 31, 2009 at 3:03 PM, Simon Riggs <simon@2ndquadrant.com> wrote:
This patch extends SIGINT to allow cancellation of transactions while
idle in both HS and normal mode. It also changes the standard message
reported on an idle transaction in aborted state to '<IDLE> in
transaction (aborted)', so that once aborted we keep the message even if
the user tries to issue further statements other than ROLLBACK or
COMMIT.This also solves the bug reported by Kris Jurka.
Was the bug reported by Kris really only about lost synchronization or
was it about SIGINT now cancelling idle transactions which it did not
do previously?
I still think that we should have three transaction cancel modes, one
to cancel an idle transaction, another one to cancel a running query
and a third one that just cancels the transaction regardless of it
being idle or not. This last one is what you are implementing now, and
it is what HS wants to do. However I think that Kris only wants to
cancel a running query but not an idle transaction. And an
administrator who wants to cancel an idle transaction can never be
sure that the transaction that he checked which has just been idle is
still idle...
(Further changes pending on HS side, so not all issues resolved by this.
I intend to use this mechanism for HS cancellations when
CONFLICT_MODE_ERROR, and another mechanism for CONFLICT_MODE_FATAL.)
CONFLICT_MODE_FATAL is what you are planning to implement via SIGUSR1
multiplexing then?
Joachim
On Thu, 2009-12-31 at 15:41 +0100, Joachim Wieland wrote:
I still think that we should have three transaction cancel modes, one
to cancel an idle transaction, another one to cancel a running query
and a third one that just cancels the transaction regardless of it
being idle or not. This last one is what you are implementing now, and
it is what HS wants to do.
pg_cancel_backend() is currently conditional on whether a statement is
active or not, so should really be called pg_cancel_if_active(). What
people want is an unconditional way to stop a transaction. I don't see
the need for 3 modes (and that has nothing to do with HS).
--
Simon Riggs www.2ndQuadrant.com
Simon Riggs wrote:
Attached is the patch I intend to commit, barring objections.
This patch extends SIGINT to allow cancellation of transactions while
idle in both HS and normal mode.
How does AbortTransactionAndAnySubtransactions() differ from
AbortOutOfAnyTransaction()? Having followed the discussions on this
patch I think I know the answer: I'm guessing that
AbortTransactionAndAnySubtransactions() leaves the backend in aborted
state, while AbortOutOfAnyTransaction() leaves it in idle state. It
would be good to point that out more clearly in the comment above
AbortTransactionAndAnySubtransactions().
I agree with Joachim's comments upthread that we should have a different
function for forcefully aborting the whole transaction, and leave the
current pg_cancel_backend() behavior alone. In any case, the
documentation of pg_cancel_backend() or the new function needs to
explain what exactly it does.
I believe people liked the idea of giving a different ERROR message when
a transaction is canceled because of conflict with recovery. It would
also be good to give a different error message when an idle transaction
is canceled using query cancel. Otherwise you don't know what hit you,
especially if you're not paying attention to NOTICEs.
It also changes the standard message
reported on an idle transaction in aborted state to '<IDLE> in
transaction (aborted)', so that once aborted we keep the message even if
the user tries to issue further statements other than ROLLBACK or
COMMIT.
That's nice.
--
Heikki Linnakangas
EnterpriseDB http://www.enterprisedb.com
On Fri, 2010-01-01 at 12:53 +0200, Heikki Linnakangas wrote:
Simon Riggs wrote:
Attached is the patch I intend to commit, barring objections.
This patch extends SIGINT to allow cancellation of transactions while
idle in both HS and normal mode.How does AbortTransactionAndAnySubtransactions() differ from
AbortOutOfAnyTransaction()? Having followed the discussions on this
patch I think I know the answer: I'm guessing that
AbortTransactionAndAnySubtransactions() leaves the backend in aborted
state, while AbortOutOfAnyTransaction() leaves it in idle state. It
would be good to point that out more clearly in the comment above
AbortTransactionAndAnySubtransactions().
OK
I agree with Joachim's comments upthread that we should have a different
function for forcefully aborting the whole transaction, and leave the
current pg_cancel_backend() behavior alone.
That would require multiple behaviours. Joachim already proposed
multiplexing SIGINT and Tom disagreed, on the simultaneous thread "Hot
Standby introduced problem with query cancel behaviour". I agree that
there seems little point in multiplexing both signals.
So the only way to have multiple cancel behaviours is to do this
cancellation via SIGUSR1 options and have additional functions to
request them.
Which amounts to rejecting this patch, since *this* patch changes the
behaviour of SIGINT for all senders, which is what I understood people
desired, i.e. not just a change for Hot Standby. I assumed Joachim did
not mean to veto his own patch, but I'm not sure what you think here?
(I don't mind either way).
In any case, the
documentation of pg_cancel_backend() or the new function needs to
explain what exactly it does.
...The patch changes the docs for pg_cancel_backend().
I believe people liked the idea of giving a different ERROR message when
a transaction is canceled because of conflict with recovery. It would
also be good to give a different error message when an idle transaction
is canceled using query cancel. Otherwise you don't know what hit you,
especially if you're not paying attention to NOTICEs.
I did like that idea when I heard it, but it seemed to have a few
problems on implementation.
Currently we say
ERROR: current transaction is aborted, commands ignored until end of
transaction block
and we repeat this every time a new statement is sent other than COMMIT
or ROLLBACK.
We could either endlessly repeat this
ERROR: current transaction is aborted because of conflict with
recovery, commands ignored until end of transaction block
or just say it once and then revert to the normal message. Neither
seemed very useful.
I'm also not sure why we would want to single out Hot Standby to
generate the reason "because of conflict with recovery" when no other
ERROR source would generate such a reason.
It also changes the standard message
reported on an idle transaction in aborted state to '<IDLE> in
transaction (aborted)', so that once aborted we keep the message even if
the user tries to issue further statements other than ROLLBACK or
COMMIT.That's nice.
--
Simon Riggs www.2ndQuadrant.com
On Jan 1, 2010, at 6:48 AM, Simon Riggs <simon@2ndQuadrant.com> w
We could either endlessly repeat this
ERROR: current transaction is aborted because of conflict with
recovery, commands ignored until end of transaction block
+1 for this option.
I'm also not sure why we would want to single out Hot Standby to
generate the reason "because of conflict with recovery" when no other
ERROR source would generate such a reason.
Well, most times when the transaction is aborted, it's because you did
something wrong. Or at least, the failure is associated with some
particular statement.
If we have other events that can asynchronously roll back a
transaction, I would think they would deserve similar handling. Off
the top of my head, I'm not sure if there are any such cases.
...Robert
Simon Riggs wrote:
On Fri, 2010-01-01 at 12:53 +0200, Heikki Linnakangas wrote:
I agree with Joachim's comments upthread that we should have a different
function for forcefully aborting the whole transaction, and leave the
current pg_cancel_backend() behavior alone.That would require multiple behaviours. Joachim already proposed
multiplexing SIGINT and Tom disagreed, on the simultaneous thread "Hot
Standby introduced problem with query cancel behaviour". I agree that
there seems little point in multiplexing both signals.So the only way to have multiple cancel behaviours is to do this
cancellation via SIGUSR1 options and have additional functions to
request them.Which amounts to rejecting this patch, since *this* patch changes the
behaviour of SIGINT for all senders, which is what I understood people
desired, i.e. not just a change for Hot Standby. I assumed Joachim did
not mean to veto his own patch, but I'm not sure what you think here?
(I don't mind either way).
I don't know, I don't feel strongly about this. Is there really no other
way?
In any case, the
documentation of pg_cancel_backend() or the new function needs to
explain what exactly it does....The patch changes the docs for pg_cancel_backend().
I don't think that's enough. "Cancel a backend's current query or abort
an idle transaction" leaves a lot of questions. When does it abort the
transaction? The whole top-level transaction or just the current
subtransaction?
I believe people liked the idea of giving a different ERROR message when
a transaction is canceled because of conflict with recovery. It would
also be good to give a different error message when an idle transaction
is canceled using query cancel. Otherwise you don't know what hit you,
especially if you're not paying attention to NOTICEs.I did like that idea when I heard it, but it seemed to have a few
problems on implementation.Currently we say
ERROR: current transaction is aborted, commands ignored until end of
transaction blockand we repeat this every time a new statement is sent other than COMMIT
or ROLLBACK.We could either endlessly repeat this
ERROR: current transaction is aborted because of conflict with
recovery, commands ignored until end of transaction blockor just say it once and then revert to the normal message. Neither
seemed very useful.
Seems useful to me, so that you know why your transaction was cancelled.
It's rather weird to see no ERRORs in the previous steps, and suddenly
you see that the transaction is aborted. And none your savepoints exist
anymore either.
I'm also not sure why we would want to single out Hot Standby to
generate the reason "because of conflict with recovery" when no other
ERROR source would generate such a reason.
Because you don't get any other ERROR for it. If your transaction is
aborted because of e.g a unique key violation, you get the ERROR about
unique key violation first, and only the subsequent commands produce the
"current transaction is aborted" message. With hot standby conflicts,
the first and only symptom the client sees is that the subsequent
commands fail with "current transaction is aborted", so it would be nice
to know why.
--
Heikki Linnakangas
EnterpriseDB http://www.enterprisedb.com
On Fri, Jan 1, 2010 at 8:38 PM, Robert Haas <robertmhaas@gmail.com> wrote:
On Jan 1, 2010, at 6:48 AM, Simon Riggs <simon@2ndQuadrant.com> w
We could either endlessly repeat this
ERROR: current transaction is aborted because of conflict with
recovery, commands ignored until end of transaction block+1 for this option.
I'm also not sure why we would want to single out Hot Standby to
generate the reason "because of conflict with recovery" when no other
ERROR source would generate such a reason.Well, most times when the transaction is aborted, it's because you did
something wrong. Or at least, the failure is associated with some
particular statement.If we have other events that can asynchronously roll back a transaction, I
would think they would deserve similar handling. Off the top of my head,
I'm not sure if there are any such cases.
Why not do the finger pointing (to HS) in the DETAIL field of the ERROR, and
let the error message remain the same.
Best regards,
--
gurjeet.singh
@ EnterpriseDB - The Enterprise Postgres Company
http://www.enterprisedb.com
singh.gurjeet@{ gmail | yahoo }.com
Twitter/Skype: singh_gurjeet
Mail sent from my BlackLaptop device
On Fri, 2010-01-01 at 07:08 -0800, Robert Haas wrote:
On Jan 1, 2010, at 6:48 AM, Simon Riggs <simon@2ndQuadrant.com> w
We could either endlessly repeat this
ERROR: current transaction is aborted because of conflict with
recovery, commands ignored until end of transaction block+1 for this option.
I'm also not sure why we would want to single out Hot Standby to
generate the reason "because of conflict with recovery" when no other
ERROR source would generate such a reason.Well, most times when the transaction is aborted, it's because you did
something wrong. Or at least, the failure is associated with some
particular statement.If we have other events that can asynchronously roll back a
transaction, I would think they would deserve similar handling. Off
the top of my head, I'm not sure if there are any such cases.
Serialization failures, deadlocks, timeouts, SIGINT, out of memory
errors etc..
--
Simon Riggs www.2ndQuadrant.com
On Jan 1, 2010, at 8:30 AM, Simon Riggs <simon@2ndQuadrant.com> wrote:
On Fri, 2010-01-01 at 07:08 -0800, Robert Haas wrote:
On Jan 1, 2010, at 6:48 AM, Simon Riggs <simon@2ndQuadrant.com> w
We could either endlessly repeat this
ERROR: current transaction is aborted because of conflict with
recovery, commands ignored until end of transaction block+1 for this option.
I'm also not sure why we would want to single out Hot Standby to
generate the reason "because of conflict with recovery" when no
other
ERROR source would generate such a reason.Well, most times when the transaction is aborted, it's because you
did
something wrong. Or at least, the failure is associated with some
particular statement.If we have other events that can asynchronously roll back a
transaction, I would think they would deserve similar handling. Off
the top of my head, I'm not sure if there are any such cases.Serialization failures, deadlocks, timeouts, SIGINT, out of memory
errors etc..
Hmm. I don't think I can get a serialization failure, deadlock, or out
of memory error while my session is idle. An idle timeout or SIGINT is
analagous, I think.
...Robert
On Fri, 2010-01-01 at 17:14 +0200, Heikki Linnakangas wrote:
Which amounts to rejecting this patch, since *this* patch changes the
behaviour of SIGINT for all senders, which is what I understood people
desired, i.e. not just a change for Hot Standby. I assumed Joachim did
not mean to veto his own patch, but I'm not sure what you think here?
(I don't mind either way).I don't know, I don't feel strongly about this. Is there really no other
way?
Multiple behaviours on signal implies multiplexing, AFAICS.
Question on the table is: Should SIGINT be extended to cancel an
idle-in-transaction session, or not? I'll wait a little while longer
before committing this to make sure I have the full spread of opinion.
Tom's opinion was...
On Tue, 2009-12-29 at 10:22 -0500, Tom Lane wrote:
Joachim Wieland <joe@mcknight.de> writes:
If we use the same signal for both cases, the receiving backend cannot
tell what the intention of the sending backend was. That's why I
proposed to make SIGINT similar to SIGUSR1 where we write a reason to
a shared memory structure first and then send the signal (see
http://archives.postgresql.org/pgsql-hackers/2009-12/msg02067.php from
a few days ago).This seems like a fairly bad idea. One of the intended use-cases is to
be able to manually "kill -INT" a misbehaving backend. Assuming that
there will be valid info about the signal in shared memory will break
that.
So it seems that we have at least one vote in favour of making SIGINT
blow anything away, no matter what its state.
I support that also, but I don't need it for HS, its just an objective
opinion. So that's plus 2, unsure about Joachim. Any others?
Seems useful to me, so that you know why your transaction was cancelled.
It's rather weird to see no ERRORs in the previous steps, and suddenly
you see that the transaction is aborted. And none your savepoints exist
anymore either.
I agree we need a message to explain, it just seems wrong to me to do
this in a way that appears to accentuate this particular source of error
over similar sources.
However, I will do as requested, though will leave existing error
sources alone.
--
Simon Riggs www.2ndQuadrant.com
On Fri, 2010-01-01 at 09:24 -0800, Robert Haas wrote:
If we have other events that can asynchronously roll back a
transaction, I would think they would deserve similar handling. Off
the top of my head, I'm not sure if there are any such cases.Serialization failures, deadlocks, timeouts, SIGINT, out of memory
errors etc..Hmm. I don't think I can get a serialization failure, deadlock, or out
of memory error while my session is idle.
Agreed. As a point of note, now that we can cancel idle transactions
there isn't any future blocker from making serialization failures or
deadlocks cancel such transactions... Other RDBMS have deadlock
detectors that can pick any session to resolve, not just the one doing
the deadlock checking.
An idle timeout or SIGINT is analagous, I think.
--
Simon Riggs www.2ndQuadrant.com
On Jan 1, 2010, at 9:42 AM, Simon Riggs <simon@2ndQuadrant.com> wrote:
On Fri, 2010-01-01 at 09:24 -0800, Robert Haas wrote:
If we have other events that can asynchronously roll back a
transaction, I would think they would deserve similar handling.
Off
the top of my head, I'm not sure if there are any such cases.Serialization failures, deadlocks, timeouts, SIGINT, out of memory
errors etc..Hmm. I don't think I can get a serialization failure, deadlock, or
out
of memory error while my session is idle.Agreed. As a point of note, now that we can cancel idle transactions
there isn't any future blocker from making serialization failures or
deadlocks cancel such transactions... Other RDBMS have deadlock
detectors that can pick any session to resolve, not just the one doing
the deadlock checking.
Interesting. It's not obvious to me how killing an *idle* session can
resolve a deadlock. AIUI a deadlock requires a cycle in the waits-for
graph, and an idle transaction is not waiting for a lock acquisition.
I can see how it could be useful in handling serialization failures,
though, and there may be other applications as well. This is a nice
improvement; I'm pleased to see it going in.
...Robert
On Fri, 2010-01-01 at 10:15 -0800, Robert Haas wrote:
Hmm. I don't think I can get a serialization failure, deadlock, or
out
of memory error while my session is idle.Agreed. As a point of note, now that we can cancel idle transactions
there isn't any future blocker from making serialization failures or
deadlocks cancel such transactions... Other RDBMS have deadlock
detectors that can pick any session to resolve, not just the one doing
the deadlock checking.Interesting. It's not obvious to me how killing an *idle* session can
resolve a deadlock. AIUI a deadlock requires a cycle in the waits-for
graph, and an idle transaction is not waiting for a lock acquisition.
In strict theory, yes.
In practice, many lock contention situations are caused by long running
idle transactions, so having a deadlock detector be able to resolve a
situation by deciding that an idle xact is actually in some kind of wait
state would be very useful.
Some people have asked for a idle-in-transaction-timeout. I would be
more inclined to have a settable time after which an idle-in-transaction
session that blocks an active lock requestor can be aborted by the
deadlock detector as a way of resolving a lock wait. Idle-in-transaction
sessions that don't hold any locks aren't the same kind of annoyance,
though there may be other reasons to remove them.
--
Simon Riggs www.2ndQuadrant.com
On Thu, 31 Dec 2009, Simon Riggs wrote:
On Thu, 2009-12-31 at 15:41 +0100, Joachim Wieland wrote:
I still think that we should have three transaction cancel modes, one
to cancel an idle transaction, another one to cancel a running query
and a third one that just cancels the transaction regardless of it
being idle or not. This last one is what you are implementing now, and
it is what HS wants to do.pg_cancel_backend() is currently conditional on whether a statement is
active or not, so should really be called pg_cancel_if_active(). What
people want is an unconditional way to stop a transaction. I don't see
the need for 3 modes (and that has nothing to do with HS).
The JDBC driver does want "cancel if active" behavior. The JDBC API
specifies Statement.cancel() where Statement is running one particular
backend query. So it really does want to cancel just that one query.
Already this is tough because of the asynchronous nature of the cancel
protocol and the inability to say exactly what should be cancelled.
Kris Jurka
On Fri, 2010-01-01 at 15:31 -0500, Kris Jurka wrote:
On Thu, 31 Dec 2009, Simon Riggs wrote:
On Thu, 2009-12-31 at 15:41 +0100, Joachim Wieland wrote:
I still think that we should have three transaction cancel modes, one
to cancel an idle transaction, another one to cancel a running query
and a third one that just cancels the transaction regardless of it
being idle or not. This last one is what you are implementing now, and
it is what HS wants to do.pg_cancel_backend() is currently conditional on whether a statement is
active or not, so should really be called pg_cancel_if_active(). What
people want is an unconditional way to stop a transaction. I don't see
the need for 3 modes (and that has nothing to do with HS).The JDBC driver does want "cancel if active" behavior. The JDBC API
specifies Statement.cancel() where Statement is running one particular
backend query. So it really does want to cancel just that one query.
Already this is tough because of the asynchronous nature of the cancel
protocol and the inability to say exactly what should be cancelled.
OK, I think that is conclusive.
CancelRequest's behaviour currently equates to SIGINT, so
processCancelRequest() can only use SIGINT if SIGINT's behaviour remains
same.
I would recommend we make SIGINT do cancel-anything, and handle
everything else via SIGUSR1, including CancelRequest. I'm not going to
do that; I'm going to make HS conflict resolution work, which means
putting in enough infrastructure to allow someone else to make SIGINT
changes work at a later time, if appropriate.
--
Simon Riggs www.2ndQuadrant.com
On Fri, Jan 1, 2010 at 10:45 PM, Simon Riggs <simon@2ndquadrant.com> wrote:
I would recommend we make SIGINT do cancel-anything, and handle
everything else via SIGUSR1, including CancelRequest. I'm not going to
do that; I'm going to make HS conflict resolution work, which means
putting in enough infrastructure to allow someone else to make SIGINT
changes work at a later time, if appropriate.
If this is the final consent then please go ahead with HS and I will
see if I can take care of the rest.
Joachim
Hi,
Simon Riggs wrote:
In practice, many lock contention situations are caused by long running
idle transactions, so having a deadlock detector be able to resolve a
situation by deciding that an idle xact is actually in some kind of wait
state would be very useful.
Hm.. so you'd abort the transaction that's been idle the longest? Is
that really the one you want to abort in every case?
We currently abort the one which is checking for deadlocks, right?
That's a pretty random pick, then. And randomization might have benefits
here (namely giving all kinds of transaction, whether interactive or
automated, the same chance of surviving a deadlock). I'm not sure
whether or not this is a good or required thing, though.
Allow me to also point out the related requirement of several
replication solutions to gather information about such a deadlock or
maybe even control the choice of which transaction to abort. See
http://wiki.postgresql.org/wiki/ClusterFeatures#A_standard_way_to_get_global_deadlock_information
Some people have asked for a idle-in-transaction-timeout. I would be
more inclined to have a settable time after which an idle-in-transaction
session that blocks an active lock requestor can be aborted by the
deadlock detector as a way of resolving a lock wait. Idle-in-transaction
sessions that don't hold any locks aren't the same kind of annoyance,
though there may be other reasons to remove them.
Aha, yes I see. That sounds more controllable (and should probably
default to no timeout).
Regards
Markus Wanner
On Fri, Jan 01, 2010 at 03:31:58PM -0500, Kris Jurka wrote:
The JDBC driver does want "cancel if active" behavior. The JDBC API
specifies Statement.cancel() where Statement is running one particular
backend query. So it really does want to cancel just that one query.
Already this is tough because of the asynchronous nature of the cancel
protocol and the inability to say exactly what should be cancelled.
I've looked in the JDBC documentation but I don't quickly see how they
expect this to work with transactions. What is being proposed seems to
me to be:
If statement active:
put transaction in aborted state
If no statement active:
do nothing
However, I see that the documentation wants to be able to abort a
*specific* statement, which is not being proposed here. Can that be
implemented on top of the current proposal?
Have a nice day,
--
Martijn van Oosterhout <kleptog@svana.org> http://svana.org/kleptog/
Show quoted text
Please line up in a tree and maintain the heap invariant while
boarding. Thank you for flying nlogn airlines.
On Fri, Jan 1, 2010 at 1:39 PM, Simon Riggs <simon@2ndquadrant.com> wrote:
Interesting. It's not obvious to me how killing an *idle* session can
resolve a deadlock. AIUI a deadlock requires a cycle in the waits-for
graph, and an idle transaction is not waiting for a lock acquisition.In strict theory, yes.
In practice, many lock contention situations are caused by long running
idle transactions, so having a deadlock detector be able to resolve a
situation by deciding that an idle xact is actually in some kind of wait
state would be very useful.Some people have asked for a idle-in-transaction-timeout. I would be
more inclined to have a settable time after which an idle-in-transaction
session that blocks an active lock requestor can be aborted by the
deadlock detector as a way of resolving a lock wait. Idle-in-transaction
sessions that don't hold any locks aren't the same kind of annoyance,
though there may be other reasons to remove them.
I think the biggest issue with idle-in-transaction sessions is MVCC
bloat, which has been considerably mitigated in 8.4 (shout-out to
Alvaro). It could still be an issue for serializable transactions,
though. So I'm not 100% sure what is most useful down the road, but
it seems you've solved the immediate problem here, which is good.
...Robert
On Fri, Jan 1, 2010 at 10:45 PM, Simon Riggs <simon@2ndquadrant.com> wrote:
CancelRequest's behaviour currently equates to SIGINT, so
processCancelRequest() can only use SIGINT if SIGINT's behaviour remains
same.I would recommend we make SIGINT do cancel-anything, and handle
everything else via SIGUSR1, including CancelRequest.
Actually, now that I look into it, if we wanted to send SIGUSR1 with a
reason to a backend from within postmaster (where
processCancelRequest() lives), we'd need to have shared memory access
in postmaster which we have not.
So the easiest way would be to keep SIGINTs behavior (cancel running
statements, not idle transactions) and allow cancellation of idle
transactions only via SQL but not via command line.
Other ideas?
Joachim
On Wed, Jan 6, 2010 at 4:37 PM, Joachim Wieland <joe@mcknight.de> wrote:
On Fri, Jan 1, 2010 at 10:45 PM, Simon Riggs <simon@2ndquadrant.com> wrote:
CancelRequest's behaviour currently equates to SIGINT, so
processCancelRequest() can only use SIGINT if SIGINT's behaviour remains
same.I would recommend we make SIGINT do cancel-anything, and handle
everything else via SIGUSR1, including CancelRequest.Actually, now that I look into it, if we wanted to send SIGUSR1 with a
reason to a backend from within postmaster (where
processCancelRequest() lives), we'd need to have shared memory access
in postmaster which we have not.So the easiest way would be to keep SIGINTs behavior (cancel running
statements, not idle transactions) and allow cancellation of idle
transactions only via SQL but not via command line.
+1. That seems like the right approach to me.
...Robert
On Sun, 2010-01-03 at 11:55 +0100, Martijn van Oosterhout wrote:
On Fri, Jan 01, 2010 at 03:31:58PM -0500, Kris Jurka wrote:
The JDBC driver does want "cancel if active" behavior. The JDBC API
specifies Statement.cancel() where Statement is running one particular
backend query. So it really does want to cancel just that one query.
Already this is tough because of the asynchronous nature of the cancel
protocol and the inability to say exactly what should be cancelled.I've looked in the JDBC documentation but I don't quickly see how they
expect this to work with transactions. What is being proposed seems to
me to be:If statement active:
put transaction in aborted state
If no statement active:
do nothingHowever, I see that the documentation wants to be able to abort a
*specific* statement, which is not being proposed here. Can that be
implemented on top of the current proposal?
That would require Statement-level abort, which we don't have.
--
Simon Riggs www.2ndQuadrant.com