On 1/27/18 22:45, Andres Freund wrote:
> I think the comment was bad, but the functionality pretty crucial. So I
> don't think
> In AtAbort_Portals(), remove the code that marks an active portal as
> failed. As the comment there already predicted, this doesn't work if
> the running command wants to keep running after transaction abort. And
> it's actually not necessary, because pquery.c is careful to run all
> portal code in a PG_TRY block and explicitly runs MarkPortalFailed() if
> there is an exception. So the code in AtAbort_Portals() is never used
> anyway.
> holds true, because FATAL errors do not follow the sigsetjmp chain.
>
> I'm uncomfortable with the idea, but without further study, it does seem
> like you might be able to largely rescue the idea by checking
> proc_exit_in_progress or similar.
Here is a patch to implement that idea. Do you have a way to test it
repeatedly, or do you just randomly cancel queries?
--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
From ef275dd88fe1533fa48aeab0774439bc4743acce Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <pete...@gmx.net>
Date: Thu, 1 Feb 2018 17:07:38 -0500
Subject: [PATCH] Fix crash when canceling parallel query
elog(FATAL) would end up calling PortalCleanup(), which would call
executor shutdown code, which could fail and crash, especially under
parallel query. This was introduced by
8561e4840c81f7e345be2df170839846814fa004, which did not want to mark an
active portal as failed by a normal transaction abort anymore. But we
do need to do that for an elog(FATAL) exit. Introduce a variable
shmem_exit_inprogress similar to the existing proc_exit_inprogress, so
we can tell whether we are in the FATAL exit scenario.
---
src/backend/storage/ipc/ipc.c | 7 +++++++
src/backend/utils/mmgr/portalmem.c | 8 ++++++++
src/include/storage/ipc.h | 1 +
3 files changed, 16 insertions(+)
diff --git a/src/backend/storage/ipc/ipc.c b/src/backend/storage/ipc/ipc.c
index 2de35efbd4..726db7b7f1 100644
--- a/src/backend/storage/ipc/ipc.c
+++ b/src/backend/storage/ipc/ipc.c
@@ -39,6 +39,11 @@
*/
bool proc_exit_inprogress = false;
+/*
+ * Set when shmem_exit() is in progress.
+ */
+bool shmem_exit_inprogress = false;
+
/*
* This flag tracks whether we've called atexit() in the current process
* (or in the parent postmaster).
@@ -214,6 +219,8 @@ proc_exit_prepare(int code)
void
shmem_exit(int code)
{
+ shmem_exit_inprogress = true;
+
/*
* Call before_shmem_exit callbacks.
*
diff --git a/src/backend/utils/mmgr/portalmem.c
b/src/backend/utils/mmgr/portalmem.c
index f3f0add1d6..75a6dde32b 100644
--- a/src/backend/utils/mmgr/portalmem.c
+++ b/src/backend/utils/mmgr/portalmem.c
@@ -22,6 +22,7 @@
#include "catalog/pg_type.h"
#include "commands/portalcmds.h"
#include "miscadmin.h"
+#include "storage/ipc.h"
#include "utils/builtins.h"
#include "utils/memutils.h"
#include "utils/snapmgr.h"
@@ -757,6 +758,13 @@ AtAbort_Portals(void)
{
Portal portal = hentry->portal;
+ /*
+ * When elog(FATAL) is progress, we need to set the active
portal to
+ * failed, so that PortalCleanup() doesn't run the executor
shutdown.
+ */
+ if (portal->status == PORTAL_ACTIVE && shmem_exit_inprogress)
+ MarkPortalFailed(portal);
+
/*
* Do nothing else to cursors held over from a previous
transaction.
*/
diff --git a/src/include/storage/ipc.h b/src/include/storage/ipc.h
index e934a83a1c..6a05a89349 100644
--- a/src/include/storage/ipc.h
+++ b/src/include/storage/ipc.h
@@ -63,6 +63,7 @@ typedef void (*shmem_startup_hook_type) (void);
/* ipc.c */
extern PGDLLIMPORT bool proc_exit_inprogress;
+extern PGDLLIMPORT bool shmem_exit_inprogress;
extern void proc_exit(int code) pg_attribute_noreturn();
extern void shmem_exit(int code);
--
2.16.1
