On Fri, Sep 4, 2015 at 6:51 AM, Tom Lane <t...@sss.pgh.pa.us> wrote:
> I experimented with the attached patch. It seems to work to stop the
> crash Michael exhibited (I've not tried to duplicate Jim's original
> example, though). However, it also causes a regression test failure,
> because transactions.sql does this:
>
Neat patch, it would have taken me longer to figure out that... I have
tried with Jim's test case and the patch is working.
> which is exactly the case we're trying to reject now. So that fills
> me with fear that this approach would break existing applications.
> On the other hand, I do not really see a good alternative :-(.
>
This behavior is present since 2004 with fe548629, so that's a real concern
to me, especially if there are drivers around relying on this behavior.
There are for example some code patterns around Postgres ODBC that could be
impacted, not sure which ones but I guess that some internal error handling
is not going to like that.
I thought about trying to detect whether the Portal actually had any
> references to "new in subtransaction" objects to decide whether to
> kill it or not, but that seems awfully fragile.
>
> Ideas?
>
Yes. This diff on top of your patch:
@@ -922,8 +922,7 @@ AtSubAbort_Portals(SubTransactionId mySubid,
* must be forced into FAILED state, for
the same reasons
* discussed below.
*/
- if (portal->status == PORTAL_READY ||
- portal->status == PORTAL_ACTIVE)
+ if (portal->status == PORTAL_ACTIVE)
MarkPortalFailed(portal);
This way only the active portals are marked as failed. The regression tests
that are failing with your patch applied visibly do not activate the portal
they use, just marking it as ready to be used. This seems to be the safest
approach to me on stable branches, as well as on master, this way we are
sure that resources on the failed subxacts are cleaned up correctly, and
that existing applications are not impacted.
I am attaching a new patch with what I changed and a test case based on my
previous one.
Regards,
--
Michael
diff --git a/src/backend/commands/portalcmds.c b/src/backend/commands/portalcmds.c
index 2794537..327b2a5 100644
--- a/src/backend/commands/portalcmds.c
+++ b/src/backend/commands/portalcmds.c
@@ -335,11 +335,7 @@ PersistHoldablePortal(Portal portal)
/*
* Check for improper portal use, and mark portal active.
*/
- if (portal->status != PORTAL_READY)
- ereport(ERROR,
- (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
- errmsg("portal \"%s\" cannot be run", portal->name)));
- portal->status = PORTAL_ACTIVE;
+ MarkPortalActive(portal);
/*
* Set up global portal context pointers.
diff --git a/src/backend/tcop/pquery.c b/src/backend/tcop/pquery.c
index 9c14e8a..0df86a2 100644
--- a/src/backend/tcop/pquery.c
+++ b/src/backend/tcop/pquery.c
@@ -734,11 +734,7 @@ PortalRun(Portal portal, long count, bool isTopLevel,
/*
* Check for improper portal use, and mark portal active.
*/
- if (portal->status != PORTAL_READY)
- ereport(ERROR,
- (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
- errmsg("portal \"%s\" cannot be run", portal->name)));
- portal->status = PORTAL_ACTIVE;
+ MarkPortalActive(portal);
/*
* Set up global portal context pointers.
@@ -1398,11 +1394,7 @@ PortalRunFetch(Portal portal,
/*
* Check for improper portal use, and mark portal active.
*/
- if (portal->status != PORTAL_READY)
- ereport(ERROR,
- (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
- errmsg("portal \"%s\" cannot be run", portal->name)));
- portal->status = PORTAL_ACTIVE;
+ MarkPortalActive(portal);
/*
* Set up global portal context pointers.
diff --git a/src/backend/utils/mmgr/portalmem.c b/src/backend/utils/mmgr/portalmem.c
index 7530498..639c2b3 100644
--- a/src/backend/utils/mmgr/portalmem.c
+++ b/src/backend/utils/mmgr/portalmem.c
@@ -232,6 +232,7 @@ CreatePortal(const char *name, bool allowDup, bool dupSilent)
portal->status = PORTAL_NEW;
portal->cleanup = PortalCleanup;
portal->createSubid = GetCurrentSubTransactionId();
+ portal->activeSubid = portal->createSubid;
portal->strategy = PORTAL_MULTI_QUERY;
portal->cursorOptions = CURSOR_OPT_NO_SCROLL;
portal->atStart = true;
@@ -403,6 +404,25 @@ UnpinPortal(Portal portal)
}
/*
+ * MarkPortalActive
+ * Transition a portal from READY to ACTIVE state.
+ *
+ * NOTE: never set portal->status = PORTAL_ACTIVE directly; call this instead.
+ */
+void
+MarkPortalActive(Portal portal)
+{
+ /* For safety, this is a runtime test not just an Assert */
+ if (portal->status != PORTAL_READY)
+ ereport(ERROR,
+ (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
+ errmsg("portal \"%s\" cannot be run", portal->name)));
+ /* Perform the state transition */
+ portal->status = PORTAL_ACTIVE;
+ portal->activeSubid = GetCurrentSubTransactionId();
+}
+
+/*
* MarkPortalDone
* Transition a portal from ACTIVE to DONE state.
*
@@ -690,6 +710,7 @@ PreCommit_Portals(bool isPrepare)
* not belonging to this transaction.
*/
portal->createSubid = InvalidSubTransactionId;
+ portal->activeSubid = InvalidSubTransactionId;
/* Report we changed state */
result = true;
@@ -836,8 +857,8 @@ AtCleanup_Portals(void)
/*
* Pre-subcommit processing for portals.
*
- * Reassign the portals created in the current subtransaction to the parent
- * subtransaction.
+ * Reassign portals created or used in the current subtransaction to the
+ * parent subtransaction.
*/
void
AtSubCommit_Portals(SubTransactionId mySubid,
@@ -859,14 +880,16 @@ AtSubCommit_Portals(SubTransactionId mySubid,
if (portal->resowner)
ResourceOwnerNewParent(portal->resowner, parentXactOwner);
}
+ if (portal->activeSubid == mySubid)
+ portal->activeSubid = parentSubid;
}
}
/*
* Subtransaction abort handling for portals.
*
- * Deactivate portals created during the failed subtransaction.
- * Note that per AtSubCommit_Portals, this will catch portals created
+ * Deactivate portals created or used during the failed subtransaction.
+ * Note that per AtSubCommit_Portals, this will catch portals created/used
* in descendants of the subtransaction too.
*
* We don't destroy any portals here; that's done in AtSubCleanup_Portals.
@@ -885,16 +908,48 @@ AtSubAbort_Portals(SubTransactionId mySubid,
{
Portal portal = hentry->portal;
+ /* Was it created in this subtransaction? */
if (portal->createSubid != mySubid)
+ {
+ /* No, but maybe it was used in this subtransaction? */
+ if (portal->activeSubid == mySubid)
+ {
+ /* Maintain activeSubid until the portal is removed */
+ portal->activeSubid = parentSubid;
+
+ /*
+ * Upper-level active portals that were used in this subxact
+ * must be forced into FAILED state, for the same reasons
+ * discussed below.
+ */
+ if (portal->status == PORTAL_ACTIVE)
+ MarkPortalFailed(portal);
+
+ /*
+ * Also, if we failed it during the current subxact (either
+ * just above, or earlier), reattach its resowner to the
+ * current subtransaction's resowner so that its resources get
+ * cleaned up while we abort this subtransaction. This is
+ * needed to avoid getting Assert failures or worse when we
+ * clean up objects created in the subtransaction.
+ */
+ if (portal->status == PORTAL_FAILED &&
+ portal->resowner)
+ {
+ ResourceOwnerNewParent(portal->resowner,
+ CurrentResourceOwner);
+ portal->resowner = NULL;
+ }
+ }
+ /* If it wasn't used in this subxact, nothing more to do */
continue;
+ }
/*
* Force any live portals of my own subtransaction into FAILED state.
* We have to do this because they might refer to objects created or
* changed in the failed subtransaction, leading to crashes if
* execution is resumed, or even if we just try to run ExecutorEnd.
- * (Note we do NOT do this to upper-level portals, since they cannot
- * have such references and hence may be able to continue.)
*/
if (portal->status == PORTAL_READY ||
portal->status == PORTAL_ACTIVE)
diff --git a/src/include/utils/portal.h b/src/include/utils/portal.h
index 1267b9e..7215e46 100644
--- a/src/include/utils/portal.h
+++ b/src/include/utils/portal.h
@@ -119,12 +119,18 @@ typedef struct PortalData
MemoryContext heap; /* subsidiary memory for portal */
ResourceOwner resowner; /* resources owned by portal */
void (*cleanup) (Portal portal); /* cleanup hook */
- SubTransactionId createSubid; /* the ID of the creating subxact */
/*
- * if createSubid is InvalidSubTransactionId, the portal is held over from
- * a previous transaction
+ * State data for remembering which subtransaction(s) the portal was
+ * created or used in. If the portal is held over from a previous
+ * transaction, both subxids are InvalidSubTransactionId. Otherwise,
+ * createSubid is the creating subxact and activeSubid is the last subxact
+ * in which we ran the portal. We must deactivate the portal if either of
+ * these subxacts becomes aborted, since it might hold references to
+ * objects created in said subxact.
*/
+ SubTransactionId createSubid; /* the creating subxact */
+ SubTransactionId activeSubid; /* the last subxact with activity */
/* The query or queries the portal will execute */
const char *sourceText; /* text of query (as of 8.4, never NULL) */
@@ -207,6 +213,7 @@ extern Portal CreatePortal(const char *name, bool allowDup, bool dupSilent);
extern Portal CreateNewPortal(void);
extern void PinPortal(Portal portal);
extern void UnpinPortal(Portal portal);
+extern void MarkPortalActive(Portal portal);
extern void MarkPortalDone(Portal portal);
extern void MarkPortalFailed(Portal portal);
extern void PortalDrop(Portal portal, bool isTopCommit);
diff --git a/src/test/regress/expected/transactions.out b/src/test/regress/expected/transactions.out
index 5d70863..42f4c78 100644
--- a/src/test/regress/expected/transactions.out
+++ b/src/test/regress/expected/transactions.out
@@ -566,6 +566,24 @@ insert into revalidate_bug values (1);
insert into revalidate_bug values (inverse(0));
drop table revalidate_bug;
drop function inverse(int);
+-- test case for relations referenced in active cursors
+CREATE OR REPLACE FUNCTION create_self_tab() RETURNS text
+LANGUAGE plpgsql AS $$
+BEGIN
+ CREATE TEMP TABLE new_table ON COMMIT DROP AS SELECT create_self_tab();
+ RETURN 'foo';
+END $$;
+BEGIN;
+DECLARE h CURSOR FOR SELECT create_self_tab();
+SAVEPOINT self_tab_point;
+FETCH h; -- error
+ERROR: relation "new_table" already exists
+CONTEXT: SQL statement "CREATE TEMP TABLE new_table ON COMMIT DROP AS SELECT create_self_tab()"
+PL/pgSQL function create_self_tab() line 3 at SQL statement
+SQL statement "CREATE TEMP TABLE new_table ON COMMIT DROP AS SELECT create_self_tab()"
+PL/pgSQL function create_self_tab() line 3 at SQL statement
+COMMIT;
+DROP FUNCTION create_self_tab();
-- verify that cursors created during an aborted subtransaction are
-- closed, but that we do not rollback the effect of any FETCHs
-- performed in the aborted subtransaction
diff --git a/src/test/regress/sql/transactions.sql b/src/test/regress/sql/transactions.sql
index 9fac4a3..8983e79 100644
--- a/src/test/regress/sql/transactions.sql
+++ b/src/test/regress/sql/transactions.sql
@@ -351,6 +351,19 @@ insert into revalidate_bug values (inverse(0));
drop table revalidate_bug;
drop function inverse(int);
+-- test case for relations referenced in active cursors
+CREATE OR REPLACE FUNCTION create_self_tab() RETURNS text
+LANGUAGE plpgsql AS $$
+BEGIN
+ CREATE TEMP TABLE new_table ON COMMIT DROP AS SELECT create_self_tab();
+ RETURN 'foo';
+END $$;
+BEGIN;
+DECLARE h CURSOR FOR SELECT create_self_tab();
+SAVEPOINT self_tab_point;
+FETCH h; -- error
+COMMIT;
+DROP FUNCTION create_self_tab();
-- verify that cursors created during an aborted subtransaction are
-- closed, but that we do not rollback the effect of any FETCHs
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
