Memory allocation in spi_printtup()

Started by Neil Conwayover 10 years ago4 messages
#1Neil Conway
neil.conway@gmail.com
1 attachment(s)

spi_printtup() has the following code (spi.c:1798):

if (tuptable->free == 0)
{
tuptable->free = 256;
tuptable->alloced += tuptable->free;
tuptable->vals = (HeapTuple *) repalloc(tuptable->vals,

tuptable->alloced * sizeof(HeapTuple));
}

i.e., it grows the size of the tuptable by a fixed amount when it runs
out of space. That seems odd; doubling the size of the table would be
more standard. Does anyone know if there is a rationale for this
behavior?

Attached is a one-liner to double the size of the table when space is
exhausted. Constructing a simple test case in which a large result is
materialized via SPI_execute() (e.g., by passing two large queries to
crosstab() from contrib/tablefunc), this change reduces the time
required to exceed the palloc size limit from ~300 seconds to ~93
seconds on my laptop.

Of course, using SPI_execute() rather than cursors for queries with
large result sets is not a great idea, but there is demonstrably code
that does this (e.g., contrib/tablefunc -- I'll send a patch for that
shortly).

Neil

Attachments:

spi-printtup-alloc-1.patchtext/x-patch; charset=US-ASCII; name=spi-printtup-alloc-1.patchDownload
diff --git a/src/backend/executor/spi.c b/src/backend/executor/spi.c
index d544ad9..2573b3f 100644
--- a/src/backend/executor/spi.c
+++ b/src/backend/executor/spi.c
@@ -1797,7 +1797,7 @@ spi_printtup(TupleTableSlot *slot, DestReceiver *self)
 
 	if (tuptable->free == 0)
 	{
-		tuptable->free = 256;
+		tuptable->free = tuptable->alloced;
 		tuptable->alloced += tuptable->free;
 		tuptable->vals = (HeapTuple *) repalloc(tuptable->vals,
 									  tuptable->alloced * sizeof(HeapTuple));
#2Tom Lane
tgl@sss.pgh.pa.us
In reply to: Neil Conway (#1)
Re: Memory allocation in spi_printtup()

Neil Conway <neil.conway@gmail.com> writes:

Hi Neil! Long time no see.

spi_printtup() has the following code (spi.c:1798):
if (tuptable->free == 0)
{
tuptable->free = 256;
tuptable->alloced += tuptable->free;
tuptable->vals = (HeapTuple *) repalloc(tuptable->vals,
tuptable->alloced * sizeof(HeapTuple));
}

i.e., it grows the size of the tuptable by a fixed amount when it runs
out of space. That seems odd; doubling the size of the table would be
more standard. Does anyone know if there is a rationale for this
behavior?

Seems like it must be just legacy code. We're only allocating pointers
here; the actual tuples will likely be significantly larger. So there's
not a lot of reason not to use the normal doubling rule.

Attached is a one-liner to double the size of the table when space is
exhausted.

I think this could use a comment, but otherwise seems OK.

Should we back-patch this change? Seems like it's arguably a
performance bug.

regards, tom lane

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

#3Neil Conway
neil.conway@gmail.com
In reply to: Tom Lane (#2)
1 attachment(s)
Re: Memory allocation in spi_printtup()

On Mon, Aug 17, 2015 at 7:56 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Hi Neil! Long time no see.

Likewise :)

Attached is a one-liner to double the size of the table when space is
exhausted.

I think this could use a comment, but otherwise seems OK.

Attached is a revised patch with a comment.

Should we back-patch this change? Seems like it's arguably a
performance bug.

Sounds good to me.

Neil

Attachments:

spi-printtup-alloc-2.patchtext/x-patch; charset=US-ASCII; name=spi-printtup-alloc-2.patchDownload
diff --git a/src/backend/executor/spi.c b/src/backend/executor/spi.c
index d544ad9..05a2b21 100644
--- a/src/backend/executor/spi.c
+++ b/src/backend/executor/spi.c
@@ -1797,7 +1797,8 @@ spi_printtup(TupleTableSlot *slot, DestReceiver *self)
 
 	if (tuptable->free == 0)
 	{
-		tuptable->free = 256;
+		/* Double the size of the table */
+		tuptable->free = tuptable->alloced;
 		tuptable->alloced += tuptable->free;
 		tuptable->vals = (HeapTuple *) repalloc(tuptable->vals,
 									  tuptable->alloced * sizeof(HeapTuple));
#4Tom Lane
tgl@sss.pgh.pa.us
In reply to: Neil Conway (#3)
Re: Memory allocation in spi_printtup()

Neil Conway <neil.conway@gmail.com> writes:

On Mon, Aug 17, 2015 at 7:56 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Should we back-patch this change? Seems like it's arguably a
performance bug.

Sounds good to me.

Committed and back-patched.

regards, tom lane

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers