Thread: Checking return value of SPI_execute

Hackers,

please find attached a patch fixing a problem previously discussed [1] 
about the code inappropriately ignoring the return value from SPI_execute.

I will be adding this to https://commitfest.postgresql.org/26/ shortly.

Mark Dilger

[1] https://www.postgresql.org/message-id/24753.1558141935%40sss.pgh.pa.us

On Tue, Nov 05, 2019 at 05:21:25PM -0800, Mark Dilger wrote:
> please find attached a patch fixing a problem previously discussed [1] about
> the code inappropriately ignoring the return value from SPI_execute.
>
> I will be adding this to https://commitfest.postgresql.org/26/
> shortly.

Yes, this should be fixed.

> -    SPI_execute(query, true, 0);
> +    spi_result = SPI_execute(query, true, 0);
> +    if (spi_result < 0)
> +        elog(ERROR, "SPI_execute returned %s", SPI_result_code_string(spi_result));

Any queries processed in xml.c are plain SELECT queries, so it seems
to me that you need to check after SPI_OK_SELECT as only valid
result.
--
Michael

On Wed, Nov 06, 2019 at 06:54:16AM +0100, Pavel Stehule wrote:
> Is generic question if this exception should not be raised somewhere in
> spi.c - maybe at SPI_execute.
>
> When you look to SPI_execute_plan, then checked errors has a character +/-
> assertions. All SQL errors are ended by a exception. This API is not too
> consistent after years what is used.
>
> I agree so this result code should be tested for better code quality. But
> this API is not consistent now, and should be refactored to use a
> exceptions instead result codes. Or instead error checking, a assertions
> should be used.
>
> What do you think about it?

I am not sure what you are proposing here, nor am I sure to what kind
of assertions you are referring to in spi.c.  If we were to change the
error reporting, what of the external and existing consumers of this
routine?  They would not expect to bump on an exception and perhaps
need to handle error code paths by themselves, no?

Anyway, any callers of SPI_execute() (tablefunc.c, matview.c) we have
now in-core react based on a status or a set of statuses they expect,
so based on that fixing this caller in xml.c sounds fine to me.
--
Michael

On 2019-Nov-06, Pavel Stehule wrote:

> My comment was about maybe obsolescence of this API. Probably it was
> designed before exception introduction.
> 
> For example - syntax error is ended by exception. Wrong numbers of argument
> is signalized by error status. I didn't study this code, but maybe was much
> more effective to raise exceptions inside SPI instead return status code.
> These errors are finished by exceptions, but these exceptions coming from
> different places. For me it looks strange, if some functions returns error
> status, but can be ended by exception too.

Yeah, I think I'd rather have more status codes and less exceptions,
than the other way around.  The problem with throwing exceptions for
every kind of error is that we don't allow exceptions to be caught (per
project policy) except to be rethrown.  It seems like for errors where
the SPI code can clean up its own resources (free memory, close portals
etc), it should do such cleanup then return SPI_SYNTAX_ERROR or whatever
and the caller can decide whether to turn this into an exception or
handle in a different way; whereas for exceptions thrown by callees (say
OOM) it would just propagate the exception.  This mean callers are
forced into adding code to check for return codes, but it allows more
flexibility.

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On 11/5/19 8:27 PM, Michael Paquier wrote:
> On Tue, Nov 05, 2019 at 05:21:25PM -0800, Mark Dilger wrote:
>> please find attached a patch fixing a problem previously discussed [1] about
>> the code inappropriately ignoring the return value from SPI_execute.
>>
>> I will be adding this to https://commitfest.postgresql.org/26/
>> shortly.
> 
> Yes, this should be fixed.
> 
>> -    SPI_execute(query, true, 0);
>> +    spi_result = SPI_execute(query, true, 0);
>> +    if (spi_result < 0)
>> +        elog(ERROR, "SPI_execute returned %s", SPI_result_code_string(spi_result));
> 
> Any queries processed in xml.c are plain SELECT queries, so it seems
> to me that you need to check after SPI_OK_SELECT as only valid
> result.

Other code that checks the return value from an SPI function is 
inconsistent about whether it checks for SPI_OK_SELECT or simply checks 
for a negative result.  I was on the fence about which precedent to 
follow, and was just slightly in favor of testing for negative rather 
than SPI_OK_SELECT due to this function, query_to_oid_list, taking the 
query string as an argument and not controlling whether that argument is 
indeed a plain SELECT.

I don't feel strongly about it.

Mark Dilger

On 11/5/19 9:54 PM, Pavel Stehule wrote:
> 
> 
> st 6. 11. 2019 v 5:28 odesílatel Michael Paquier <michael@paquier.xyz 
> <mailto:michael@paquier.xyz>> napsal:
> 
>     On Tue, Nov 05, 2019 at 05:21:25PM -0800, Mark Dilger wrote:
>      > please find attached a patch fixing a problem previously
>     discussed [1] about
>      > the code inappropriately ignoring the return value from SPI_execute.
>      >
>      > I will be adding this to https://commitfest.postgresql.org/26/
>      > shortly.
> 
>     Yes, this should be fixed.
> 
>      > -     SPI_execute(query, true, 0);
>      > +     spi_result = SPI_execute(query, true, 0);
>      > +     if (spi_result < 0)
>      > +             elog(ERROR, "SPI_execute returned %s",
>     SPI_result_code_string(spi_result));
> 
>     Any queries processed in xml.c are plain SELECT queries, so it seems
>     to me that you need to check after SPI_OK_SELECT as only valid
>     result.
> 
> 
> Is generic question if this exception should not be raised somewhere in 
> spi.c - maybe at SPI_execute
> 
> When you look to SPI_execute_plan, then checked errors has a character 
> +/- assertions. All SQL errors are ended by a exception. This API is not 
> too consistent after years what is used.
> 
> I agree so this result code should be tested for better code quality. 
> But this API is not consistent now, and should be refactored to use a 
> exceptions instead result codes. Or instead error checking, a assertions 
> should be used.
> 
> What do you think about it?

I am creating another patch which removes most of the error codes from 
the interface and uses elog(ERROR) or ereport(ERROR) instead, but I 
anticipate a lot of debate about that design and wanted to get this 
simpler patch into the queue.  I don't think we need to reject this 
patch in favor of redesigning the entire SPI API.  Instead, we can apply 
this patch as a simple bug fix, and then if it gets removed later when 
the other, larger patch is committed, so be it.

Does that plan seem acceptable?

Mark Dilger

On Wed, Nov 06, 2019 at 07:35:18AM -0800, Mark Dilger wrote:
> Other code that checks the return value from an SPI function is inconsistent
> about whether it checks for SPI_OK_SELECT or simply checks for a negative
> result.  I was on the fence about which precedent to follow, and was just
> slightly in favor of testing for negative rather than SPI_OK_SELECT due to
> this function, query_to_oid_list, taking the query string as an argument and
> not controlling whether that argument is indeed a plain SELECT.
>
> I don't feel strongly about it.

The code relies on SELECT queries now to fetch a list of relation
OIDs and it is read-only.  If it happens that another query type makes
sense for this code path, then the person using the routine will need
to think about what to do when seeing the new error.  The current code
exists for ages, so I have applied your change only on HEAD.
--
Michael

On 11/6/19 7:11 AM, Alvaro Herrera wrote:
> On 2019-Nov-06, Pavel Stehule wrote:
> 
>> My comment was about maybe obsolescence of this API. Probably it was
>> designed before exception introduction.
>>
>> For example - syntax error is ended by exception. Wrong numbers of argument
>> is signalized by error status. I didn't study this code, but maybe was much
>> more effective to raise exceptions inside SPI instead return status code.
>> These errors are finished by exceptions, but these exceptions coming from
>> different places. For me it looks strange, if some functions returns error
>> status, but can be ended by exception too.
> 
> Yeah, I think I'd rather have more status codes and less exceptions,
> than the other way around.  The problem with throwing exceptions for
> every kind of error is that we don't allow exceptions to be caught (per
> project policy) except to be rethrown.  It seems like for errors where
> the SPI code can clean up its own resources (free memory, close portals
> etc), it should do such cleanup then return SPI_SYNTAX_ERROR or whatever
> and the caller can decide whether to turn this into an exception or
> handle in a different way; whereas for exceptions thrown by callees (say
> OOM) it would just propagate the exception.  This mean callers are
> forced into adding code to check for return codes, but it allows more
> flexibility.
> 

I like to distinguish between (a) errors that can happen when a well 
written bit of C code passes possibly bad SQL through SPI, and (b) 
errors that can only happen when SPI is called from a poorly written C 
program.

Examples of (a) are SPI_ERROR_COPY and SPI_ERROR_TRANSACTION, which can 
both happen from disallowed actions within a plpgsql function.

An example of (b) is SPI_ERROR_PARAM, which only gets returned if the 
caller passed into SPI a plan which has nargs > 0 but then negligently 
passed in NULL for the args and/or argtypes.

I'd like to keep the status codes for (a) but deprecate error codes for 
(b) in favor of elog(ERROR).  I don't see that these elogs should ever 
be a problem, since getting one in testing would indicate the need to 
fix bad C code, not the need to catch an exception and take remedial 
action at run time.  Does this adequately address your concern?

My research so far indicates that most return codes are either totally 
unused or of type (b), with only a few of type (a).

-- 
Mark Dilger

On 2019-Nov-07, Mark Dilger wrote:

> I'd like to keep the status codes for (a) but deprecate error codes for (b)
> in favor of elog(ERROR).  I don't see that these elogs should ever be a
> problem, since getting one in testing would indicate the need to fix bad C
> code, not the need to catch an exception and take remedial action at run
> time.  Does this adequately address your concern?

Yes, I think it does.


-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services