Обсуждение: Is it safe to ignore the return value of SPI_finish and SPI_execute?
Hackers, 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. Examples can be found in: src/backend/utils/adt/xml.c src/backend/utils/adt/tsvector_op.c src/backend/utils/adt/tsquery_rewrite.c src/test/regress/regress.c contrib/spi/refint.c The return value of SPI_execute is ignored in one spot: src/backend/utils/adt/xml.c circa line 2465. I checked the archives and did not see any discussion about this in the past. Please excuse me if this has been asked before.
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
On Fri, May 17, 2019 at 6:12 PM Tom Lane <tgl@sss.pgh.pa.us> wrote: > > 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. Agreed. > 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. Does this proposal of yours seem good enough for me to make a patch based on this design? > 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. I would like to clean this up and submit a patch, so long as the general solution seems acceptable to the pgsql-hackers list. Just as background information: I only hit this issue because I have been auditing the version 12 code and adding __attribute__((warn_unused_result)) on non-void functions in the tree and then checking each one that gets compiler warnings to see if there is a bug inherent in the way it is being used. These SPI_* functions are the first ones I found where it seemed clearly wrong to me that the return values were being ignored. There have been many others where ignoring the return value seemed acceptable given the way the function is designed to work, and though I am not always happy with the design, I'm not trying to go so far as redesigning large sections of the code. > > 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. mark
Mark Dilger <hornschnorter@gmail.com> writes:
> On Fri, May 17, 2019 at 6:12 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> 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.
> Does this proposal of yours seem good enough for me to make a patch
> based on this design?
Just to clarify --- I think what's being discussed here is "change some
large fraction of the SPI functions that can return SPI_ERROR_xxx error
codes to throw elog/ereport(ERROR) instead". Figuring out what fraction
that should be is part of the work --- but just in a quick scan through
spi.c, it seems like there might be a case for deprecating practically
all the SPI_ERROR_xxx codes except for SPI_ERROR_NOATTRIBUTE.
I'd definitely argue that SPI_ERROR_UNCONNECTED and SPI_ERROR_ARGUMENT
deserve that treatment.
I'm for it, if you want to do the work, but I don't speak for everybody.
It's not entirely clear to me whether we ought to change the return
convention to be "returns void" or make it "always returns SPI_OK"
for those functions where the return code becomes trivial. The
latter would avoid churn for external modules, but it seems not to
have much other attractiveness.
regards, tom lane
On Wed, May 22, 2019 at 1:52 PM Tom Lane <tgl@sss.pgh.pa.us> wrote: > > Mark Dilger <hornschnorter@gmail.com> writes: > > On Fri, May 17, 2019 at 6:12 PM Tom Lane <tgl@sss.pgh.pa.us> wrote: > >> 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. > > > Does this proposal of yours seem good enough for me to make a patch > > based on this design? > > Just to clarify --- I think what's being discussed here is "change some > large fraction of the SPI functions that can return SPI_ERROR_xxx error > codes to throw elog/ereport(ERROR) instead". Yes, I was talking about that, but was ambiguous in how I phrased my question. > Figuring out what fraction > that should be is part of the work --- but just in a quick scan through > spi.c, it seems like there might be a case for deprecating practically > all the SPI_ERROR_xxx codes except for SPI_ERROR_NOATTRIBUTE. > I'd definitely argue that SPI_ERROR_UNCONNECTED and SPI_ERROR_ARGUMENT > deserve that treatment. > > I'm for it, if you want to do the work, but I don't speak for everybody. I do want to write the patch, but I'll wait for other opinions. mark
I wrote:
> It's not entirely clear to me whether we ought to change the return
> convention to be "returns void" or make it "always returns SPI_OK"
> for those functions where the return code becomes trivial. The
> latter would avoid churn for external modules, but it seems not to
> have much other attractiveness.
Further thought about that --- it's clearly better in the long run
if we switch to "returns void" where possible. We don't want to
encourage people to waste code space on dead error checks. However,
doing that could be quite a PITA for people who are trying to maintain
extension code that works across multiple backend versions.
We could address that by providing compatibility macros, say per
this sketch:
extern void SPI_finish(void);
...
#ifdef BACKWARDS_COMPATIBLE_SPI_CALLS
#define SPI_finish() (SPI_finish(), SPI_OK_FINISH)
...
#endif
(This relies on non-recursive macro expansion, but that's been
standard since C89.)
The #ifdef stanza could be ripped out someday when all older branches
are out of support, but there wouldn't be any hurry.
regards, tom lane
On 2019-May-22, Mark Dilger wrote: > On Wed, May 22, 2019 at 1:52 PM Tom Lane <tgl@sss.pgh.pa.us> wrote: > > Figuring out what fraction > > that should be is part of the work --- but just in a quick scan through > > spi.c, it seems like there might be a case for deprecating practically > > all the SPI_ERROR_xxx codes except for SPI_ERROR_NOATTRIBUTE. > > I'd definitely argue that SPI_ERROR_UNCONNECTED and SPI_ERROR_ARGUMENT > > deserve that treatment. > > > > I'm for it, if you want to do the work, but I don't speak for everybody. > > I do want to write the patch, but I'll wait for other opinions. In my perusal, the SPI API is unnecessarily baroque and could stand some simplification, so +1 for the proposed approach. -- Álvaro Herrera https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services