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 <[email protected]> wrote:
> > > Robert Haas <[email protected]> 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 20000-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 <[email protected]> http://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
***************
*** 414,420 ****
#log_checkpoints = off
#log_connections = off
#log_disconnections = off
! #log_duration = off
#log_error_verbosity = default # terse, default, or verbose messages
#log_hostname = off
#log_line_prefix = '' # special values:
--- 414,420 ----
#log_checkpoints = off
#log_connections = off
#log_disconnections = off
! #log_duration = on
#log_error_verbosity = default # terse, default, or verbose messages
#log_hostname = off
#log_line_prefix = '' # special values:
diff --git a/src/backend/tcop/postgres.c b/src/backend/tcop/postgres.c
new file mode 100644
index 33720e8..e606602
*** a/src/backend/tcop/postgres.c
--- b/src/backend/tcop/postgres.c
*************** finish_xact_command(void)
*** 2460,2465 ****
--- 2460,2468 ----
/* Cancel any active statement timeout before committing */
disable_timeout(STATEMENT_TIMEOUT, false);
+ /* ignore cancel coming from the client */
+ HOLD_CANCEL_INTERRUPTS();
+
/* Now commit the command */
ereport(DEBUG3,
(errmsg_internal("CommitTransactionCommand")));
*************** finish_xact_command(void)
*** 2477,2482 ****
--- 2480,2488 ----
MemoryContextStats(TopMemoryContext);
#endif
+ QueryCancelPending = false;
+ RESUME_CANCEL_INTERRUPTS();
+
xact_started = false;
}
}
--
Sent via pgsql-hackers mailing list ([email protected])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
