Re: Is it safe to ignore the return value of SPI_finish and SPI_execute?

On Fri, May 17, 2019 at 6:12 PM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>
> Mark Dilger <hornschnorter(at)gmail(dot)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