st 6. 11. 2019 v 8:56 odesílatel Michael Paquier <michael@paquier.xyz> 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.