pg.ddx.io pgsql-hackers@postgresql.org mailing list archivehelp / color / mirror / Atom feed
Further hacking on SPITupleTable struct 2+ messages / 2 participants [nested] [flat]
Commits from this thread: 2019-07-18 bc8393cf27 Further adjust SPITupleTable to provide a public row-count field.
* Further hacking on SPITupleTable struct @ 2019-07-17 20:35 Tom Lane <tgl@sss.pgh.pa.us> 2019-07-19 12:11 ` Re: Further hacking on SPITupleTable struct Daniel Gustafsson <daniel@yesql.se> 0 siblings, 1 reply; 2+ messages in thread From: Tom Lane @ 2019-07-17 20:35 UTC (permalink / raw) To: pgsql-hackers@lists.postgresql.org; +Cc: Daniel Gustafsson <daniel@yesql.se> 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 Attachments:^ permalink raw reply [nested|flat] 2+ messages in threadprovide-public-numvals-field.patch (text/x-diff, 5.0K) download
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;
* Re: Further hacking on SPITupleTable struct 2019-07-17 20:35 Further hacking on SPITupleTable struct Tom Lane <tgl@sss.pgh.pa.us> @ 2019-07-19 12:11 ` Daniel Gustafsson <daniel@yesql.se> 0 siblings, 0 replies; 2+ messages in thread From: Daniel Gustafsson @ 2019-07-19 12:11 UTC (permalink / raw) To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: pgsql-hackers@lists.postgresql.org > On 17 Jul 2019, at 22:35, Tom Lane <tgl@sss.pgh.pa.us> wrote: > > 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. Sorry for being slow to return to this, I see you have already committed it. FWIW, I do agree that this makes a lot more sense. Retroactively +1’ing it. Regarding the core code I agree that no callers directly benefit without some refactoring, but contrib/xml2/xpath.c has one case which seems applicable as per the attached. Now, since contrib/xml2 has been deprecated for a long time it’s probably not worth bothering, but it was the one case I found so I figured I’d record it in this thread. cheers ./daniel Attachments:^ permalink raw reply [nested|flat] 2+ messages in threadxpath_numvals.diff (application/octet-stream, 809B) download
diff --git a/contrib/xml2/xpath.c b/contrib/xml2/xpath.c index 1e5b71d9a0..445b97ad7a 100644 --- a/contrib/xml2/xpath.c +++ b/contrib/xml2/xpath.c @@ -553,7 +553,6 @@ xpath_table(PG_FUNCTION_ARGS) int numpaths; int ret; - uint64 proc; int j; int rownr; /* For issuing multiple rows from one original * document */ @@ -662,7 +661,6 @@ xpath_table(PG_FUNCTION_ARGS) elog(ERROR, "xpath_table: SPI execution failed for query %s", query_buf.data); - proc = SPI_processed; tuptable = SPI_tuptable; spi_tupdesc = tuptable->tupdesc; @@ -692,7 +690,7 @@ xpath_table(PG_FUNCTION_ARGS) /* For each row i.e. document returned from SPI */ uint64 i; - for (i = 0; i < proc; i++) + for (i = 0; i < tuptable->numvals; i++) { char *pkey; char *xmldoc;
end of thread, other threads:[~2019-07-19 12:11 UTC | newest] Thread overview: 2+ messages (download: mbox mbox.gz follow: Atom feed) -- links below jump to the message on this page -- 2019-07-17 20:35 Further hacking on SPITupleTable struct Tom Lane <tgl@sss.pgh.pa.us> 2019-07-19 12:11 ` Daniel Gustafsson <daniel@yesql.se>
This inbox is served by DDX for PostgreSQL; see mirroring instructions for how to clone and mirror all data and code used for this inbox