Horton

Thread

Commits

  1. Don't bother checking the result of SPI_connect[_ext] anymore.

  1. SPI_connect, SPI_connect_ext return type

    Stepan Neretin <sndcppg@gmail.com> — 2024-08-10T13:55:46Z

    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.

  2. Re: SPI_connect, SPI_connect_ext return type

    Stepan Neretin <sndcppg@gmail.com> — 2024-08-10T14:00:02Z

    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.

  3. Re: SPI_connect, SPI_connect_ext return type

    Tom Lane <tgl@sss.pgh.pa.us> — 2024-08-10T14:12:04Z

    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

  4. Re: SPI_connect, SPI_connect_ext return type

    Stepan Neretin <sndcppg@gmail.com> — 2024-08-10T14:45:01Z

    > >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?
Best Regards, Stepan Neretin.

  5. Re: SPI_connect, SPI_connect_ext return type

    David G. Johnston <david.g.johnston@gmail.com> — 2024-08-10T15:18:31Z

    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.

  6. Re: SPI_connect, SPI_connect_ext return type

    Tom Lane <tgl@sss.pgh.pa.us> — 2024-08-10T16:29:04Z

    "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

  7. Re: SPI_connect, SPI_connect_ext return type

    David G. Johnston <david.g.johnston@gmail.com> — 2024-08-10T17:25:29Z

    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.

https://www.postgresql.org/docs/current/spi-examples.html

    /* 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;

David J.

  8. Re: SPI_connect, SPI_connect_ext return type

    Stepan Neretin <sndcppg@gmail.com> — 2024-08-10T18:34:43Z

    > I'd give it decent odds since our example usage doesn't include the test.
> https://www.postgresql.org/docs/current/spi-examples.html

https://www.postgresql.org/docs/devel/trigger-example.html
<https://www.postgresql.org/docs/current/spi-examples.html>

> /* 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.

  9. Re: SPI_connect, SPI_connect_ext return type

    Peter Eisentraut <peter@eisentraut.org> — 2024-08-12T12:39:04Z

    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.

  10. Re: SPI_connect, SPI_connect_ext return type

    Tom Lane <tgl@sss.pgh.pa.us> — 2024-09-05T21:13:50Z

    Peter Eisentraut <peter@eisentraut.org> writes:
> On 10.08.24 16:12, Tom Lane wrote:
>> 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.

After thinking about this for awhile, one reason that it's practical
to change it today for SPI_connect is that SPI_connect has not
returned anything except SPI_OK_CONNECT since v10.  So if we tell
extension authors they don't need to check the result, it's unlikely
that that will cause any new code they write to get used with PG
versions where it would be wrong.  I fear that we'd need a similar
multi-year journey to get to a place where we could deprecate checking
the result of any other SPI function.

Nonetheless, there seems no reason not to do it now for SPI_connect.
So attached is a patch that documents the result value as vestigial
and removes the calling-code checks in our own code, but doesn't
touch SPI_connect[_ext] itself.  This combines portions of Stepan's
two patches with some additional work (mostly, that he'd missed fixing
any of contrib/).

			regards, tom lane

  11. Re: SPI_connect, SPI_connect_ext return type

    Stepan Neretin <sndcppg@gmail.com> — 2024-09-08T06:35:00Z

    > So if we tell extension authors they don't need to check the result, it's
unlikely
> that that will cause any new code they write to get used with PG
> versions where it would be wrong.
Yes, I concur.

> This combines portions of Stepan's
> two patches with some additional work (mostly, that he'd missed fixing
> any of contrib/).
Thank you for the feedback! I believe the patch looks satisfactory. Should
we await a review? The changes seem straightforward to me.

  12. Re: SPI_connect, SPI_connect_ext return type

    Tom Lane <tgl@sss.pgh.pa.us> — 2024-09-08T07:31:23Z

    Stepan Neretin <sndcppg@gmail.com> writes:
>> This combines portions of Stepan's
>> two patches with some additional work (mostly, that he'd missed fixing
>> any of contrib/).

> Thank you for the feedback! I believe the patch looks satisfactory. Should
> we await a review? The changes seem straightforward to me.

I too think it's good to go.  If no one complains or asks for
more time to review, I will push it Monday or so.

			regards, tom lane

  13. Re: SPI_connect, SPI_connect_ext return type

    Tom Lane <tgl@sss.pgh.pa.us> — 2024-09-09T16:19:58Z

    I wrote:
> I too think it's good to go.  If no one complains or asks for
> more time to review, I will push it Monday or so.

And done.

			regards, tom lane

  14. Re: SPI_connect, SPI_connect_ext return type

    Robert Haas <robertmhaas@gmail.com> — 2024-09-09T16:29:20Z

    On Mon, Sep 9, 2024 at 12:20 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> I wrote:
> > I too think it's good to go.  If no one complains or asks for
> > more time to review, I will push it Monday or so.
>
> And done.

I didn't see this thread until after the commit had already happened,
but a belated +1 for this and any other cruft removal we can do in
SPI-land.


--
Robert Haas
EDB: http://www.enterprisedb.com