Thread: SPI refactoring
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
Attachment
pá 8. 11. 2019 v 0:39 odesílatel Mark Dilger <hornschnorter@gmail.com> 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 */
+#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
On 2019-Nov-07, Mark Dilger wrote: > From 113d42772be2c2abd71fd142cde9240522f143d7 Mon Sep 17 00:00:00 2001 > From: Mark Dilger <hornschnorter@gmail.com> > 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