From 1591fcbec77f6544a0f665758bae912c68e6fa42 Mon Sep 17 00:00:00 2001
From: Tom Lane <tgl@sss.pgh.pa.us>
Date: Wed, 2 Apr 2008 18:31:50 +0000
Subject: [PATCH] Revert my bad decision of about a year ago to make
PortalDefineQuery responsible for copying the query string into the new
Portal. Such copying is unnecessary in the common code path through
exec_simple_query, and in this case it can be enormously expensive because
the string might contain a large number of individual commands; we were
copying the entire, long string for each command, resulting in O(N^2)
behavior for N commands. (This is the cause of bug #4079.) A second problem
with it is that PortalDefineQuery really can't risk error, because if it
elog's before having set up the Portal, we will leak the plancache refcount
that the caller is trying to hand off to the portal. So go back to the
design in which the caller is responsible for making sure everything is
copied into the portal if necessary.
---
src/backend/commands/portalcmds.c | 5 +++-
src/backend/commands/prepare.c | 8 ++++--
src/backend/executor/spi.c | 22 +++++++++++++---
src/backend/tcop/postgres.c | 42 +++++++++++++++++++++++-------
src/backend/utils/mmgr/portalmem.c | 20 ++++++++------
5 files changed, 73 insertions(+), 24 deletions(-)
diff --git a/src/backend/commands/portalcmds.c b/src/backend/commands/portalcmds.c
index 64d0492b83e..83998233b49 100644
--- a/src/backend/commands/portalcmds.c
+++ b/src/backend/commands/portalcmds.c
@@ -14,7 +14,7 @@
*
*
* IDENTIFICATION
- * $PostgreSQL: pgsql/src/backend/commands/portalcmds.c,v 1.72 2008/03/26 18:48:59 alvherre Exp $
+ * $PostgreSQL: pgsql/src/backend/commands/portalcmds.c,v 1.73 2008/04/02 18:31:50 tgl Exp $
*
*-------------------------------------------------------------------------
*/
@@ -77,6 +77,9 @@ PerformCursorOpen(PlannedStmt *stmt, ParamListInfo params,
stmt = copyObject(stmt);
stmt->utilityStmt = NULL; /* make it look like plain SELECT */
+ if (queryString) /* copy the source text too for safety */
+ queryString = pstrdup(queryString);
+
PortalDefineQuery(portal,
NULL,
queryString,
diff --git a/src/backend/commands/prepare.c b/src/backend/commands/prepare.c
index 681647fac56..575dad9fb3f 100644
--- a/src/backend/commands/prepare.c
+++ b/src/backend/commands/prepare.c
@@ -10,7 +10,7 @@
* Copyright (c) 2002-2008, PostgreSQL Global Development Group
*
* IDENTIFICATION
- * $PostgreSQL: pgsql/src/backend/commands/prepare.c,v 1.84 2008/03/26 18:48:59 alvherre Exp $
+ * $PostgreSQL: pgsql/src/backend/commands/prepare.c,v 1.85 2008/04/02 18:31:50 tgl Exp $
*
*-------------------------------------------------------------------------
*/
@@ -250,9 +250,13 @@ ExecuteQuery(ExecuteStmt *stmt, const char *queryString,
plan_list = cplan->stmt_list;
}
+ /*
+ * Note: we don't bother to copy the source query string into the portal.
+ * Any errors it might be useful for will already have been reported.
+ */
PortalDefineQuery(portal,
NULL,
- entry->plansource->query_string,
+ NULL,
entry->plansource->commandTag,
plan_list,
cplan);
diff --git a/src/backend/executor/spi.c b/src/backend/executor/spi.c
index 42187fe43ad..f7d41373b4a 100644
--- a/src/backend/executor/spi.c
+++ b/src/backend/executor/spi.c
@@ -8,7 +8,7 @@
*
*
* IDENTIFICATION
- * $PostgreSQL: pgsql/src/backend/executor/spi.c,v 1.192 2008/04/01 03:09:30 tgl Exp $
+ * $PostgreSQL: pgsql/src/backend/executor/spi.c,v 1.193 2008/04/02 18:31:50 tgl Exp $
*
*-------------------------------------------------------------------------
*/
@@ -918,6 +918,7 @@ SPI_cursor_open(const char *name, SPIPlanPtr plan,
CachedPlanSource *plansource;
CachedPlan *cplan;
List *stmt_list;
+ char *query_string;
ParamListInfo paramLI;
Snapshot snapshot;
MemoryContext oldcontext;
@@ -968,10 +969,22 @@ SPI_cursor_open(const char *name, SPIPlanPtr plan,
portal = CreatePortal(name, false, false);
}
+ /*
+ * Prepare to copy stuff into the portal's memory context. We do all this
+ * copying first, because it could possibly fail (out-of-memory) and we
+ * don't want a failure to occur between RevalidateCachedPlan and
+ * PortalDefineQuery; that would result in leaking our plancache refcount.
+ */
+ oldcontext = MemoryContextSwitchTo(PortalGetHeapMemory(portal));
+
+ /* Copy the plan's query string, if available, into the portal */
+ query_string = plansource->query_string;
+ if (query_string)
+ query_string = pstrdup(query_string);
+
/* If the plan has parameters, copy them into the portal */
if (plan->nargs > 0)
{
- oldcontext = MemoryContextSwitchTo(PortalGetHeapMemory(portal));
/* sizeof(ParamListInfoData) includes the first array element */
paramLI = (ParamListInfo) palloc(sizeof(ParamListInfoData) +
(plan->nargs - 1) *sizeof(ParamExternData));
@@ -1000,11 +1013,12 @@ SPI_cursor_open(const char *name, SPIPlanPtr plan,
paramTypByVal, paramTypLen);
}
}
- MemoryContextSwitchTo(oldcontext);
}
else
paramLI = NULL;
+ MemoryContextSwitchTo(oldcontext);
+
if (plan->saved)
{
/* Replan if needed, and increment plan refcount for portal */
@@ -1025,7 +1039,7 @@ SPI_cursor_open(const char *name, SPIPlanPtr plan,
*/
PortalDefineQuery(portal,
NULL, /* no statement name */
- plansource->query_string,
+ query_string,
plansource->commandTag,
stmt_list,
cplan);
diff --git a/src/backend/tcop/postgres.c b/src/backend/tcop/postgres.c
index 95bea2ef8a9..d44e69fb10a 100644
--- a/src/backend/tcop/postgres.c
+++ b/src/backend/tcop/postgres.c
@@ -8,7 +8,7 @@
*
*
* IDENTIFICATION
- * $PostgreSQL: pgsql/src/backend/tcop/postgres.c,v 1.547 2008/03/26 18:48:59 alvherre Exp $
+ * $PostgreSQL: pgsql/src/backend/tcop/postgres.c,v 1.548 2008/04/02 18:31:50 tgl Exp $
*
* NOTES
* this is the "main" module of the postgres backend and
@@ -933,6 +933,11 @@ exec_simple_query(const char *query_string)
/* Don't display the portal in pg_cursors */
portal->visible = false;
+ /*
+ * We don't have to copy anything into the portal, because everything
+ * we are passsing here is in MessageContext, which will outlive the
+ * portal anyway.
+ */
PortalDefineQuery(portal,
NULL,
query_string,
@@ -1356,8 +1361,11 @@ exec_bind_message(StringInfo input_message)
CachedPlanSource *psrc;
CachedPlan *cplan;
Portal portal;
+ char *query_string;
+ char *saved_stmt_name;
ParamListInfo params;
List *plan_list;
+ MemoryContext oldContext;
bool save_log_statement_stats = log_statement_stats;
char msec_str[32];
@@ -1461,16 +1469,32 @@ exec_bind_message(StringInfo input_message)
else
portal = CreatePortal(portal_name, false, false);
+ /*
+ * Prepare to copy stuff into the portal's memory context. We do all this
+ * copying first, because it could possibly fail (out-of-memory) and we
+ * don't want a failure to occur between RevalidateCachedPlan and
+ * PortalDefineQuery; that would result in leaking our plancache refcount.
+ */
+ oldContext = MemoryContextSwitchTo(PortalGetHeapMemory(portal));
+
+ /* Copy the plan's query string, if available, into the portal */
+ query_string = psrc->query_string;
+ if (query_string)
+ query_string = pstrdup(query_string);
+
+ /* Likewise make a copy of the statement name, unless it's unnamed */
+ if (stmt_name[0])
+ saved_stmt_name = pstrdup(stmt_name);
+ else
+ saved_stmt_name = NULL;
+
/*
* Fetch parameters, if any, and store in the portal's memory context.
*/
if (numParams > 0)
{
- MemoryContext oldContext;
int paramno;
- oldContext = MemoryContextSwitchTo(PortalGetHeapMemory(portal));
-
/* sizeof(ParamListInfoData) includes the first array element */
params = (ParamListInfo) palloc(sizeof(ParamListInfoData) +
(numParams - 1) *sizeof(ParamExternData));
@@ -1595,12 +1619,13 @@ exec_bind_message(StringInfo input_message)
params->params[paramno].pflags = PARAM_FLAG_CONST;
params->params[paramno].ptype = ptype;
}
-
- MemoryContextSwitchTo(oldContext);
}
else
params = NULL;
+ /* Done storing stuff in portal's context */
+ MemoryContextSwitchTo(oldContext);
+
/* Get the result format codes */
numRFormats = pq_getmsgint(input_message, 2);
if (numRFormats > 0)
@@ -1627,7 +1652,6 @@ exec_bind_message(StringInfo input_message)
}
else
{
- MemoryContext oldContext;
List *query_list;
/*
@@ -1665,8 +1689,8 @@ exec_bind_message(StringInfo input_message)
* Define portal and start execution.
*/
PortalDefineQuery(portal,
- stmt_name[0] ? stmt_name : NULL,
- psrc->query_string,
+ saved_stmt_name,
+ query_string,
psrc->commandTag,
plan_list,
cplan);
diff --git a/src/backend/utils/mmgr/portalmem.c b/src/backend/utils/mmgr/portalmem.c
index 4940f7721f6..88bad32e29e 100644
--- a/src/backend/utils/mmgr/portalmem.c
+++ b/src/backend/utils/mmgr/portalmem.c
@@ -12,7 +12,7 @@
* Portions Copyright (c) 1994, Regents of the University of California
*
* IDENTIFICATION
- * $PostgreSQL: pgsql/src/backend/utils/mmgr/portalmem.c,v 1.108 2008/03/25 22:42:45 tgl Exp $
+ * $PostgreSQL: pgsql/src/backend/utils/mmgr/portalmem.c,v 1.109 2008/04/02 18:31:50 tgl Exp $
*
*-------------------------------------------------------------------------
*/
@@ -274,9 +274,7 @@ CreateNewPortal(void)
*
* Notes: commandTag shall be NULL if and only if the original query string
* (before rewriting) was an empty string. Also, the passed commandTag must
- * be a pointer to a constant string, since it is not copied. However,
- * prepStmtName and sourceText, if provided, are copied into the portal's
- * heap context for safekeeping.
+ * be a pointer to a constant string, since it is not copied.
*
* If cplan is provided, then it is a cached plan containing the stmts,
* and the caller must have done RevalidateCachedPlan(), causing a refcount
@@ -285,6 +283,14 @@ CreateNewPortal(void)
* If cplan is NULL, then it is the caller's responsibility to ensure that
* the passed plan trees have adequate lifetime. Typically this is done by
* copying them into the portal's heap context.
+ *
+ * The caller is also responsible for ensuring that the passed prepStmtName
+ * and sourceText (if not NULL) have adequate lifetime.
+ *
+ * NB: this function mustn't do much beyond storing the passed values; in
+ * particular don't do anything that risks elog(ERROR). If that were to
+ * happen here before storing the cplan reference, we'd leak the plancache
+ * refcount that the caller is trying to hand off to us.
*/
void
PortalDefineQuery(Portal portal,
@@ -299,10 +305,8 @@ PortalDefineQuery(Portal portal,
Assert(commandTag != NULL || stmts == NIL);
- portal->prepStmtName = prepStmtName ?
- MemoryContextStrdup(PortalGetHeapMemory(portal), prepStmtName) : NULL;
- portal->sourceText = sourceText ?
- MemoryContextStrdup(PortalGetHeapMemory(portal), sourceText) : NULL;
+ portal->prepStmtName = prepStmtName;
+ portal->sourceText = sourceText;
portal->commandTag = commandTag;
portal->stmts = stmts;
portal->cplan = cplan;
--
GitLab