Re: [HACKERS] cancelling statement due to user request error occurs but the transaction has committed.
Bruce Momjian br...@momjian.us writes: On Thu, Mar 19, 2015 at 04:36:38PM -0400, Robert Haas wrote: So, either way, what happens if the query cancel shows up just an instant after you clear the flag? Oh, good point. This version handles that case addressing only the log_duration* block. This is just moving the failure cases around, and not by very much either. The core issue here, I think, is that xact.c only holds off interrupts during what it considers to be the commit critical section --- which is okay from the standpoint of transactional consistency. But the complaint has to do with what the client perceives to have happened if a SIGINT arrives somewhere between where xact.c has committed and where postgres.c has reported the commit to the client. If we want to address that, I think postgres.c needs to hold off interrupts for the entire duration from just before CommitTransactionCommand() to just after ReadyForQuery(). That's likely to be rather messy, because there are so many code paths there, especially when you consider error cases. A possible way to do this without incurring unacceptably high risk of breakage (in particular, ending up with interrupts still held off when they shouldn't be any longer) is to assume that there should never be a case where we reach ReadCommand() with interrupts still held off. Then we could invent an additional interrupt primitive RESET_INTERRUPTS() that forces InterruptHoldoffCount to zero (and, presumably, then does a CHECK_FOR_INTERRUPTS()); then putting a HOLD_INTERRUPTS() before calling CommitTransactionCommand() and a RESET_INTERRUPTS() before waiting for client input would presumably be pretty safe. On the other hand, that approach could easily mask interrupt holdoff mismatch bugs elsewhere in the code base. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] cancelling statement due to user request error occurs but the transaction has committed.
On Thu, Mar 19, 2015 at 08:43:52PM -0400, Tom Lane wrote: Bruce Momjian br...@momjian.us writes: On Thu, Mar 19, 2015 at 06:59:20PM -0300, Alvaro Herrera wrote: I don't understand why aren't interrupts held until after the commit is done -- including across the mentioned ereports. Uh, I think Robert was thinking of pre-commit triggers at the top of CommitTransaction() that might take a long time and we might want to cancel. Yeah, that's a good point. So really the only way to make this work as requested is to have some cooperation between xact.c and postgres.c, so that the hold taken midway through CommitTransaction is kept until we reach the idle point. The attached is only very lightly tested but shows what we probably would need for this. It's a bit messy in that the API for CommitTransactionCommand leaves it unspecified whether interrupts are held at exit; I'm not sure if it's useful or feasible to be more precise. Oh, I see what you are saying, and why a global variable will not work. I thought all paths reset the cancel state when the returned from a commit, but it seems there are many places that don't do reset, so you have to pass a flag down into CommitTransaction() to control that --- makes sense. -- Bruce Momjian br...@momjian.ushttp://momjian.us EnterpriseDB http://enterprisedb.com + Everyone has their own god. + -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] cancelling statement due to user request error occurs but the transaction has committed.
On Thu, Mar 19, 2015 at 04:36:38PM -0400, Robert Haas wrote:
On Thu, Mar 19, 2015 at 10:23 AM, Bruce Momjian br...@momjian.us wrote:
First attached patch is more surgical and clears a possible cancel
request before we report the query duration in the logs --- this doesn't
affect any after triggers that might include CHECK_FOR_INTERRUPTS()
calls we want to honor.
Another approach would be to have CommitTransaction() clear any pending
cancel before it calls RESUME_INTERRUPTS(). The second attached patch
takes that approach, and also works.
So, either way, what happens if the query cancel shows up just an
instant after you clear the flag?
Oh, good point. This version handles that case addressing only the
log_duration* block.
--
Bruce Momjian br...@momjian.ushttp://momjian.us
EnterpriseDB http://enterprisedb.com
+ Everyone has their own god. +
diff --git a/src/backend/tcop/postgres.c b/src/backend/tcop/postgres.c
new file mode 100644
index 33720e8..4374fb4
*** a/src/backend/tcop/postgres.c
--- b/src/backend/tcop/postgres.c
*** exec_simple_query(const char *query_stri
*** 1165,1184
/*
* Emit duration logging if appropriate.
*/
- switch (check_log_duration(msec_str, was_logged))
{
! case 1:
! ereport(LOG,
! (errmsg(duration: %s ms, msec_str),
! errhidestmt(true)));
! break;
! case 2:
! ereport(LOG,
! (errmsg(duration: %s ms statement: %s,
! msec_str, query_string),
! errhidestmt(true),
! errdetail_execute(parsetree_list)));
! break;
}
if (save_log_statement_stats)
--- 1165,1193
/*
* Emit duration logging if appropriate.
*/
{
! int output_duration_level = check_log_duration(msec_str, was_logged);
!
! if (output_duration_level != 0)
! {
! /* hold client cancel as we have already committed */
! HOLD_CANCEL_INTERRUPTS();
!
! if (output_duration_level == 1)
! ereport(LOG,
! (errmsg(duration: %s ms, msec_str),
! errhidestmt(true)));
! else if (output_duration_level == 2)
! ereport(LOG,
! (errmsg(duration: %s ms statement: %s,
! msec_str, query_string),
! errhidestmt(true),
! errdetail_execute(parsetree_list)));
!
! /* clear client cancel */
! QueryCancelPending = false;
! RESUME_CANCEL_INTERRUPTS();
! }
}
if (save_log_statement_stats)
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] cancelling statement due to user request error occurs but the transaction has committed.
On Thu, Mar 19, 2015 at 07:19:02PM -0400, Tom Lane wrote: The core issue here, I think, is that xact.c only holds off interrupts during what it considers to be the commit critical section --- which is okay from the standpoint of transactional consistency. But the complaint has to do with what the client perceives to have happened if a SIGINT arrives somewhere between where xact.c has committed and where postgres.c has reported the commit to the client. If we want to address that, I think postgres.c needs to hold off interrupts for the entire duration from just before CommitTransactionCommand() to just after ReadyForQuery(). That's likely to be rather messy, because there are so many code paths there, especially when you consider error cases. A possible way to do this without incurring unacceptably high risk of breakage (in particular, ending up with interrupts still held off when they shouldn't be any longer) is to assume that there should never be a case where we reach ReadCommand() with interrupts still held off. Then we could invent an additional interrupt primitive RESET_INTERRUPTS() that forces InterruptHoldoffCount to zero (and, presumably, then does a CHECK_FOR_INTERRUPTS()); then putting a HOLD_INTERRUPTS() before calling CommitTransactionCommand() and a RESET_INTERRUPTS() before waiting for client input would presumably be pretty safe. On the other hand, that approach could easily mask interrupt holdoff mismatch bugs elsewhere in the code base. Well, right now we allow interrupts for as long as possible, i.e. to the middle of CommitTransaction(). Your approach would block interrupts for a larger span, which might be worse than the bug we are fixing. It also feels like it would be unmodular as functions would change the blocking state when they are called. Tom is right that my cancel5.diff version has an area between the first cancel erase and the second cancel erase where a cancel might arrive. I assumed there were no checks in that area, but I might be wrong, and there could be checks there someday. The fundamental problem is that the place we need to block cancels starts several levels down in a function, and the place we need to clear it is higher. Maybe the entire HOLD/RESUME block API we have for this is wrong and we just need a global variable to ignore client cancels to be read inside the signal handler, and we just set and clear it. I can work on a patch if people like that idea. -- Bruce Momjian br...@momjian.ushttp://momjian.us EnterpriseDB http://enterprisedb.com + Everyone has their own god. + -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] cancelling statement due to user request error occurs but the transaction has committed.
On Thu, Mar 19, 2015 at 06:59:20PM -0300, Alvaro Herrera wrote:
Robert Haas wrote:
On Thu, Mar 19, 2015 at 10:23 AM, Bruce Momjian br...@momjian.us wrote:
The issue with CommitTransaction() is that it only _holds_ the signal
--- it doesn't clear it. Now, since there are very few
CHECK_FOR_INTERRUPTS() calls in the typical commit process flow, the
signal is normally erased. However, if log_duration or
log_min_duration_statement are set, they call ereport, which calls
errfinish(), which has a call to CHECK_FOR_INTERRUPTS().
First attached patch is more surgical and clears a possible cancel
request before we report the query duration in the logs --- this doesn't
affect any after triggers that might include CHECK_FOR_INTERRUPTS()
calls we want to honor.
Another approach would be to have CommitTransaction() clear any pending
cancel before it calls RESUME_INTERRUPTS(). The second attached patch
takes that approach, and also works.
So, either way, what happens if the query cancel shows up just an
instant after you clear the flag?
I don't understand why aren't interrupts held until after the commit is
done -- including across the mentioned ereports.
Uh, I think Robert was thinking of pre-commit triggers at the top of
CommitTransaction() that might take a long time and we might want to
cancel. In fact, he is right that mid-way into CommitTransaction(),
after those pre-commit triggers, we do HOLD_INTERRUPTS(), then set our
clog bit and continue to the bottom of that function. What is happening
is that we don't _clear_ the cancel bit and log_duration is finding the
cancel.
In an ideal world, we would clear the client cancel in
CommitTransaction() and when we do log_duration*, and the attached patch
now does that.
--
Bruce Momjian br...@momjian.ushttp://momjian.us
EnterpriseDB http://enterprisedb.com
+ Everyone has their own god. +
diff --git a/src/backend/access/transam/xact.c b/src/backend/access/transam/xact.c
new file mode 100644
index 1495bb4..853671f
*** a/src/backend/access/transam/xact.c
--- b/src/backend/access/transam/xact.c
*** CommitTransaction(void)
*** 1958,1963
--- 1958,1966
*/
s-state = TRANS_DEFAULT;
+ /* We have committed so clear any client cancel. */
+ QueryCancelPending = false;
+
RESUME_INTERRUPTS();
}
diff --git a/src/backend/tcop/postgres.c b/src/backend/tcop/postgres.c
new file mode 100644
index 33720e8..b797cad
*** a/src/backend/tcop/postgres.c
--- b/src/backend/tcop/postgres.c
*** exec_simple_query(const char *query_stri
*** 1165,1184
/*
* Emit duration logging if appropriate.
*/
- switch (check_log_duration(msec_str, was_logged))
{
! case 1:
! ereport(LOG,
! (errmsg(duration: %s ms, msec_str),
! errhidestmt(true)));
! break;
! case 2:
! ereport(LOG,
! (errmsg(duration: %s ms statement: %s,
! msec_str, query_string),
! errhidestmt(true),
! errdetail_execute(parsetree_list)));
! break;
}
if (save_log_statement_stats)
--- 1165,1194
/*
* Emit duration logging if appropriate.
*/
{
! int output_duration_level = check_log_duration(msec_str, was_logged);
!
! if (output_duration_level != 0)
! {
! /* hold client cancel as we have already committed */
! HOLD_CANCEL_INTERRUPTS();
!
! if (output_duration_level == 1)
! ereport(LOG,
! (errmsg(duration: %s ms, msec_str),
! errhidestmt(true)));
! else if (output_duration_level == 2)
! ereport(LOG,
! (errmsg(duration: %s ms statement: %s,
! msec_str, query_string),
! errhidestmt(true),
! errdetail_execute(parsetree_list)));
!
! /* clear client cancel */
! QueryCancelPending = false;
!
! RESUME_CANCEL_INTERRUPTS();
! }
}
if (save_log_statement_stats)
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] cancelling statement due to user request error occurs but the transaction has committed.
On Wed, Mar 18, 2015 at 10:56 PM, Bruce Momjian br...@momjian.us wrote: I have researched this issue originally reported in June of 2014 and implemented a patch to ignore cancel while we are completing a commit. I am not clear if this is the proper place for this code, though a disable_timeout() call on the line above suggests I am close. :-) This would also disable cancel interrupts while running AFTER triggers, which seems almost certain to be wrong. TBH, I'm not sure why the existing HOLD_INTERRUPTS() in CommitTransaction() isn't already preventing this problem. Did you investigate that at all? -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] cancelling statement due to user request error occurs but the transaction has committed.
On Thu, Mar 19, 2015 at 07:54:02AM -0400, Robert Haas wrote: On Wed, Mar 18, 2015 at 10:56 PM, Bruce Momjian br...@momjian.us wrote: I have researched this issue originally reported in June of 2014 and implemented a patch to ignore cancel while we are completing a commit. I am not clear if this is the proper place for this code, though a disable_timeout() call on the line above suggests I am close. :-) This would also disable cancel interrupts while running AFTER triggers, which seems almost certain to be wrong. TBH, I'm not sure why the existing HOLD_INTERRUPTS() in CommitTransaction() isn't already preventing this problem. Did you investigate that at all? Yes, the situation is complex, and was suggested by the original poster. The issue with CommitTransaction() is that it only _holds_ the signal --- it doesn't clear it. Now, since there are very few CHECK_FOR_INTERRUPTS() calls in the typical commit process flow, the signal is normally erased. However, if log_duration or log_min_duration_statement are set, they call ereport, which calls errfinish(), which has a call to CHECK_FOR_INTERRUPTS(). First attached patch is more surgical and clears a possible cancel request before we report the query duration in the logs --- this doesn't affect any after triggers that might include CHECK_FOR_INTERRUPTS() calls we want to honor. Another approach would be to have CommitTransaction() clear any pending cancel before it calls RESUME_INTERRUPTS(). The second attached patch takes that approach, and also works. -- Bruce Momjian br...@momjian.ushttp://momjian.us EnterpriseDB http://enterprisedb.com + Everyone has their own god. + diff --git a/src/backend/tcop/postgres.c b/src/backend/tcop/postgres.c new file mode 100644 index 33720e8..9521c48 *** a/src/backend/tcop/postgres.c --- b/src/backend/tcop/postgres.c *** exec_simple_query(const char *query_stri *** 1163,1168 --- 1163,1174 NullCommand(dest); /* + * We have already committed, so clear any cancel requests + * that might be processed by the ereport() calls below. + */ + QueryCancelPending = false; + + /* * Emit duration logging if appropriate. */ switch (check_log_duration(msec_str, was_logged)) diff --git a/src/backend/access/transam/xact.c b/src/backend/access/transam/xact.c new file mode 100644 index 1495bb4..9b6da95 *** a/src/backend/access/transam/xact.c --- b/src/backend/access/transam/xact.c *** CommitTransaction(void) *** 1958,1963 --- 1958,1969 */ s-state = TRANS_DEFAULT; + /* + * We have already committed, so clear any cancel requests + * that might be processed later. + */ + QueryCancelPending = false; + RESUME_INTERRUPTS(); } -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] cancelling statement due to user request error occurs but the transaction has committed.
On Wed, Sep 10, 2014 at 08:10:45PM -0400, Bruce Momjian wrote:
On Tue, Jun 10, 2014 at 10:30:24AM -0400, Robert Haas wrote:
On Tue, Jun 10, 2014 at 10:18 AM, Tom Lane t...@sss.pgh.pa.us wrote:
Robert Haas robertmh...@gmail.com writes:
I don't agree with this analysis. If the connection is closed after
the client sends a COMMIT and before it gets a response, then the
client must indeed be smart enough to figure out whether or not the
commit happened. But if the server sends a response, the client
should be able to rely on that response being correct. In this case,
an ERROR is getting sent but the transaction is getting committed;
yuck. I'm not sure whether the fix is right, but this definitely
seems like a bug.
In general, the only way to avoid that sort of behavior for a post-commit
error would be to PANIC ... and even then, the transaction got committed,
which might not be the expectation of a client that got an error message,
even if it said PANIC. So this whole area is a minefield, and the only
attractive thing we can do is to try to reduce the number of errors that
can get thrown post-commit. We already, for example, do not treat
post-commit file unlink failures as ERROR, though we surely would prefer
to do that.
We could treated it as a lost-communication scenario. The appropriate
recovery actions from the client's point of view are identical.
So from this standpoint, redefining SIGINT as not throwing an error when
we're in post-commit seems like a good idea. I'm not endorsing any
details of the patch here, but the 2-foot view seems generally sound.
Cool, that makes sense to me also.
Did we ever do anything about this?
I have researched this issue originally reported in June of 2014 and
implemented a patch to ignore cancel while we are completing a commit.
I am not clear if this is the proper place for this code, though a
disable_timeout() call on the line above suggests I am close. :-)
(The disable_timeout disables internal timeouts, but it doesn't disable
cancels coming from the client.)
The first patch is for testing and adds a sleep(5) to the end of the
TRUNCATE command, to give the tester time to press Control-C from psql,
and enables log_duration so the cancel is checked.
The second patch is the patch that disables cancel when we are in the
process of committing; before:
test= CREATE TABLE test(x INT);
CREATE TABLE
test= INSERT INTO test VALUES (3);
INSERT 0 1
test= TRUNCATE test;
^CCancel request sent
-- ERROR: canceling statement due to user request
test= SELECT * FROM test;
x
---
(0 rows)
and with both patches:
test= CREATE TABLE test(x INT);
CREATE TABLE
test= INSERT INTO test VALUES (3);
INSERT 0 1
test= TRUNCATE test;
^CCancel request sent
-- TRUNCATE TABLE
test= SELECT * FROM test;
x
---
(0 rows)
--
Bruce Momjian br...@momjian.ushttp://momjian.us
EnterpriseDB http://enterprisedb.com
+ Everyone has their own god. +
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
new file mode 100644
index 623e6bf..a5d66d8
*** a/src/backend/commands/tablecmds.c
--- b/src/backend/commands/tablecmds.c
***
*** 13,18
--- 13,19
*-
*/
#include postgres.h
+ #include unistd.h
#include access/genam.h
#include access/heapam.h
*** ExecuteTruncate(TruncateStmt *stmt)
*** 1265,1270
--- 1266,1272
heap_close(rel, NoLock);
}
+ sleep(5);
}
/*
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
new file mode 100644
index 26275bd..9147a79
*** a/src/backend/utils/misc/guc.c
--- b/src/backend/utils/misc/guc.c
*** extern const struct config_enum_entry dy
*** 408,414
/*
* GUC option variables that are exported from this module
*/
! bool log_duration = false;
bool Debug_print_plan = false;
bool Debug_print_parse = false;
bool Debug_print_rewritten = false;
--- 408,414
/*
* GUC option variables that are exported from this module
*/
! bool log_duration = true;
bool Debug_print_plan = false;
bool Debug_print_parse = false;
bool Debug_print_rewritten = false;
*** static struct config_bool ConfigureNames
*** 1082,1088
NULL
},
log_duration,
! false,
NULL, NULL, NULL
},
{
--- 1082,1088
NULL
},
log_duration,
! true,
NULL, NULL, NULL
},
{
diff --git a/src/backend/utils/misc/postgresql.conf.sample b/src/backend/utils/misc/postgresql.conf.sample
new file mode 100644
index 110983f..82eca10
*** a/src/backend/utils/misc/postgresql.conf.sample
--- b/src/backend/utils/misc/postgresql.conf.sample
Re: [HACKERS] cancelling statement due to user request error occurs but the transaction has committed.
On Tue, Jun 10, 2014 at 10:30:24AM -0400, Robert Haas wrote: On Tue, Jun 10, 2014 at 10:18 AM, Tom Lane t...@sss.pgh.pa.us wrote: Robert Haas robertmh...@gmail.com writes: I don't agree with this analysis. If the connection is closed after the client sends a COMMIT and before it gets a response, then the client must indeed be smart enough to figure out whether or not the commit happened. But if the server sends a response, the client should be able to rely on that response being correct. In this case, an ERROR is getting sent but the transaction is getting committed; yuck. I'm not sure whether the fix is right, but this definitely seems like a bug. In general, the only way to avoid that sort of behavior for a post-commit error would be to PANIC ... and even then, the transaction got committed, which might not be the expectation of a client that got an error message, even if it said PANIC. So this whole area is a minefield, and the only attractive thing we can do is to try to reduce the number of errors that can get thrown post-commit. We already, for example, do not treat post-commit file unlink failures as ERROR, though we surely would prefer to do that. We could treated it as a lost-communication scenario. The appropriate recovery actions from the client's point of view are identical. So from this standpoint, redefining SIGINT as not throwing an error when we're in post-commit seems like a good idea. I'm not endorsing any details of the patch here, but the 2-foot view seems generally sound. Cool, that makes sense to me also. Did we ever do anything about this? -- Bruce Momjian br...@momjian.ushttp://momjian.us EnterpriseDB http://enterprisedb.com + Everyone has their own god. + -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] cancelling statement due to user request error occurs but the transaction has committed.
Hi, Well, the only other principled fix I can see is to add a new reponse along the lines of ERRORBUTITCOMMITTED, which does not seem attractive either, since all clients will have to be taught to understand it. +1 I think current specification hard to understand for many users. It is really good if PostgreSQL gave us a message such as a replication abort warning: ### WARNING: canceling wait for synchronous replication due to user request DETAIL: The transaction has already committed locally, but might not have been replicated to the standby. ### Regards, Naoya --- Naoya Anzai Engineering Department NEC Solution Inovetors, Ltd. E-Mail: anzai-na...@mxu.nes.nec.co.jp --- -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] cancelling statement due to user request error occurs but the transaction has committed.
On Sun, Jun 8, 2014 at 2:52 AM, Amit Kapila amit.kapil...@gmail.com wrote: I think this cancellation request must not interrupt the internal commited transaction. This is because clients may misunderstand the transaction has rollbacked. There can be similar observation if the server goes off (power outage or anything like) after committing transaction, client will receive connection broken, so he can misunderstand that as well. I think for such corner cases, client needs to reconfirm his action results with database before concluding anything. I don't agree with this analysis. If the connection is closed after the client sends a COMMIT and before it gets a response, then the client must indeed be smart enough to figure out whether or not the commit happened. But if the server sends a response, the client should be able to rely on that response being correct. In this case, an ERROR is getting sent but the transaction is getting committed; yuck. I'm not sure whether the fix is right, but this definitely seems like a bug. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] cancelling statement due to user request error occurs but the transaction has committed.
Robert Haas robertmh...@gmail.com writes: I don't agree with this analysis. If the connection is closed after the client sends a COMMIT and before it gets a response, then the client must indeed be smart enough to figure out whether or not the commit happened. But if the server sends a response, the client should be able to rely on that response being correct. In this case, an ERROR is getting sent but the transaction is getting committed; yuck. I'm not sure whether the fix is right, but this definitely seems like a bug. In general, the only way to avoid that sort of behavior for a post-commit error would be to PANIC ... and even then, the transaction got committed, which might not be the expectation of a client that got an error message, even if it said PANIC. So this whole area is a minefield, and the only attractive thing we can do is to try to reduce the number of errors that can get thrown post-commit. We already, for example, do not treat post-commit file unlink failures as ERROR, though we surely would prefer to do that. So from this standpoint, redefining SIGINT as not throwing an error when we're in post-commit seems like a good idea. I'm not endorsing any details of the patch here, but the 2-foot view seems generally sound. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] cancelling statement due to user request error occurs but the transaction has committed.
Robert Haas robertmh...@gmail.com wrote: If the connection is closed after the client sends a COMMIT and before it gets a response, then the client must indeed be smart enough to figure out whether or not the commit happened. But if the server sends a response, the client should be able to rely on that response being correct. In this case, an ERROR is getting sent but the transaction is getting committed; yuck. I'm not sure whether the fix is right, but this definitely seems like a bug. +1 It is one thing to send a request and experience a crash or loss of connection before a response is delivered. You have to consider that the state of the transaction is indeterminate and needs to be checked. But if the client receives a response saying that the commit was successful, or that the transaction was rolled back, that had better reflect reality; otherwise it is a clear bug. -- Kevin Grittner EDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] cancelling statement due to user request error occurs but the transaction has committed.
On Tue, Jun 10, 2014 at 10:18 AM, Tom Lane t...@sss.pgh.pa.us wrote: Robert Haas robertmh...@gmail.com writes: I don't agree with this analysis. If the connection is closed after the client sends a COMMIT and before it gets a response, then the client must indeed be smart enough to figure out whether or not the commit happened. But if the server sends a response, the client should be able to rely on that response being correct. In this case, an ERROR is getting sent but the transaction is getting committed; yuck. I'm not sure whether the fix is right, but this definitely seems like a bug. In general, the only way to avoid that sort of behavior for a post-commit error would be to PANIC ... and even then, the transaction got committed, which might not be the expectation of a client that got an error message, even if it said PANIC. So this whole area is a minefield, and the only attractive thing we can do is to try to reduce the number of errors that can get thrown post-commit. We already, for example, do not treat post-commit file unlink failures as ERROR, though we surely would prefer to do that. We could treated it as a lost-communication scenario. The appropriate recovery actions from the client's point of view are identical. So from this standpoint, redefining SIGINT as not throwing an error when we're in post-commit seems like a good idea. I'm not endorsing any details of the patch here, but the 2-foot view seems generally sound. Cool, that makes sense to me also. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] cancelling statement due to user request error occurs but the transaction has committed.
Robert Haas robertmh...@gmail.com writes: On Tue, Jun 10, 2014 at 10:18 AM, Tom Lane t...@sss.pgh.pa.us wrote: ... So this whole area is a minefield, and the only attractive thing we can do is to try to reduce the number of errors that can get thrown post-commit. We already, for example, do not treat post-commit file unlink failures as ERROR, though we surely would prefer to do that. We could treated it as a lost-communication scenario. The appropriate recovery actions from the client's point of view are identical. I'd hardly rate that as an attractive option. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] cancelling statement due to user request error occurs but the transaction has committed.
On Tue, Jun 10, 2014 at 10:42 AM, Tom Lane t...@sss.pgh.pa.us wrote: Robert Haas robertmh...@gmail.com writes: On Tue, Jun 10, 2014 at 10:18 AM, Tom Lane t...@sss.pgh.pa.us wrote: ... So this whole area is a minefield, and the only attractive thing we can do is to try to reduce the number of errors that can get thrown post-commit. We already, for example, do not treat post-commit file unlink failures as ERROR, though we surely would prefer to do that. We could treated it as a lost-communication scenario. The appropriate recovery actions from the client's point of view are identical. I'd hardly rate that as an attractive option. Well, the only other principled fix I can see is to add a new reponse along the lines of ERRORBUTITCOMMITTED, which does not seem attractive either, since all clients will have to be taught to understand it. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] cancelling statement due to user request error occurs but the transaction has committed.
Hi Amit, Thank you for your response. There can be similar observation if the server goes off (power outage or anything like) after committing transaction, client will receive connection broken, so he can misunderstand that as well. I think for such corner cases, client needs to reconfirm his action results with database before concluding anything. I see. Now, I understand that ProcessInterrupts Error (ProcDie, QueryCancel, ClientLost..) does not mean That query has been RollBacked. Regards, Naoya --- Naoya Anzai Engineering Department NEC Solution Inovetors, Ltd. E-Mail: anzai-na...@mxu.nes.nec.co.jp --- -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] cancelling statement due to user request error occurs but the transaction has committed.
On Fri, Jun 6, 2014 at 2:11 PM, Naoya Anzai anzai-na...@mxu.nes.nec.co.jp wrote: Hi All, When log_duration is true ( or log_min_duration_statement=0 ), If a transaction has internally been commited receives a SIGINT signal then a query cancellation error is output. For example, 1. A query like a TRUNCATE is removing bigger table files. 2. The session receives SIGINT signal. 3. Query cancellation error occurs. 4. But the query has commited. naoya=# truncate hoge; Cancel request sent ERROR: canceling statement due to user request naoya=# select count(*) from hoge; count --- 0 (1 row) --- This is because ProcessInterrupts function is called by errfinish ( in query-duration ereport). I think this cancellation request must not interrupt the internal commited transaction. This is because clients may misunderstand the transaction has rollbacked. There can be similar observation if the server goes off (power outage or anything like) after committing transaction, client will receive connection broken, so he can misunderstand that as well. I think for such corner cases, client needs to reconfirm his action results with database before concluding anything. With Regards, Amit Kapila. EnterpriseDB: http://www.enterprisedb.com
[HACKERS] cancelling statement due to user request error occurs but the transaction has committed.
Hi All, When log_duration is true ( or log_min_duration_statement=0 ), If a transaction has internally been commited receives a SIGINT signal then a query cancellation error is output. For example, 1. A query like a TRUNCATE is removing bigger table files. 2. The session receives SIGINT signal. 3. Query cancellation error occurs. 4. But the query has commited. e.g.) --- naoya=# \d List of relations Schema | Name | Type | Owner +--+---+--- public | hoge | table | naoya (1 row) naoya=# set log_duration=on; SET naoya=# select count(*) from hoge; count 10 (1 row) naoya=# truncate hoge; Cancel request sent ERROR: canceling statement due to user request naoya=# select count(*) from hoge; count --- 0 (1 row) --- This is because ProcessInterrupts function is called by errfinish ( in query-duration ereport). I think this cancellation request must not interrupt the internal commited transaction. This is because clients may misunderstand the transaction has rollbacked. Now, I tried to fix the problem. --- postgresql-fe7337f/src/backend/utils/error/elog.c 2014-06-06 11:57:44.0 +0900 +++ postgresql-fe7337f.new/src/backend/utils/error/elog.c 2014-06-06 13:10:51.0 +0900 @@ -580,7 +580,8 @@ * can stop a query emitting tons of notice or warning messages, even if * it's in a loop that otherwise fails to check for interrupts. */ - CHECK_FOR_INTERRUPTS(); + if (IsTransactionState()) + CHECK_FOR_INTERRUPTS(); } Thereby, When ereport(non error level) calls and not in-transaction state, PostgreSQL never calls ProcessInterrupts function by errfinish. But I have a anxiety to fix errfinish function because errfinish is called in many many situations.. Could you please confirm it? Regards, Naoya --- Naoya Anzai Engineering Department NEC Solution Inovetors, Ltd. E-Mail: anzai-na...@mxu.nes.nec.co.jp --- postgresql-fe7337f_elog.patch Description: postgresql-fe7337f_elog.patch -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
