Re: Let's get rid of SPI_push/SPI_pop

I wrote:
> Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com> writes:
>> 2016-11-07 2:16 GMT+01:00 Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>:
>>> So I think we should just delete these functions and adjust SPI_connect
>>> and SPI_finish so that they just push/pop a context level unconditionally.
>>> (Which will make them simpler, not more complicated.)

>> cannot be there some performance impacts?

> Any impact will be for the better, since we're removing logic.
> I haven't gone through it in detail, but it might be as simple
> as removing _SPI_curid and code that manipulates it.

After studying spi.c more closely, I find that it can't be quite as
simple as that. There are five functions that look at _SPI_curid
to determine where they will palloc their results:
SPI_copytuple
SPI_returntuple
SPI_modifytuple
SPI_palloc
SPI_datumTransfer
Specifically, when used in a normal SPI execution context, they
will palloc in the "savedcxt", the one that had been current when
SPI_connect was called; this behavior is used for returning values
out of a SPI-using function. However, when not in a SPI context,
they just allocate in CurrentMemoryContext. So their behavior is
different after SPI_push than before it.

The majority of call sites use these functions while connected to SPI,
so that it wouldn't change things if we redefined these functions
to insist on being in SPI context and always use the "savedcxt".
However, there are about half a dozen places that use SPI_modifytuple
while not connected. Some of them never do connect at all, and
some use it after disconnecting.

I don't think that this discovery constitutes a reason not to get rid of
SPI_push/SPI_pop. What it shows is that forgetting to call them actually
has another, subtler way of causing bugs. If you forget, and call some
code that is using one of these functions this way and expecting to
produce a result in CurrentMemoryContext, it won't produce the result
there but in the context of the caller of your SPI-using procedure.
That probably means the memory will get leaked until you return, which
could be quite a long time (consider a long-running plpgsql function
that repeatedly calls something that acts like this).

So I think that we should still get rid of SPI_push/SPI_pop, and to that
end, redefine these five functions to throw error outside SPI context.

To simplify adapting the places that use SPI_modifytuple outside SPI
context, we could provide a function with a similar API that allocates
in CurrentMemoryContext. I have not found any caller that really needs
a run-time determination of which memory context to use.

regards, tom lane