Subject: Re: Checking return value of SPI_execute
To: Alvaro Herrera <alvherre@2ndquadrant.com>,
 Pavel Stehule <pavel.stehule@gmail.com>
Cc: Michael Paquier <michael@paquier.xyz>,
 PostgreSQL-development <pgsql-hackers@postgresql.org>
References: <20191106151112.GA12251@alvherre.pgsql>
From: Mark Dilger <hornschnorter@gmail.com>
Message-ID: <1de13b09-a5a0-4c6e-0470-5ecebc0a7784@gmail.com>
Date: Thu, 7 Nov 2019 09:05:53 -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: <20191106151112.GA12251@alvherre.pgsql>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Precedence: bulk


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