Subject: Re: Checking return value of SPI_execute
To: Michael Paquier <michael@paquier.xyz>
Cc: PostgreSQL-development <pgsql-hackers@postgresql.org>
References: <17d30445-4862-7917-170f-84328dcd292d@gmail.com>
 <20191106042757.GE1604@paquier.xyz>
From: Mark Dilger <hornschnorter@gmail.com>
Message-ID: <4bc8eca7-6efa-8174-a8cd-69167f233f6d@gmail.com>
Date: Wed, 6 Nov 2019 07:35:18 -0800
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101
 Thunderbird/60.7.1
MIME-Version: 1.0
In-Reply-To: <20191106042757.GE1604@paquier.xyz>
Content-Type: text/plain; charset=windows-1252; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Precedence: bulk


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