Re: Checking return value of SPI_execute

[Alvaro Herrera]

On 2019-Nov-07, Mark Dilger wrote:

> 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?

Yes, I think it does.


-- 
Álvaro Herrerahttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: Checking return value of SPI_execute

[Mark Dilger]

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

Re: Checking return value of SPI_execute

[Michael Paquier]

On Wed, Nov 06, 2019 at 07:35:18AM -0800, Mark Dilger wrote:
> 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.

The code relies on SELECT queries now to fetch a list of relation
OIDs and it is read-only.  If it happens that another query type makes
sense for this code path, then the person using the routine will need
to think about what to do when seeing the new error.  The current code
exists for ages, so I have applied your change only on HEAD.
--
Michael


signature.asc
Description: PGP signature

Re: Checking return value of SPI_execute

[Pavel Stehule]

st 6. 11. 2019 v 16:38 odesílatel Mark Dilger 
napsal:

>
>
> On 11/5/19 9:54 PM, Pavel Stehule wrote:
> >
> >
> > st 6. 11. 2019 v 5:28 odesílatel Michael Paquier  > > 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
>

Re: Checking return value of SPI_execute

[Mark Dilger]

On 11/5/19 9:54 PM, Pavel Stehule wrote:



st 6. 11. 2019 v 5:28 odesílatel Michael Paquier > 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

Re: Checking return value of SPI_execute

[Mark Dilger]

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

Re: Checking return value of SPI_execute

[Alvaro Herrera]

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.

-- 
Álvaro Herrerahttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: Checking return value of SPI_execute

[Pavel Stehule]

st 6. 11. 2019 v 8:56 odesílatel Michael Paquier 
napsal:

> On Wed, Nov 06, 2019 at 06:54:16AM +0100, Pavel Stehule wrote:
> > 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 not sure what you are proposing here, nor am I sure to what kind
> of assertions you are referring to in spi.c.  If we were to change the
> error reporting, what of the external and existing consumers of this
> routine?  They would not expect to bump on an exception and perhaps
> need to handle error code paths by themselves, no?
>

> Anyway, any callers of SPI_execute() (tablefunc.c, matview.c) we have
> now in-core react based on a status or a set of statuses they expect,
> so based on that fixing this caller in xml.c sounds fine to me.
>

This fix is correct.

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.

Pavel

> --
> Michael
>

Re: Checking return value of SPI_execute

[Michael Paquier]

On Wed, Nov 06, 2019 at 06:54:16AM +0100, Pavel Stehule wrote:
> 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 not sure what you are proposing here, nor am I sure to what kind
of assertions you are referring to in spi.c.  If we were to change the
error reporting, what of the external and existing consumers of this
routine?  They would not expect to bump on an exception and perhaps
need to handle error code paths by themselves, no?

Anyway, any callers of SPI_execute() (tablefunc.c, matview.c) we have
now in-core react based on a status or a set of statuses they expect,
so based on that fixing this caller in xml.c sounds fine to me.
--
Michael


signature.asc
Description: PGP signature

Re: Checking return value of SPI_execute

[Pavel Stehule]

st 6. 11. 2019 v 5:28 odesílatel Michael Paquier 
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?

Pavel



--
> Michael
>

Re: Checking return value of SPI_execute

[Michael Paquier]

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.
--
Michael


signature.asc
Description: PGP signature

Checking return value of SPI_execute

[Mark Dilger]

Hackers,

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.

Mark Dilger

[1] https://www.postgresql.org/message-id/24753.1558141935%40sss.pgh.pa.us
>From 5c4013e41fbe212e41116509c54a032e1b9ebc0d Mon Sep 17 00:00:00 2001
From: Mark Dilger 
Date: Tue, 5 Nov 2019 16:40:58 -0800
Subject: [PATCH v1] Checking return value of SPI_execute.

In query_to_oid_list, the return code from SPI_execute was ignored.  I
know of no case where this manifests as a live bug, since the query
strings passed into query_to_oid_list appear to always succeed (and are
not user supplied).  Even so, ignoring the return code seems poor form
and could be a source of bugs if surrounding code were to change.

See discussion with Tom Lane near the end of
https://www.postgresql.org/message-id/24753.1558141935%40sss.pgh.pa.us
---
 src/backend/utils/adt/xml.c | 5 -
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/src/backend/utils/adt/xml.c b/src/backend/utils/adt/xml.c
index 3a493dd6bf..ff0577fc0b 100644
--- a/src/backend/utils/adt/xml.c
+++ b/src/backend/utils/adt/xml.c
@@ -2461,8 +2461,11 @@ query_to_oid_list(const char *query)
 {
 	uint64		i;
 	List	   *list = NIL;
+	int			spi_result;
 
-	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));
 
 	for (i = 0; i < SPI_processed; i++)
 	{
-- 
2.20.1