Thread: SPI_connect, SPI_connect_ext return type
Hi, hackers! If you look at the code in the src/backend/executor/spi.c file, you will see the SPI_connect function familiar to many there, which internally simply calls SPI_connect_ext. The return type is int, at the end it says return SPI_OK_CONNECT;
It confuses me that nothing but OK, judging by the code, can return.(I understand that earlier, before 1833f1a1, it could also return SPI_ERROR_CONNECT). Therefore, I suggest making the returned value void instead of int and not checking the returned value. What do you think about this?
Best Regards, Stepan Neretin.
Attachment
Regarding checking the return value of these functions, I would also like to add that somewhere in the code before my patch, the value is checked, and somewhere it is not. I removed the check everywhere and it became the same style.
Stepan <sndcppg@gmail.com> writes:
> Hi, hackers! If you look at the code in the src/backend/executor/spi.c file,
> you will see the SPI_connect function familiar to many there, which
> internally simply calls SPI_connect_ext. The return type is int, at the end
> it says return SPI_OK_CONNECT;
> It confuses me that nothing but OK, judging by the code, can return.(I
> understand that earlier, before 1833f1a1, it could also return
> SPI_ERROR_CONNECT). Therefore, I suggest making the returned value void
> instead of int and not checking the returned value. What do you think about
> this?
That would break a lot of code (much of it not under our control) to
little purpose; it would also foreclose the option to return to using
SPI_ERROR_CONNECT someday.
We go to a lot of effort to keep the SPI API as stable as we can
across major versions, so I don't see why we'd just randomly make
an API-breaking change like this.
regards, tom lane
>That would break a lot of code (much of it not under our control) to
little purpose; it would also foreclose the option to return to using
SPI_ERROR_CONNECT someday.
Agree, it makes sense.
The only question left is, is there any logic in why in some places its return value of these functions is checked, and in some not? Can I add checks everywhere?
The only question left is, is there any logic in why in some places its return value of these functions is checked, and in some not? Can I add checks everywhere?
Best Regards, Stepan Neretin.
On Saturday, August 10, 2024, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Stepan <sndcppg@gmail.com> writes:
> Hi, hackers! If you look at the code in the src/backend/executor/spi.c file,
> you will see the SPI_connect function familiar to many there, which
> internally simply calls SPI_connect_ext. The return type is int, at the end
> it says return SPI_OK_CONNECT;
> It confuses me that nothing but OK, judging by the code, can return.(I
> understand that earlier, before 1833f1a1, it could also return
> SPI_ERROR_CONNECT). Therefore, I suggest making the returned value void
> instead of int and not checking the returned value. What do you think about
> this?
That would break a lot of code (much of it not under our control) to
little purpose; it would also foreclose the option to return to using
SPI_ERROR_CONNECT someday.
I suggest we document it as deprecated and insist any future attempt to implement a return-on-error connection function define a completely new function.
David J.
"David G. Johnston" <david.g.johnston@gmail.com> writes:
> On Saturday, August 10, 2024, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> That would break a lot of code (much of it not under our control) to
>> little purpose; it would also foreclose the option to return to using
>> SPI_ERROR_CONNECT someday.
> I suggest we document it as deprecated and insist any future attempt to
> implement a return-on-error connection function define a completely new
> function.
True; we're kind of in an intermediate place right now where certain
call sites aren't bothering to check the return code, but it's hard
to argue that they're really wrong --- and more to the point,
re-introducing use of SPI_ERROR_CONNECT would break them. I don't
know if that usage pattern has propagated outside Postgres core,
but it might've. Perhaps it would be better to update the docs to
say that the only return value is SPI_OK_CONNECT and all failure
cases are reported via elog/ereport.
regards, tom lane
On Sat, Aug 10, 2024 at 9:29 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
"David G. Johnston" <david.g.johnston@gmail.com> writes:
> On Saturday, August 10, 2024, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> That would break a lot of code (much of it not under our control) to
>> little purpose; it would also foreclose the option to return to using
>> SPI_ERROR_CONNECT someday.
> I suggest we document it as deprecated and insist any future attempt to
> implement a return-on-error connection function define a completely new
> function.
I don't
know if that usage pattern has propagated outside Postgres core,
but it might've.
I'd give it decent odds since our example usage doesn't include the test.
/* Convert given text object to a C string */
command = text_to_cstring(PG_GETARG_TEXT_PP(0));
cnt = PG_GETARG_INT32(1);
SPI_connect();
ret = SPI_exec(command, cnt);
proc = SPI_processed;
command = text_to_cstring(PG_GETARG_TEXT_PP(0));
cnt = PG_GETARG_INT32(1);
SPI_connect();
ret = SPI_exec(command, cnt);
proc = SPI_processed;
David J.
> /* connect to SPI manager */
> if ((ret = SPI_connect()) < 0) > elog(ERROR, "trigf (fired %s): SPI_connect returned %d", when, ret);
in this page check include in the example.
I think we need to give examples of one kind. If there is no void in the code, then there should be checks everywhere (at least in the documentation).
What do you think about the attached patch?
Best Regards, Stepan Neretin.
Attachment
On 10.08.24 16:12, Tom Lane wrote: > Stepan <sndcppg@gmail.com> writes: >> Hi, hackers! If you look at the code in the src/backend/executor/spi.c file, >> you will see the SPI_connect function familiar to many there, which >> internally simply calls SPI_connect_ext. The return type is int, at the end >> it says return SPI_OK_CONNECT; >> It confuses me that nothing but OK, judging by the code, can return.(I >> understand that earlier, before 1833f1a1, it could also return >> SPI_ERROR_CONNECT). Therefore, I suggest making the returned value void >> instead of int and not checking the returned value. What do you think about >> this? > > That would break a lot of code (much of it not under our control) to > little purpose; it would also foreclose the option to return to using > SPI_ERROR_CONNECT someday. > > We go to a lot of effort to keep the SPI API as stable as we can > across major versions, so I don't see why we'd just randomly make > an API-breaking change like this. Here is a previous discussion: https://www.postgresql.org/message-id/flat/1356682025.20017.4.camel%40vanquo.pezone.net I like the idea that we would keep the API but convert most errors to exceptions.