portal pinning
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
Attachments:
0001-Use-portal-pinning-in-PL-Perl-and-PL-Python.patchtext/plain; charset=UTF-8; name=0001-Use-portal-pinning-in-PL-Perl-and-PL-Python.patch; x-mac-creator=0; x-mac-type=0Download
From ee052fc03d51d35617935679c30041127b635e21 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <peter_e@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
On 12/12/17 10:34, Peter Eisentraut wrote:
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.
Here is a patch for this option.
The above sentence was not quite correct. Only unnamed portals should
be pinned automatically. Named portals are of course possible to be
passed around as refcursors for example.
--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Attachments:
0001-Move-portal-pinning-from-PL-pgSQL-to-SPI.patchtext/plain; charset=UTF-8; name=0001-Move-portal-pinning-from-PL-pgSQL-to-SPI.patch; x-mac-creator=0; x-mac-type=0Download
From f9aaa47cf4e46aac973e532a790ac2099d017523 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <peter_e@gmx.net>
Date: Fri, 15 Dec 2017 15:24:10 -0500
Subject: [PATCH] Move portal pinning from PL/pgSQL to SPI
PL/pgSQL "pins" internally generated (unnamed) portals so that user code
cannot close them by guessing their names. This logic is also useful in
other languages and really for any code. So move that logic into SPI.
An unnamed portal obtained through SPI_cursor_open() and related
functions is now automatically pinned, and SPI_cursor_close()
automatically unpins a portal that is pinned.
---
src/backend/executor/spi.c | 9 +++++++++
src/pl/plpgsql/src/pl_exec.c | 8 --------
2 files changed, 9 insertions(+), 8 deletions(-)
diff --git a/src/backend/executor/spi.c b/src/backend/executor/spi.c
index f3da2ddd08..1f0a07ce0b 100644
--- a/src/backend/executor/spi.c
+++ b/src/backend/executor/spi.c
@@ -1175,6 +1175,12 @@ SPI_cursor_open_internal(const char *name, SPIPlanPtr plan,
{
/* Use a random nonconflicting name */
portal = CreateNewPortal();
+
+ /*
+ * Make sure the portal doesn't get closed by the user statements we
+ * execute.
+ */
+ PinPortal(portal);
}
else
{
@@ -1413,6 +1419,9 @@ SPI_cursor_close(Portal portal)
if (!PortalIsValid(portal))
elog(ERROR, "invalid portal in SPI cursor operation");
+ if (portal->portalPinned)
+ UnpinPortal(portal);
+
PortalDrop(portal, false);
}
diff --git a/src/pl/plpgsql/src/pl_exec.c b/src/pl/plpgsql/src/pl_exec.c
index fa4d573e50..243396abd1 100644
--- a/src/pl/plpgsql/src/pl_exec.c
+++ b/src/pl/plpgsql/src/pl_exec.c
@@ -5330,12 +5330,6 @@ exec_for_query(PLpgSQL_execstate *estate, PLpgSQL_stmt_forq *stmt,
/* Fetch loop variable's datum entry */
var = (PLpgSQL_variable *) estate->datums[stmt->var->dno];
- /*
- * Make sure the portal doesn't get closed by the user statements we
- * execute.
- */
- PinPortal(portal);
-
/*
* Fetch the initial tuple(s). If prefetching is allowed then we grab a
* few more rows to avoid multiple trips through executor startup
@@ -5450,8 +5444,6 @@ exec_for_query(PLpgSQL_execstate *estate, PLpgSQL_stmt_forq *stmt,
*/
SPI_freetuptable(tuptab);
- UnpinPortal(portal);
-
/*
* Set the FOUND variable to indicate the result of executing the loop
* (namely, whether we looped one or more times). This must be set last so
--
2.15.1
On 12/15/2017 03:36 PM, Peter Eisentraut wrote:
On 12/12/17 10:34, Peter Eisentraut wrote:
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.Here is a patch for this option.
The above sentence was not quite correct. Only unnamed portals should
be pinned automatically. Named portals are of course possible to be
passed around as refcursors for example.
This seems like a good idea, and the code change is tiny and clean. I
don't know of any third party PLs or other libraries might be pinning
the portals already on their own. How would they be affected if they did?
Anyway, marking ready for committer.
cheers
andrew
--
Andrew Dunstan https://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On 1/8/18 15:27, Andrew Dunstan wrote:
This seems like a good idea, and the code change is tiny and clean. I
don't know of any third party PLs or other libraries might be pinning
the portals already on their own. How would they be affected if they did?
They would get an error if they tried to pin it a second time. So this
would require a small source-level adjustment. But I doubt this is
actually the case anywhere, seeing that we are not even using this
consistently in core.
--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On 1/8/18 20:28, Peter Eisentraut wrote:
On 1/8/18 15:27, Andrew Dunstan wrote:
This seems like a good idea, and the code change is tiny and clean. I
don't know of any third party PLs or other libraries might be pinning
the portals already on their own. How would they be affected if they did?They would get an error if they tried to pin it a second time. So this
would require a small source-level adjustment. But I doubt this is
actually the case anywhere, seeing that we are not even using this
consistently in core.
committed
--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
committed
I'm afraid it causes regressions for pgjdbc.
Here's CI log: https://travis-ci.org/pgjdbc/pgjdbc/jobs/327327402
The errors are:
testMetaData[typeName = REF_CURSOR, cursorType =
2,012](org.postgresql.test.jdbc2.RefCursorTest) Time elapsed: 0.032 sec
<<< ERROR! org.postgresql.util.PSQLException: ERROR: cannot drop pinned
portal "<unnamed portal 1>"
It looks like "refcursor" expressions are somehow broken.
The test code is to execute testspg__getRefcursor procedure
that is defined as follows
CREATE OR REPLACE FUNCTION testspg__getRefcursor () RETURNS refcursor AS '
declare v_resset refcursor; begin open v_resset for select id from testrs
order by id;
return v_resset; end;' LANGUAGE plpgsql;
Would you please check that?
Vladimir
On 1/10/18 13:53, Vladimir Sitnikov wrote:
committed
I'm afraid it causes regressions for pgjdbc.
Here's CI log: https://travis-ci.org/pgjdbc/pgjdbc/jobs/327327402The errors are:
testMetaData[typeName = REF_CURSOR, cursorType =
2,012](org.postgresql.test.jdbc2.RefCursorTest) Time elapsed: 0.032 sec
<<< ERROR! org.postgresql.util.PSQLException: ERROR: cannot drop pinned
portal "<unnamed portal 1>"It looks like "refcursor" expressions are somehow broken.
Hmm, it seems like we are missing some test coverage in core for that.
I guess I'll have to revert this patch and go with the first approach of
putting it directly into PL/Perl and PL/Python.
--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services