From: Mark Dilger <hornschnorter@gmail.com>
To: Pavel Stehule <pavel.stehule@gmail.com>
To: Michael Paquier <michael@paquier.xyz>
Cc: PostgreSQL-development <pgsql-hackers@postgresql.org>
Subject: Re: Checking return value of SPI_execute
Date: Wed, 6 Nov 2019 07:38:27 -0800
Message-ID: <2c4d660d-fbde-abb3-8ec9-7f417dbd0f41@gmail.com> (raw)
In-Reply-To: <CAFj8pRAuzJNOCm3SCu2oZBMKKN0A8KaRhk+Sn2jUeYo_LDpE_g@mail.gmail.com>
References: <17d30445-4862-7917-170f-84328dcd292d@gmail.com>
<20191106042757.GE1604@paquier.xyz>
<CAFj8pRAuzJNOCm3SCu2oZBMKKN0A8KaRhk+Sn2jUeYo_LDpE_g@mail.gmail.com>
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
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: hornschnorter@gmail.com, pavel.stehule@gmail.com, michael@paquier.xyz
Subject: Re: Checking return value of SPI_execute
In-Reply-To: <2c4d660d-fbde-abb3-8ec9-7f417dbd0f41@gmail.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox