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

classic Classic list List threaded Threaded
7 messages Options
Reply | Threaded
Open this post in threaded view
|

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

Mark Dilger-4
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.




Reply | Threaded
Open this post in threaded view
|

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

Tom Lane-2
Mark Dilger <[hidden email]> 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


Reply | Threaded
Open this post in threaded view
|

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

Mark Dilger-4
On Fri, May 17, 2019 at 6:12 PM Tom Lane <[hidden email]> wrote:

>
> Mark Dilger <[hidden email]> 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


Reply | Threaded
Open this post in threaded view
|

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

Tom Lane-2
Mark Dilger <[hidden email]> writes:
> On Fri, May 17, 2019 at 6:12 PM Tom Lane <[hidden email]> 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


Reply | Threaded
Open this post in threaded view
|

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

Mark Dilger-4
On Wed, May 22, 2019 at 1:52 PM Tom Lane <[hidden email]> wrote:

>
> Mark Dilger <[hidden email]> writes:
> > On Fri, May 17, 2019 at 6:12 PM Tom Lane <[hidden email]> 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


Reply | Threaded
Open this post in threaded view
|

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

Tom Lane-2
In reply to this post by Tom Lane-2
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


Reply | Threaded
Open this post in threaded view
|

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

Alvaro Herrera-9
In reply to this post by Mark Dilger-4
On 2019-May-22, Mark Dilger wrote:

> On Wed, May 22, 2019 at 1:52 PM Tom Lane <[hidden email]> 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