Re: SPI refactoring

[Alvaro Herrera]

On 2019-Nov-07, Mark Dilger wrote:

> From 113d42772be2c2abd71fd142cde9240522f143d7 Mon Sep 17 00:00:00 2001
> From: Mark Dilger 
> 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 Herrerahttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: SPI refactoring

[Pavel Stehule]

pá 8. 11. 2019 v 0:39 odesílatel Mark Dilger 
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
>