Further hacking on SPITupleTable struct - Mailing list pgsql-hackers

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

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 pass to
    SPI functions dealing with rows.
   </para>
@@ -4631,12 +4635,12 @@ execq(PG_FUNCTION_ARGS)
      */
     if (ret > 0 && SPI_tuptable != NULL)
     {
-        TupleDesc tupdesc = SPI_tuptable->tupdesc;
         SPITupleTable *tuptable = SPI_tuptable;
+        TupleDesc tupdesc = tuptable->tupdesc;
         char buf[8192];
         uint64 j;

-        for (j = 0; j < proc; j++)
+        for (j = 0; j < tuptable->numvals; j++)
         {
             HeapTuple tuple = tuptable->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 = tuptable->free = 128;
+    tuptable->alloced = 128;
     tuptable->vals = (HeapTuple *) palloc(tuptable->alloced * sizeof(HeapTuple));
+    tuptable->numvals = 0;
     tuptable->tupdesc = CreateTupleDescCopy(typeinfo);

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

     oldcxt = MemoryContextSwitchTo(tuptable->tuptabcxt);

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

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

     MemoryContextSwitchTo(oldcxt);

@@ -2324,8 +2325,7 @@ _SPI_execute_plan(SPIPlanPtr plan, ParamListInfo paramLI,

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

                 res = SPI_OK_UTILITY;

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

     if (tuptable == NULL)        /* spi_dest_startup was not called */
         failed = true;
-    else if (processed != (tuptable->alloced - tuptable->free))
+    else if (processed != tuptable->numvals)
         failed = 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;