Re: Checking return value of SPI_execute

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