Обсуждение: 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
2016-11-07 2:16 GMT+01:00 Tom Lane <tgl@sss.pgh.pa.us>:
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?
cannot be there some performance impacts?
Regards
Pavel
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
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
2016-11-07 15:47 GMT+01:00 Tom Lane <tgl@sss.pgh.pa.us>:
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.
ok
Pavel
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