Обсуждение: Let's get rid of SPI_push/SPI_pop

The intent of SPI_push/SPI_pop seems to be to draw a boundary line between
nested layers of SPI callers.  Which is fine, but the SPI_connect and
SPI_finish calls of the inner layer would suffice for that.  AFAICS,
the only thing that SPI_push/SPI_pop buy for us is the ability to detect
a missing SPI_connect or SPI_finish in an inner function layer.  And
that seems pretty useless, because any such bug in a function would be
immediately detected in simple testing that calls it without any outer
level of SPI calls.

As against that, we have the risk of forgotten SPI_push/SPI_pop calls that
go undetected for years, as just seen in commit fc8b81a29.  We've had that
type of bug before too, cf 0d4899e44.  And then there's the fact that we
put conditional SPI_push/SPI_pop calls into various places, eg deac9488d,
which it seems to me largely destroys whatever debugging value the concept
did have.

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.)

We can provide do-nothing macros by these names to avoid an API break
for third-party extensions.

Comments, objections?
        regards, tom lane

Pavel Stehule <pavel.stehule@gmail.com> writes:
> 2016-11-07 2:16 GMT+01:00 Tom Lane <tgl@sss.pgh.pa.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.
        regards, tom lane

I wrote:
> Pavel Stehule <pavel.stehule@gmail.com> writes:
>> 2016-11-07 2:16 GMT+01:00 Tom Lane <tgl@sss.pgh.pa.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_copytupleSPI_returntupleSPI_modifytupleSPI_pallocSPI_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