Replace uses of SPI_modifytuple that intend to allocate in current context.

tglsfdc · tglsfdc · commit 9257f0787257 · 2016-11-08T15:36:44.000-05:00
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: &lt;9633.1478552022@sss.pgh.pa.us&gt;
diff --git a/contrib/spi/autoinc.c b/contrib/spi/autoinc.c
@@ -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,23 +105,23 @@ autoinc(PG_FUNCTION_ARGS)
 			newvals[chnattrs] = DirectFunctionCall1(nextval, seqname);
 			newvals[chnattrs] = Int32GetDatum((int32) DatumGetInt64(newvals[chnattrs]));
 		}
+		newnulls[chnattrs] = false;
 		pfree(DatumGetTextP(seqname));
 		chnattrs++;
 		i++;
 	}
 
 	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);
 }
diff --git a/contrib/spi/insert_username.c b/contrib/spi/insert_username.c
@@ -1,13 +1,12 @@
 /*
- * 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
  * usage:  insert_username (column_name)
  */
 #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);
 
diff --git a/contrib/spi/moddatetime.c b/contrib/spi/moddatetime.c
@@ -15,6 +15,7 @@ OH, me, I'm Terry Mackintosh <terry@terrym.com>
 */
 #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);
diff --git a/contrib/spi/timetravel.c b/contrib/spi/timetravel.c
@@ -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 */
diff --git a/doc/src/sgml/spi.sgml b/doc/src/sgml/spi.sgml
@@ -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
diff --git a/src/backend/access/common/heaptuple.c b/src/backend/access/common/heaptuple.c
@@ -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
diff --git a/src/backend/utils/adt/tsvector_op.c b/src/backend/utils/adt/tsvector_op.c
@@ -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);
 }
diff --git a/src/include/access/htup_details.h b/src/include/access/htup_details.h
@@ -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);
diff --git a/src/pl/plpgsql/src/pl_exec.c b/src/pl/plpgsql/src/pl_exec.c
diff --git a/src/test/regress/regress.c b/src/test/regress/regress.c
