]> granicus.if.org Git - postgresql/commitdiff
Revert my bad decision of about a year ago to make PortalDefineQuery
authorTom Lane <tgl@sss.pgh.pa.us>
Wed, 2 Apr 2008 18:32:00 +0000 (18:32 +0000)
committerTom Lane <tgl@sss.pgh.pa.us>
Wed, 2 Apr 2008 18:32:00 +0000 (18:32 +0000)
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
src/backend/commands/prepare.c
src/backend/executor/spi.c
src/backend/tcop/postgres.c
src/backend/utils/mmgr/portalmem.c

index 6efd09c44b14bd0cfb233e9268c1c9cf15d793df..5a93aec6890ab9394b441b93f8154dbaa72ae7c0 100644 (file)
@@ -14,7 +14,7 @@
  *
  *
  * IDENTIFICATION
- *       $PostgreSQL: pgsql/src/backend/commands/portalcmds.c,v 1.69 2008/01/01 19:45:49 momjian Exp $
+ *       $PostgreSQL: pgsql/src/backend/commands/portalcmds.c,v 1.69.2.1 2008/04/02 18:32:00 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -76,6 +76,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,
index 2199e109d46ec142c0fa3c0626e9e252a9009975..7dcd3cb5a85901b810d209555d1b4904508e6586 100644 (file)
@@ -10,7 +10,7 @@
  * Copyright (c) 2002-2008, PostgreSQL Global Development Group
  *
  * IDENTIFICATION
- *       $PostgreSQL: pgsql/src/backend/commands/prepare.c,v 1.80 2008/01/01 19:45:49 momjian Exp $
+ *       $PostgreSQL: pgsql/src/backend/commands/prepare.c,v 1.80.2.1 2008/04/02 18:32:00 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -249,9 +249,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);
index a23c4d017a86f74b9442b3a9f17ef885d1fe5b1d..f79868630f68c6cefdd3591efd577ae236979721 100644 (file)
@@ -8,7 +8,7 @@
  *
  *
  * IDENTIFICATION
- *       $PostgreSQL: pgsql/src/backend/executor/spi.c,v 1.188 2008/02/12 04:09:44 tgl Exp $
+ *       $PostgreSQL: pgsql/src/backend/executor/spi.c,v 1.188.2.1 2008/04/02 18:32:00 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -858,6 +858,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;
@@ -908,10 +909,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));
@@ -940,11 +953,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 */
@@ -965,7 +979,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);
index a6afecaba603dfb2fd4c4a252b93a29bf19e777b..c3531b584494bc4051d0de910c33086b5a462c3b 100644 (file)
@@ -8,7 +8,7 @@
  *
  *
  * IDENTIFICATION
- *       $PostgreSQL: pgsql/src/backend/tcop/postgres.c,v 1.542.2.1 2008/03/12 23:58:35 tgl Exp $
+ *       $PostgreSQL: pgsql/src/backend/tcop/postgres.c,v 1.542.2.2 2008/04/02 18:32:00 tgl Exp $
  *
  * NOTES
  *       this is the "main" module of the postgres backend and
@@ -931,6 +931,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,
@@ -1354,8 +1359,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];
 
@@ -1459,16 +1467,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));
@@ -1593,12 +1617,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)
@@ -1625,7 +1650,6 @@ exec_bind_message(StringInfo input_message)
        }
        else
        {
-               MemoryContext oldContext;
                List       *query_list;
 
                /*
@@ -1663,8 +1687,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);
index ca5604a61d5162666508912fb707a022ce144bf5..b977396fb420c55fe61ce756b81b828737777229 100644 (file)
@@ -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.106 2008/01/01 19:45:55 momjian Exp $
+ *       $PostgreSQL: pgsql/src/backend/utils/mmgr/portalmem.c,v 1.106.2.1 2008/04/02 18:32:00 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;