Mark Dilger <hornschnorter@gmail.com> writes:
> most places that use SPI_connect ... SPI_finish check the
> return value of SPI_finish and elog if it failed. There
> are a few places that do not, and it is unclear to me
> why this is safe. SPI_finish appears to be needed to
> clean up memory contexts.
Well, looking through spi.c, the only failure return that SPI_finish
actually has right now is from _SPI_begin_call:
if (_SPI_current == NULL)
return SPI_ERROR_UNCONNECTED;
and if you're willing to posit that those callers did call SPI_connect,
that's unreachable for them. The more interesting cases such as
failure within memory context cleanup would throw elog/ereport, so
they're not at issue here.
But I agree that not checking is crap coding practice, because there is
certainly no reason why SPI_finish could not have other error-return
cases in future.
One reasonable solution would be to change the callers that got this
wrong. Another one would be to reconsider whether the error-return-code
convention makes any sense at all here. If we changed the above-quoted
bit to be an ereport(ERROR), then we could say that SPI_finish either
returns 0 or throws error, making it moot whether callers check, and
allowing removal of now-useless checks from all the in-core callers.
I don't think that actually doing that would be a great idea unless
we went through all of the SPI functions and did it for every "unexpected"
error case. Is it worth the trouble? Maybe, but I don't wanna do
the legwork.
> The return value of SPI_execute is ignored in one spot:
> src/backend/utils/adt/xml.c circa line 2465.
That seems like possibly a real bug. It's certainly poor practice
as things stand.
regards, tom lane