SPI refactoring

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
3 messages Options
Reply | Threaded
Open this post in threaded view
|

SPI refactoring

Mark Dilger-4
Hackers,

As discussed with Tom in [1] and again with Pavel and Alvaro in [2],
here is a partial WIP refactoring of the SPI interface.  The goal is to
remove as many of the SPI_ERROR_xxx codes as possible from the
interface, replacing them with elog(ERROR), without removing the ability
of callers to check meaningful return codes and make their own decisions
about what to do next.  The crucial point here is that many of the error
codes in SPI are "can't happen" or "you made a programmatic mistake"
type errors that don't require run time remediation, but rather require
fixing the C code and recompiling.  Those seem better handled as an
elog(ERROR) to avoid the need for tests against such error codes
sprinkled throughout the system.

One upshot of the refactoring is that some of the SPI functions that
previously returned an error code can be changed to return void.  Tom
suggested a nice way to use macros to provide backward compatibility
with older third-party software, and I used his suggestion.

I need guidance with SPI_ERROR_ARGUMENT because it is used by functions
that don't return an integer error code, but rather return a SPIPlanPtr,
such as SPI_prepare.  Those functions return NULL and set a global
variable named SPI_result to the error code.  I'd be happy to just
convert this to throwing an error, too, but it is a greater API break
than anything implemented in the attached patches so far.  How do others
feel about it?

If more places like this can be converted to use elog(ERROR), it may be
possible to convert more functions to return void.


[1] https://www.postgresql.org/message-id/13404.1558558354%40sss.pgh.pa.us

[2]
https://www.postgresql.org/message-id/20191106151112.GA12251%40alvherre.pgsql
--
Mark Dilger

v1-0001-Deprecating-unused-SPI-error-codes.patch (4K) Download Attachment
v1-0002-Changing-SPI_connect-and-SPI_connect_ext-to-retur.patch (13K) Download Attachment
v1-0003-Refactoring-SPI-trigger-related-errors.patch (4K) Download Attachment
v1-0004-Refactoring-SPI-execute-related-errors.patch (3K) Download Attachment
v1-0005-Refactoring-SPI-related-errors.patch (2K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: SPI refactoring

Pavel Stehule


pá 8. 11. 2019 v 0:39 odesílatel Mark Dilger <[hidden email]> napsal:
Hackers,

As discussed with Tom in [1] and again with Pavel and Alvaro in [2],
here is a partial WIP refactoring of the SPI interface.  The goal is to
remove as many of the SPI_ERROR_xxx codes as possible from the
interface, replacing them with elog(ERROR), without removing the ability
of callers to check meaningful return codes and make their own decisions
about what to do next.  The crucial point here is that many of the error
codes in SPI are "can't happen" or "you made a programmatic mistake"
type errors that don't require run time remediation, but rather require
fixing the C code and recompiling.  Those seem better handled as an
elog(ERROR) to avoid the need for tests against such error codes
sprinkled throughout the system.

One upshot of the refactoring is that some of the SPI functions that
previously returned an error code can be changed to return void.  Tom
suggested a nice way to use macros to provide backward compatibility
with older third-party software, and I used his suggestion.

I need guidance with SPI_ERROR_ARGUMENT because it is used by functions
that don't return an integer error code, but rather return a SPIPlanPtr,
such as SPI_prepare.  Those functions return NULL and set a global
variable named SPI_result to the error code.  I'd be happy to just
convert this to throwing an error, too, but it is a greater API break
than anything implemented in the attached patches so far.  How do others
feel about it?

If more places like this can be converted to use elog(ERROR), it may be
possible to convert more functions to return void.


[1] https://www.postgresql.org/message-id/13404.1558558354%40sss.pgh.pa.us

[2]
https://www.postgresql.org/message-id/20191106151112.GA12251%40alvherre.pgsql

Generally lot of API used by extensions are changing - SPI is not different, and I don't see too much benefit of compatibility API. When you need to define BACKWARDS_COMPATIBLE_SPI_CALLS, then you can clean code.

It looks for me needlessly. If we change internal API, then should be clean signal so code should be fixed, so I don't like

-#define SPI_ERROR_PARAM (-7)
+#define SPI_ERROR_PARAM (-7) /* not used anymore */

It should be removed.

I am maybe too aggressive - but because any extension should be compiled for any postgres release, I don't think so we should to hold some internal obsolete API. BACKWARDS_COMPATIBLE.. is not used else where, and I would not to introduce this concept here. It can helps in short perspective, but it can be trap in long perspective.

Regards

Pavel





--
Mark Dilger
Reply | Threaded
Open this post in threaded view
|

Re: SPI refactoring

Alvaro Herrera-9
In reply to this post by Mark Dilger-4
On 2019-Nov-07, Mark Dilger wrote:

> From 113d42772be2c2abd71fd142cde9240522f143d7 Mon Sep 17 00:00:00 2001
> From: Mark Dilger <[hidden email]>
> Date: Thu, 7 Nov 2019 07:51:06 -0800
> Subject: [PATCH v1 1/5] Deprecating unused SPI error codes.
>
> The SPI_ERROR_NOOUTFUNC and SPI_ERROR_CONNECT codes, defined in spi.h,
> were no longer used anywhere in the sources except nominally in spi.c
> where SPI_result_code_string(int code) was translating them to a cstring
> for use in error messages.  But since the system never returns these
> error codes, it seems unlikely anybody still needs to be able to convert
> them to a string.
>
> Removing these from spi.c, from the docs, and from a code comment in
> contrib.  Leaving the definition in spi.h for backwards compatibility of
> third-party applications.

Because of PG_MODULE_MAGIC forcing a recompile of modules for each major
server version, there's no ABI-stability requirement for these  values.
If we were to leave the definitions in spi.h and remove the code that
handles them, then code could compile but at run-time it would produce
the "unrecognized" string.  Therefore I think it is better to remove the
definitions from spi.h, so that we can be sure that the code will never
be needed.

I didn't look at the other patches, but I suppose the same argument
applies to retaining their defines too.

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