Re: Checking return value of SPI_execute

st 6. 11. 2019 v 16:38 odesílatel Mark Dilger <hornschnorter(at)gmail(dot)com>
napsal:

>
>
> On 11/5/19 9:54 PM, Pavel Stehule wrote:
> >
> >
> > st 6. 11. 2019 v 5:28 odesílatel Michael Paquier <michael(at)paquier(dot)xyz
> > <mailto:michael(at)paquier(dot)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?
>

I am not against these fix.

Regards

Pavel

>
> Mark Dilger
>