From: Tom Lane <tgl@sss.pgh.pa.us>
To: pgsql-hackers@lists.postgresql.org
cc: Daniel Gustafsson <daniel@yesql.se>
Subject: Further hacking on SPITupleTable struct
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="----- =_aaaaaaaaaa0"
Content-ID: <16761.1563395582.0@sss.pgh.pa.us>
Date: Wed, 17 Jul 2019 16:35:22 -0400
Message-ID: <16852.1563395722@sss.pgh.pa.us>
Precedence: bulk

------- =_aaaaaaaaaa0
Content-Type: text/plain; charset="us-ascii"
Content-ID: <16761.1563395582.1@sss.pgh.pa.us>

Thinking more about the public/private field distinction we just
specified --- it's always annoyed me that SPITupleTable doesn't
provide a number-of-valid-rows field, so that callers have to
look at the entirely separate SPI_processed variable in order
to make sense of SPI_tuptable.  I looked a bit more closely at
the code in question, and realized that it was just Randomly
Doing Things Differently from every other implementation we have
of expansible arrays.  Not only is it randomly different, but
it's not even better than our usual method of tracking current
and maximum numbers of elements: it has to do extra subtractions.

Accordingly, I propose the attached follow-on to fec0778c8,
which replaces the "free" field with a "numvals" field that
is considered public.

I poked around for callers that might prefer to use SPI_tuptable->numvals
in place of SPI_processed, and didn't immediately find anything where the
benefit of changing seemed compelling.  In principle, though, it should
be possible to simplify some callers by needing only one variable to be
passed around instead of two.

Thoughts?

			regards, tom lane


------- =_aaaaaaaaaa0
Content-Type: text/x-diff; name="provide-public-numvals-field.patch";
	charset="us-ascii"
Content-ID: <16761.1563395582.2@sss.pgh.pa.us>
Content-Description: provide-public-numvals-field.patch
Content-Transfer-Encoding: quoted-printable

diff --git a/doc/src/sgml/spi.sgml b/doc/src/sgml/spi.sgml
index e39f99f..9e37fc3 100644
--- a/doc/src/sgml/spi.sgml
+++ b/doc/src/sgml/spi.sgml
@@ -323,19 +323,23 @@ typedef struct SPITupleTable
     /* Public members */
     TupleDesc   tupdesc;        /* tuple descriptor */
     HeapTuple  *vals;           /* array of tuples */
+    uint64      numvals;        /* number of valid tuples */
 =

     /* Private members, not intended for external callers */
+    uint64      alloced;        /* allocated length of vals array */
     MemoryContext tuptabcxt;    /* memory context of result table */
-    uint64      alloced;        /* # of alloced vals */
-    uint64      free;           /* # of free vals */
     slist_node  next;           /* link for internal bookkeeping */
     SubTransactionId subid;     /* subxact in which tuptable was created =
*/
 } SPITupleTable;
 </programlisting>
-   <structfield>vals</structfield> and <structfield>tupdesc</structfield>=
 can
-   be used by SPI callers, the remaining fields are internal.
-   <structfield>vals</structfield> is an array of pointers to rows.  (The=
 number
-   of valid entries is given by <varname>SPI_processed</varname>.)
+   <structfield>tupdesc</structfield>,
+   <structfield>vals</structfield>, and
+   <structfield>numvals</structfield>
+   can be used by SPI callers; the remaining fields are internal.
+   <structfield>vals</structfield> is an array of pointers to rows.
+   The number of rows is given by <structfield>numvals</structfield>
+   (for somewhat historical reasons, this count is also returned
+   in <varname>SPI_processed</varname>).
    <structfield>tupdesc</structfield> is a row descriptor which you can p=
ass to
    SPI functions dealing with rows.
   </para>
@@ -4631,12 +4635,12 @@ execq(PG_FUNCTION_ARGS)
      */
     if (ret &gt; 0 &amp;&amp; SPI_tuptable !=3D NULL)
     {
-        TupleDesc tupdesc =3D SPI_tuptable-&gt;tupdesc;
         SPITupleTable *tuptable =3D SPI_tuptable;
+        TupleDesc tupdesc =3D tuptable-&gt;tupdesc;
         char buf[8192];
         uint64 j;
 =

-        for (j =3D 0; j &lt; proc; j++)
+        for (j =3D 0; j &lt; tuptable-&gt;numvals; j++)
         {
             HeapTuple tuple =3D tuptable-&gt;vals[j];
             int i;
diff --git a/src/backend/executor/spi.c b/src/backend/executor/spi.c
index 8eedb61..74f8d89 100644
--- a/src/backend/executor/spi.c
+++ b/src/backend/executor/spi.c
@@ -1872,8 +1872,9 @@ spi_dest_startup(DestReceiver *self, int operation, =
TupleDesc typeinfo)
 	slist_push_head(&_SPI_current->tuptables, &tuptable->next);
 =

 	/* set up initial allocations */
-	tuptable->alloced =3D tuptable->free =3D 128;
+	tuptable->alloced =3D 128;
 	tuptable->vals =3D (HeapTuple *) palloc(tuptable->alloced * sizeof(HeapT=
uple));
+	tuptable->numvals =3D 0;
 	tuptable->tupdesc =3D CreateTupleDescCopy(typeinfo);
 =

 	MemoryContextSwitchTo(oldcxt);
@@ -1899,18 +1900,18 @@ spi_printtup(TupleTableSlot *slot, DestReceiver *s=
elf)
 =

 	oldcxt =3D MemoryContextSwitchTo(tuptable->tuptabcxt);
 =

-	if (tuptable->free =3D=3D 0)
+	if (tuptable->numvals >=3D tuptable->alloced)
 	{
 		/* Double the size of the pointer array */
-		tuptable->free =3D tuptable->alloced;
-		tuptable->alloced +=3D tuptable->free;
+		uint64		newalloced =3D tuptable->alloced * 2;
+
 		tuptable->vals =3D (HeapTuple *) repalloc_huge(tuptable->vals,
-													 tuptable->alloced * sizeof(HeapTuple));
+													 newalloced * sizeof(HeapTuple));
+		tuptable->alloced =3D newalloced;
 	}
 =

-	tuptable->vals[tuptable->alloced - tuptable->free] =3D
-		ExecCopySlotHeapTuple(slot);
-	(tuptable->free)--;
+	tuptable->vals[tuptable->numvals] =3D ExecCopySlotHeapTuple(slot);
+	(tuptable->numvals)++;
 =

 	MemoryContextSwitchTo(oldcxt);
 =

@@ -2324,8 +2325,7 @@ _SPI_execute_plan(SPIPlanPtr plan, ParamListInfo par=
amLI,
 =

 				/* Update "processed" if stmt returned tuples */
 				if (_SPI_current->tuptable)
-					_SPI_current->processed =3D _SPI_current->tuptable->alloced -
-						_SPI_current->tuptable->free;
+					_SPI_current->processed =3D _SPI_current->tuptable->numvals;
 =

 				res =3D SPI_OK_UTILITY;
 =

@@ -2694,7 +2694,7 @@ _SPI_checktuples(void)
 =

 	if (tuptable =3D=3D NULL)		/* spi_dest_startup was not called */
 		failed =3D true;
-	else if (processed !=3D (tuptable->alloced - tuptable->free))
+	else if (processed !=3D tuptable->numvals)
 		failed =3D true;
 =

 	return failed;
diff --git a/src/include/executor/spi.h b/src/include/executor/spi.h
index af4f8c8..ad69a5c 100644
--- a/src/include/executor/spi.h
+++ b/src/include/executor/spi.h
@@ -24,11 +24,11 @@ typedef struct SPITupleTable
 	/* Public members */
 	TupleDesc	tupdesc;		/* tuple descriptor */
 	HeapTuple  *vals;			/* array of tuples */
+	uint64		numvals;		/* number of valid tuples */
 =

 	/* Private members, not intended for external callers */
+	uint64		alloced;		/* allocated length of vals array */
 	MemoryContext tuptabcxt;	/* memory context of result table */
-	uint64		alloced;		/* # of alloced vals */
-	uint64		free;			/* # of free vals */
 	slist_node	next;			/* link for internal bookkeeping */
 	SubTransactionId subid;		/* subxact in which tuptable was created */
 } SPITupleTable;

------- =_aaaaaaaaaa0--