Replace uses of SPI_modifytuple that intend to allocate in current context.
authorTom Lane <[email protected]>
Tue, 8 Nov 2016 20:36:36 +0000 (15:36 -0500)
committerTom Lane <[email protected]>
Tue, 8 Nov 2016 20:36:44 +0000 (15:36 -0500)
Invent a new function heap_modify_tuple_by_cols() that is functionally
equivalent to SPI_modifytuple except that it always allocates its result
by simple palloc.  I chose however to make the API details a bit more
like heap_modify_tuple: pass a tupdesc rather than a Relation, and use
bool convention for the isnull array.

Use this function in place of SPI_modifytuple at all call sites where the
intended behavior is to allocate in current context.  (There actually are
only two call sites left that depend on the old behavior, which makes me
wonder if we should just drop this function rather than keep it.)

This new function is easier to use than heap_modify_tuple() for purposes
of replacing a single column (or, really, any fixed number of columns).
There are a number of places where it would simplify the code to change
over, but I resisted that temptation for the moment ... everywhere except
in plpgsql's exec_assign_value(); changing that might offer some small
performance benefit, so I did it.

This is on the way to removing SPI_push/SPI_pop, but it seems like
good code cleanup in its own right.

Discussion: <9633.1478552022@sss.pgh.pa.us>

contrib/spi/autoinc.c
contrib/spi/insert_username.c
contrib/spi/moddatetime.c
contrib/spi/timetravel.c
doc/src/sgml/spi.sgml
src/backend/access/common/heaptuple.c
src/backend/utils/adt/tsvector_op.c
src/include/access/htup_details.h
src/pl/plpgsql/src/pl_exec.c
src/test/regress/regress.c

index fc657a7c06e203177abade3db6df7f9b3df5ecaf..54f85a37090a3443c176294f4680279d8dc6a0e7 100644 (file)
@@ -3,6 +3,7 @@
  */
 #include "postgres.h"
 
+#include "access/htup_details.h"
 #include "catalog/pg_type.h"
 #include "commands/sequence.h"
 #include "commands/trigger.h"
@@ -23,6 +24,7 @@ autoinc(PG_FUNCTION_ARGS)
    int        *chattrs;        /* attnums of attributes to change */
    int         chnattrs = 0;   /* # of above */
    Datum      *newvals;        /* vals of above */
+   bool       *newnulls;       /* null flags for above */
    char      **args;           /* arguments */
    char       *relname;        /* triggered relation name */
    Relation    rel;            /* triggered relation */
@@ -64,6 +66,7 @@ autoinc(PG_FUNCTION_ARGS)
 
    chattrs = (int *) palloc(nargs / 2 * sizeof(int));
    newvals = (Datum *) palloc(nargs / 2 * sizeof(Datum));
+   newnulls = (bool *) palloc(nargs / 2 * sizeof(bool));
 
    for (i = 0; i < nargs;)
    {
@@ -102,6 +105,7 @@ autoinc(PG_FUNCTION_ARGS)
            newvals[chnattrs] = DirectFunctionCall1(nextval, seqname);
            newvals[chnattrs] = Int32GetDatum((int32) DatumGetInt64(newvals[chnattrs]));
        }
+       newnulls[chnattrs] = false;
        pfree(DatumGetTextP(seqname));
        chnattrs++;
        i++;
@@ -109,16 +113,15 @@ autoinc(PG_FUNCTION_ARGS)
 
    if (chnattrs > 0)
    {
-       rettuple = SPI_modifytuple(rel, rettuple, chnattrs, chattrs, newvals, NULL);
-       if (rettuple == NULL)
-           /* internal error */
-           elog(ERROR, "autoinc (%s): %d returned by SPI_modifytuple",
-                relname, SPI_result);
+       rettuple = heap_modify_tuple_by_cols(rettuple, tupdesc,
+                                            chnattrs, chattrs,
+                                            newvals, newnulls);
    }
 
    pfree(relname);
    pfree(chattrs);
    pfree(newvals);
+   pfree(newnulls);
 
    return PointerGetDatum(rettuple);
 }
index 617c60a81c57e619d539b41f5878f191197a899a..a2e1747ff74c7667665dcc334f54ad368885d83c 100644 (file)
@@ -1,6 +1,4 @@
 /*
- * insert_username.c
- * $Modified: Thu Oct 16 08:13:42 1997 by brook $
  * contrib/spi/insert_username.c
  *
  * insert user name in response to a trigger
@@ -8,6 +6,7 @@
  */
 #include "postgres.h"
 
+#include "access/htup_details.h"
 #include "catalog/pg_type.h"
 #include "commands/trigger.h"
 #include "executor/spi.h"
@@ -26,6 +25,7 @@ insert_username(PG_FUNCTION_ARGS)
    Trigger    *trigger;        /* to get trigger name */
    int         nargs;          /* # of arguments */
    Datum       newval;         /* new value of column */
+   bool        newnull;        /* null flag */
    char      **args;           /* arguments */
    char       *relname;        /* triggered relation name */
    Relation    rel;            /* triggered relation */
@@ -80,13 +80,11 @@ insert_username(PG_FUNCTION_ARGS)
 
    /* create fields containing name */
    newval = CStringGetTextDatum(GetUserNameFromId(GetUserId(), false));
+   newnull = false;
 
    /* construct new tuple */
-   rettuple = SPI_modifytuple(rel, rettuple, 1, &attnum, &newval, NULL);
-   if (rettuple == NULL)
-       /* internal error */
-       elog(ERROR, "insert_username (\"%s\"): %d returned by SPI_modifytuple",
-            relname, SPI_result);
+   rettuple = heap_modify_tuple_by_cols(rettuple, tupdesc,
+                                        1, &attnum, &newval, &newnull);
 
    pfree(relname);
 
index cd700fe6d131cc2d8356e7adfa37293a5c9e9de1..2d1f22c4e172a2dc6e67d814fe5fbd5f927617a6 100644 (file)
@@ -15,6 +15,7 @@ OH, me, I'm Terry Mackintosh <[email protected]>
 */
 #include "postgres.h"
 
+#include "access/htup_details.h"
 #include "catalog/pg_type.h"
 #include "executor/spi.h"
 #include "commands/trigger.h"
@@ -34,6 +35,7 @@ moddatetime(PG_FUNCTION_ARGS)
    int         attnum;         /* positional number of field to change */
    Oid         atttypid;       /* type OID of field to change */
    Datum       newdt;          /* The current datetime. */
+   bool        newdtnull;      /* null flag for it */
    char      **args;           /* arguments */
    char       *relname;        /* triggered relation name */
    Relation    rel;            /* triggered relation */
@@ -115,22 +117,13 @@ moddatetime(PG_FUNCTION_ARGS)
                        args[0], relname)));
        newdt = (Datum) 0;      /* keep compiler quiet */
    }
+   newdtnull = false;
 
-/* 1 is the number of items in the arrays attnum and newdt.
-   attnum is the positional number of the field to be updated.
-   newdt is the new datetime stamp.
-   NOTE that attnum and newdt are not arrays, but then a 1 element array
-   is not an array any more then they are.  Thus, they can be considered a
-   one element array.
-*/
-   rettuple = SPI_modifytuple(rel, rettuple, 1, &attnum, &newdt, NULL);
-
-   if (rettuple == NULL)
-       /* internal error */
-       elog(ERROR, "moddatetime (%s): %d returned by SPI_modifytuple",
-            relname, SPI_result);
+   /* Replace the attnum'th column with newdt */
+   rettuple = heap_modify_tuple_by_cols(rettuple, tupdesc,
+                                        1, &attnum, &newdt, &newdtnull);
 
-/* Clean up */
+   /* Clean up */
    pfree(relname);
 
    return PointerGetDatum(rettuple);
index 30dcfd4d3ecc90cce6f5a9aaa0c8c4c46f2e1a79..2733aa231e84ecf69417bb6feea965ea5e2f208f 100644 (file)
@@ -11,6 +11,7 @@
 
 #include <ctype.h>
 
+#include "access/htup_details.h"
 #include "catalog/pg_type.h"
 #include "commands/trigger.h"
 #include "executor/spi.h"
@@ -183,13 +184,13 @@ timetravel(PG_FUNCTION_ARGS)
        int         chnattrs = 0;
        int         chattrs[MaxAttrNum];
        Datum       newvals[MaxAttrNum];
-       char        newnulls[MaxAttrNum];
+       bool        newnulls[MaxAttrNum];
 
        oldtimeon = SPI_getbinval(trigtuple, tupdesc, attnum[a_time_on], &isnull);
        if (isnull)
        {
            newvals[chnattrs] = GetCurrentAbsoluteTime();
-           newnulls[chnattrs] = ' ';
+           newnulls[chnattrs] = false;
            chattrs[chnattrs] = attnum[a_time_on];
            chnattrs++;
        }
@@ -201,7 +202,7 @@ timetravel(PG_FUNCTION_ARGS)
                (chnattrs > 0 && DatumGetInt32(newvals[a_time_on]) >= NOEND_ABSTIME))
                elog(ERROR, "timetravel (%s): %s is infinity", relname, args[a_time_on]);
            newvals[chnattrs] = NOEND_ABSTIME;
-           newnulls[chnattrs] = ' ';
+           newnulls[chnattrs] = false;
            chattrs[chnattrs] = attnum[a_time_off];
            chnattrs++;
        }
@@ -220,21 +221,23 @@ timetravel(PG_FUNCTION_ARGS)
        {
            /* clear update_user value */
            newvals[chnattrs] = nulltext;
-           newnulls[chnattrs] = 'n';
+           newnulls[chnattrs] = true;
            chattrs[chnattrs] = attnum[a_upd_user];
            chnattrs++;
            /* clear delete_user value */
            newvals[chnattrs] = nulltext;
-           newnulls[chnattrs] = 'n';
+           newnulls[chnattrs] = true;
            chattrs[chnattrs] = attnum[a_del_user];
            chnattrs++;
            /* set insert_user value */
            newvals[chnattrs] = newuser;
-           newnulls[chnattrs] = ' ';
+           newnulls[chnattrs] = false;
            chattrs[chnattrs] = attnum[a_ins_user];
            chnattrs++;
        }
-       rettuple = SPI_modifytuple(rel, trigtuple, chnattrs, chattrs, newvals, newnulls);
+       rettuple = heap_modify_tuple_by_cols(trigtuple, tupdesc,
+                                            chnattrs, chattrs,
+                                            newvals, newnulls);
        return PointerGetDatum(rettuple);
        /* end of INSERT */
    }
@@ -395,13 +398,11 @@ timetravel(PG_FUNCTION_ARGS)
            chnattrs++;
        }
 
-       rettuple = SPI_modifytuple(rel, newtuple, chnattrs, chattrs, newvals, newnulls);
-
        /*
-        * SPI_copytuple allocates tmptuple in upper executor context - have
-        * to free allocation using SPI_pfree
+        * Use SPI_modifytuple() here because we are inside SPI environment
+        * but rettuple must be allocated in caller's context.
         */
-       /* SPI_pfree(tmptuple); */
+       rettuple = SPI_modifytuple(rel, newtuple, chnattrs, chattrs, newvals, newnulls);
    }
    else
        /* DELETE case */
index 817a5d0120ad4c6e640bc1f4e94272fe84c89add..39133c90385f68e80eaef3ce5c9b79c09d704eef 100644 (file)
@@ -3382,8 +3382,9 @@ char * SPI_getnspname(Relation <parameter>rel</parameter>)
    <function>repalloc</function>, or SPI utility functions (except for
    <function>SPI_copytuple</function>,
    <function>SPI_returntuple</function>,
-   <function>SPI_modifytuple</function>, and
-   <function>SPI_palloc</function>) are made in this context.  When a
+   <function>SPI_modifytuple</function>,
+   <function>SPI_palloc</function>, and
+   <function>SPI_datumTransfer</function>) are made in this context.  When a
    procedure disconnects from the SPI manager (via
    <function>SPI_finish</function>) the current context is restored to
    the upper executor context, and all allocations made in the
index 6d0f3f37673a5d3a48358149213405f7bbadde43..e27ec78b7148973360873bb0ded9e2a7022a4d62 100644 (file)
@@ -846,6 +846,72 @@ heap_modify_tuple(HeapTuple tuple,
    return newTuple;
 }
 
+/*
+ * heap_modify_tuple_by_cols
+ *     form a new tuple from an old tuple and a set of replacement values.
+ *
+ * This is like heap_modify_tuple, except that instead of specifying which
+ * column(s) to replace by a boolean map, an array of target column numbers
+ * is used.  This is often more convenient when a fixed number of columns
+ * are to be replaced.  The replCols, replValues, and replIsnull arrays must
+ * be of length nCols.  Target column numbers are indexed from 1.
+ *
+ * The result is allocated in the current memory context.
+ */
+HeapTuple
+heap_modify_tuple_by_cols(HeapTuple tuple,
+                         TupleDesc tupleDesc,
+                         int nCols,
+                         int *replCols,
+                         Datum *replValues,
+                         bool *replIsnull)
+{
+   int         numberOfAttributes = tupleDesc->natts;
+   Datum      *values;
+   bool       *isnull;
+   HeapTuple   newTuple;
+   int         i;
+
+   /*
+    * allocate and fill values and isnull arrays from the tuple, then replace
+    * selected columns from the input arrays.
+    */
+   values = (Datum *) palloc(numberOfAttributes * sizeof(Datum));
+   isnull = (bool *) palloc(numberOfAttributes * sizeof(bool));
+
+   heap_deform_tuple(tuple, tupleDesc, values, isnull);
+
+   for (i = 0; i < nCols; i++)
+   {
+       int         attnum = replCols[i];
+
+       if (attnum <= 0 || attnum > numberOfAttributes)
+           elog(ERROR, "invalid column number %d", attnum);
+       values[attnum - 1] = replValues[i];
+       isnull[attnum - 1] = replIsnull[i];
+   }
+
+   /*
+    * create a new tuple from the values and isnull arrays
+    */
+   newTuple = heap_form_tuple(tupleDesc, values, isnull);
+
+   pfree(values);
+   pfree(isnull);
+
+   /*
+    * copy the identification info of the old tuple: t_ctid, t_self, and OID
+    * (if any)
+    */
+   newTuple->t_data->t_ctid = tuple->t_data->t_ctid;
+   newTuple->t_self = tuple->t_self;
+   newTuple->t_tableOid = tuple->t_tableOid;
+   if (tupleDesc->tdhasoid)
+       HeapTupleSetOid(newTuple, HeapTupleGetOid(tuple));
+
+   return newTuple;
+}
+
 /*
  * heap_deform_tuple
  *     Given a tuple, extract data into values/isnull arrays; this is
index 0e9ae5ff9cf538b5570f63ff82bc8d3cd48f17ff..c9d5060f2c7d39a1d6f5020a5e9dd59c9b3f9d11 100644 (file)
@@ -2329,8 +2329,10 @@ tsvector_update_trigger(PG_FUNCTION_ARGS, bool config_column)
    if (prs.curwords)
    {
        datum = PointerGetDatum(make_tsvector(&prs));
-       rettuple = SPI_modifytuple(rel, rettuple, 1, &tsvector_attr_num,
-                                  &datum, NULL);
+       isnull = false;
+       rettuple = heap_modify_tuple_by_cols(rettuple, rel->rd_att,
+                                            1, &tsvector_attr_num,
+                                            &datum, &isnull);
        pfree(DatumGetPointer(datum));
    }
    else
@@ -2340,14 +2342,12 @@ tsvector_update_trigger(PG_FUNCTION_ARGS, bool config_column)
        SET_VARSIZE(out, CALCDATASIZE(0, 0));
        out->size = 0;
        datum = PointerGetDatum(out);
-       rettuple = SPI_modifytuple(rel, rettuple, 1, &tsvector_attr_num,
-                                  &datum, NULL);
+       isnull = false;
+       rettuple = heap_modify_tuple_by_cols(rettuple, rel->rd_att,
+                                            1, &tsvector_attr_num,
+                                            &datum, &isnull);
        pfree(prs.words);
    }
 
-   if (rettuple == NULL)       /* internal error */
-       elog(ERROR, "tsvector_update_trigger: %d returned by SPI_modifytuple",
-            SPI_result);
-
    return PointerGetDatum(rettuple);
 }
index d7e5fad11e56bf68ad5a65e867b915e8f5fd4a34..8fb1f6ddea14a9de86ae0e10dadd6f4e2e15af4c 100644 (file)
@@ -805,6 +805,12 @@ extern HeapTuple heap_modify_tuple(HeapTuple tuple,
                  Datum *replValues,
                  bool *replIsnull,
                  bool *doReplace);
+extern HeapTuple heap_modify_tuple_by_cols(HeapTuple tuple,
+                         TupleDesc tupleDesc,
+                         int nCols,
+                         int *replCols,
+                         Datum *replValues,
+                         bool *replIsnull);
 extern void heap_deform_tuple(HeapTuple tuple, TupleDesc tupleDesc,
                  Datum *values, bool *isnull);
 extern void heap_freetuple(HeapTuple htup);
index 042b31fd77f831e1e794555ba8c8257349080243..91e1f8dd3fd8918c4ef6ee5e37a2911199ec874e 100644 (file)
@@ -4562,10 +4562,9 @@ exec_assign_value(PLpgSQL_execstate *estate,
                PLpgSQL_rec *rec;
                int         fno;
                HeapTuple   newtup;
-               int         natts;
-               Datum      *values;
-               bool       *nulls;
-               bool       *replaces;
+               int         colnums[1];
+               Datum       values[1];
+               bool        nulls[1];
                Oid         atttype;
                int32       atttypmod;
 
@@ -4584,9 +4583,8 @@ exec_assign_value(PLpgSQL_execstate *estate,
                           errdetail("The tuple structure of a not-yet-assigned record is indeterminate.")));
 
                /*
-                * Get the number of the records field to change and the
-                * number of attributes in the tuple.  Note: disallow system
-                * column names because the code below won't cope.
+                * Get the number of the record field to change.  Disallow
+                * system columns because the code below won't cope.
                 */
                fno = SPI_fnumber(rec->tupdesc, recfield->fieldname);
                if (fno <= 0)
@@ -4594,42 +4592,25 @@ exec_assign_value(PLpgSQL_execstate *estate,
                            (errcode(ERRCODE_UNDEFINED_COLUMN),
                             errmsg("record \"%s\" has no field \"%s\"",
                                    rec->refname, recfield->fieldname)));
-               fno--;
-               natts = rec->tupdesc->natts;
-
-               /*
-                * Set up values/control arrays for heap_modify_tuple. For all
-                * the attributes except the one we want to replace, use the
-                * value that's in the old tuple.
-                */
-               values = eval_mcontext_alloc(estate, sizeof(Datum) * natts);
-               nulls = eval_mcontext_alloc(estate, sizeof(bool) * natts);
-               replaces = eval_mcontext_alloc(estate, sizeof(bool) * natts);
-
-               memset(replaces, false, sizeof(bool) * natts);
-               replaces[fno] = true;
+               colnums[0] = fno;
 
                /*
                 * Now insert the new value, being careful to cast it to the
                 * right type.
                 */
-               atttype = rec->tupdesc->attrs[fno]->atttypid;
-               atttypmod = rec->tupdesc->attrs[fno]->atttypmod;
-               values[fno] = exec_cast_value(estate,
-                                             value,
-                                             &isNull,
-                                             valtype,
-                                             valtypmod,
-                                             atttype,
-                                             atttypmod);
-               nulls[fno] = isNull;
-
-               /*
-                * Now call heap_modify_tuple() to create a new tuple that
-                * replaces the old one in the record.
-                */
-               newtup = heap_modify_tuple(rec->tup, rec->tupdesc,
-                                          values, nulls, replaces);
+               atttype = rec->tupdesc->attrs[fno - 1]->atttypid;
+               atttypmod = rec->tupdesc->attrs[fno - 1]->atttypmod;
+               values[0] = exec_cast_value(estate,
+                                           value,
+                                           &isNull,
+                                           valtype,
+                                           valtypmod,
+                                           atttype,
+                                           atttypmod);
+               nulls[0] = isNull;
+
+               newtup = heap_modify_tuple_by_cols(rec->tup, rec->tupdesc,
+                                                  1, colnums, values, nulls);
 
                if (rec->freetup)
                    heap_freetuple(rec->tup);
index 119a59ab0732ca12419f63ec19a3bbace3a9ba1d..32703fcdcf9c3978277933efd7859dd31af654c6 100644 (file)
@@ -639,15 +639,8 @@ ttdummy(PG_FUNCTION_ARGS)
 
    /* Tuple to return to upper Executor ... */
    if (newtuple)               /* UPDATE */
-   {
-       HeapTuple   tmptuple;
-
-       tmptuple = SPI_copytuple(trigtuple);
-       rettuple = SPI_modifytuple(rel, tmptuple, 1, &(attnum[1]), &newoff, NULL);
-       SPI_freetuple(tmptuple);
-   }
-   else
-       /* DELETE */
+       rettuple = SPI_modifytuple(rel, trigtuple, 1, &(attnum[1]), &newoff, NULL);
+   else    /* DELETE */
        rettuple = trigtuple;
 
    SPI_finish();               /* don't forget say Bye to SPI mgr */