While working on transaction control in procedures, I noticed some
inconsistencies in how portal pinning is used.
This mechanism was introduced in
eb81b6509f4c9109ecf8839d8c482cc597270687 to prevent user code from
closing cursors that PL/pgSQL has created internally, mainly for FOR
loops. Otherwise, user code could just write CLOSE '<unnamed portal X>'
to mess with the language internals.
It seems to me that PL/Perl and PL/Python should also use that
mechanism, because the same problem could happen there. (PL/Tcl does
not expose any cursor functionality AFAICT.) Currently, in PL/Perl, if
an internally generated cursor is closed, PL/Perl just thinks the cursor
has been exhausted and silently does nothing. PL/Python comes back with
a slightly bizarre error message "closing a cursor in an aborted
subtransaction", which might apply in some situations but not in all.
Attached is a sample patch that adds portal pinning to PL/Perl and
PL/Python.
But I also wonder whether we shouldn't automatically pin/unpin portals
in SPI_cursor_open() and SPI_cursor_close(). This makes sense if you
consider "pinned" to mean "internally generated". I don't think there
is a scenario in which user code should directly operate on a portal
created by SPI.
Comments?
--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
From ee052fc03d51d35617935679c30041127b635e21 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <pete...@gmx.net>
Date: Tue, 12 Dec 2017 10:26:47 -0500
Subject: [PATCH] Use portal pinning in PL/Perl and PL/Python
---
src/backend/utils/mmgr/portalmem.c | 14 ++++++++++----
src/pl/plperl/plperl.c | 8 ++++++++
src/pl/plpython/plpy_cursorobject.c | 8 ++++++++
3 files changed, 26 insertions(+), 4 deletions(-)
diff --git a/src/backend/utils/mmgr/portalmem.c
b/src/backend/utils/mmgr/portalmem.c
index d03b779407..e1872b8fda 100644
--- a/src/backend/utils/mmgr/portalmem.c
+++ b/src/backend/utils/mmgr/portalmem.c
@@ -464,11 +464,17 @@ PortalDrop(Portal portal, bool isTopCommit)
/*
* Don't allow dropping a pinned portal, it's still needed by whoever
- * pinned it. Not sure if the PORTAL_ACTIVE case can validly happen or
- * not...
+ * pinned it.
*/
- if (portal->portalPinned ||
- portal->status == PORTAL_ACTIVE)
+ if (portal->portalPinned)
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_CURSOR_STATE),
+ errmsg("cannot drop pinned portal \"%s\"",
portal->name)));
+
+ /*
+ * Not sure if the PORTAL_ACTIVE case can validly happen or not...
+ */
+ if (portal->status == PORTAL_ACTIVE)
ereport(ERROR,
(errcode(ERRCODE_INVALID_CURSOR_STATE),
errmsg("cannot drop active portal \"%s\"",
portal->name)));
diff --git a/src/pl/plperl/plperl.c b/src/pl/plperl/plperl.c
index 9f5313235f..5dd3026dc2 100644
--- a/src/pl/plperl/plperl.c
+++ b/src/pl/plperl/plperl.c
@@ -3405,6 +3405,8 @@ plperl_spi_query(char *query)
SPI_result_code_string(SPI_result));
cursor = cstr2sv(portal->name);
+ PinPortal(portal);
+
/* Commit the inner transaction, return to outer xact context */
ReleaseCurrentSubTransaction();
MemoryContextSwitchTo(oldcontext);
@@ -3468,6 +3470,7 @@ plperl_spi_fetchrow(char *cursor)
SPI_cursor_fetch(p, true, 1);
if (SPI_processed == 0)
{
+ UnpinPortal(p);
SPI_cursor_close(p);
row = &PL_sv_undef;
}
@@ -3519,7 +3522,10 @@ plperl_spi_cursor_close(char *cursor)
p = SPI_cursor_find(cursor);
if (p)
+ {
+ UnpinPortal(p);
SPI_cursor_close(p);
+ }
}
SV *
@@ -3883,6 +3889,8 @@ plperl_spi_query_prepared(char *query, int argc, SV
**argv)
cursor = cstr2sv(portal->name);
+ PinPortal(portal);
+
/* Commit the inner transaction, return to outer xact context */
ReleaseCurrentSubTransaction();
MemoryContextSwitchTo(oldcontext);
diff --git a/src/pl/plpython/plpy_cursorobject.c
b/src/pl/plpython/plpy_cursorobject.c
index 9467f64808..a527585b81 100644
--- a/src/pl/plpython/plpy_cursorobject.c
+++ b/src/pl/plpython/plpy_cursorobject.c
@@ -151,6 +151,8 @@ PLy_cursor_query(const char *query)
cursor->portalname = MemoryContextStrdup(cursor->mcxt,
portal->name);
+ PinPortal(portal);
+
PLy_spi_subtransaction_commit(oldcontext, oldowner);
}
PG_CATCH();
@@ -266,6 +268,8 @@ PLy_cursor_plan(PyObject *ob, PyObject *args)
cursor->portalname = MemoryContextStrdup(cursor->mcxt,
portal->name);
+ PinPortal(portal);
+
PLy_spi_subtransaction_commit(oldcontext, oldowner);
}
PG_CATCH();
@@ -317,7 +321,10 @@ PLy_cursor_dealloc(PyObject *arg)
portal = GetPortalByName(cursor->portalname);
if (PortalIsValid(portal))
+ {
+ UnpinPortal(portal);
SPI_cursor_close(portal);
+ }
cursor->closed = true;
}
if (cursor->mcxt)
@@ -508,6 +515,7 @@ PLy_cursor_close(PyObject *self, PyObject *unused)
return NULL;
}
+ UnpinPortal(portal);
SPI_cursor_close(portal);
cursor->closed = true;
}
--
2.15.1
