Horton

Thread

Commits

  1. Move contrib/spi testing from core regression tests to contrib/spi.

  1. Move tests of contrib/spi/ out of the core regression tests

    Tom Lane <tgl@sss.pgh.pa.us> — 2025-04-08T01:59:19Z

    The attached patch removes test cases concerned with contrib/spi/
from the core regression tests and instead puts them into new
test files in contrib/spi/ itself.

My original motivation for looking at this was the discussion in
[1] about whether to remove contrib/spi/refint.c entirely, since
it's rather buggy and not a great example of our modern coding
practices.  So I wondered whether the core test cases that use it
were contributing any significant amount of code coverage on the
core code.  (Spoiler: nope.)  But I think this is generally good
cleanup anyway, because it locates the test code for contrib/spi
where a person would expect to find that, and removes some rather
grotty coding in src/test/regress's Makefile and meson.build.
As a side benefit, it removes some small number of cycles from
the core tests, which seems like a good thing.

The tests for the refint module are just moved over verbatim,
except for using CREATE EXTENSION instead of manual declaration
of the C functions.  Also, I kept the tests for COMMENT ON TRIGGER
in the core tests, by applying them to a different trigger.

The tests for autoinc were kind of messy, because the behavior of
autoinc itself was impossibly intertwined with the behavior of
"ttdummy", which is an undocumented C function in regress.c.
After some thought I decided we could just nuke ttdummy altogether,
so the new autoinc.sql test is much simpler and more straightforward.

(I realized while doing this that the description of autoinc in
the SGML docs is not a great description of what the function
actually does, so the patch includes some updates to those docs.)

So far as I can tell, the code coverage of the core regression
tests is unchanged by this patch: the removed test cases were
100% redundant with other cases, so far as the core is concerned.

This is too late for v18 of course, so I'll park it in the next CF.

			regards, tom lane

[1] https://www.postgresql.org/message-id/flat/79755a2b18ed4fe5e29da6a87a1e00d1%40postgrespro.ru

  2. Re: Move tests of contrib/spi/ out of the core regression tests

    Heikki Linnakangas <hlinnaka@iki.fi> — 2025-04-08T09:21:47Z

    On 08/04/2025 04:59, Tom Lane wrote:
> The attached patch removes test cases concerned with contrib/spi/
> from the core regression tests and instead puts them into new
> test files in contrib/spi/ itself.

+1

> My original motivation for looking at this was the discussion in
> [1] about whether to remove contrib/spi/refint.c entirely, since
> it's rather buggy and not a great example of our modern coding
> practices.  So I wondered whether the core test cases that use it
> were contributing any significant amount of code coverage on the
> core code.  (Spoiler: nope.)  But I think this is generally good
> cleanup anyway, because it locates the test code for contrib/spi
> where a person would expect to find that, and removes some rather
> grotty coding in src/test/regress's Makefile and meson.build.
> As a side benefit, it removes some small number of cycles from
> the core tests, which seems like a good thing.
> 
> The tests for the refint module are just moved over verbatim,
> except for using CREATE EXTENSION instead of manual declaration
> of the C functions.  Also, I kept the tests for COMMENT ON TRIGGER
> in the core tests, by applying them to a different trigger.
> 
> The tests for autoinc were kind of messy, because the behavior of
> autoinc itself was impossibly intertwined with the behavior of
> "ttdummy", which is an undocumented C function in regress.c.
> After some thought I decided we could just nuke ttdummy altogether,
> so the new autoinc.sql test is much simpler and more straightforward.

My only worry would if 'ttdummy' was a good showcase for how to write a 
trigger function in C. But it's not a particularly good example. There 
is a better example in the docs, and there's the 'autoinc' trigger too.

> This is too late for v18 of course, so I'll park it in the next CF.

In my opinion this would still be totally OK for v18. It's just test 
changes. I would rather commit cleanups like this sooner than later.

-- 
Heikki Linnakangas
Neon (https://neon.tech)

  3. Re: Move tests of contrib/spi/ out of the core regression tests

    wenhui qiu <qiuwenhuifx@gmail.com> — 2025-04-08T12:25:29Z

    Hi
> This is too late for v18 of course, so I'll park it in the next CF.

> In my opinion this would still be totally OK for v18. It's just test
> changes. I would rather commit cleanups like this sooner than later.
Agree +1

On Tue, Apr 8, 2025 at 5:22 PM Heikki Linnakangas <hlinnaka@iki.fi> wrote:

> On 08/04/2025 04:59, Tom Lane wrote:
> > The attached patch removes test cases concerned with contrib/spi/
> > from the core regression tests and instead puts them into new
> > test files in contrib/spi/ itself.
>
> +1
>
> > My original motivation for looking at this was the discussion in
> > [1] about whether to remove contrib/spi/refint.c entirely, since
> > it's rather buggy and not a great example of our modern coding
> > practices.  So I wondered whether the core test cases that use it
> > were contributing any significant amount of code coverage on the
> > core code.  (Spoiler: nope.)  But I think this is generally good
> > cleanup anyway, because it locates the test code for contrib/spi
> > where a person would expect to find that, and removes some rather
> > grotty coding in src/test/regress's Makefile and meson.build.
> > As a side benefit, it removes some small number of cycles from
> > the core tests, which seems like a good thing.
> >
> > The tests for the refint module are just moved over verbatim,
> > except for using CREATE EXTENSION instead of manual declaration
> > of the C functions.  Also, I kept the tests for COMMENT ON TRIGGER
> > in the core tests, by applying them to a different trigger.
> >
> > The tests for autoinc were kind of messy, because the behavior of
> > autoinc itself was impossibly intertwined with the behavior of
> > "ttdummy", which is an undocumented C function in regress.c.
> > After some thought I decided we could just nuke ttdummy altogether,
> > so the new autoinc.sql test is much simpler and more straightforward.
>
> My only worry would if 'ttdummy' was a good showcase for how to write a
> trigger function in C. But it's not a particularly good example. There
> is a better example in the docs, and there's the 'autoinc' trigger too.
>
> > This is too late for v18 of course, so I'll park it in the next CF.
>
> In my opinion this would still be totally OK for v18. It's just test
> changes. I would rather commit cleanups like this sooner than later.
>
> --
> Heikki Linnakangas
> Neon (https://neon.tech)
>
>
>

  4. Re: Move tests of contrib/spi/ out of the core regression tests

    Andrew Dunstan <andrew@dunslane.net> — 2025-04-08T13:17:40Z

    On 2025-04-08 Tu 5:21 AM, Heikki Linnakangas wrote:
>
>
>> This is too late for v18 of course, so I'll park it in the next CF.
>
> In my opinion this would still be totally OK for v18. It's just test 
> changes. I would rather commit cleanups like this sooner than later.


Yeah, that was my reaction too.


cheers


andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

  5. Re: Move tests of contrib/spi/ out of the core regression tests

    Tom Lane <tgl@sss.pgh.pa.us> — 2025-04-08T13:55:35Z

    Andrew Dunstan <andrew@dunslane.net> writes:
> On 2025-04-08 Tu 5:21 AM, Heikki Linnakangas wrote:
>>> This is too late for v18 of course, so I'll park it in the next CF.

>> In my opinion this would still be totally OK for v18. It's just test 
>> changes. I would rather commit cleanups like this sooner than later.

> Yeah, that was my reaction too.

Well, I don't mind pushing it, but does anyone want to review
it first?  It sounded like Heikki had at least eyeballed the
patch, but I'm not sure if he's ready to sign off on it.

			regards, tom lane

  6. Re: Move tests of contrib/spi/ out of the core regression tests

    Heikki Linnakangas <hlinnaka@iki.fi> — 2025-04-08T19:51:54Z

    On 08/04/2025 16:55, Tom Lane wrote:
> Well, I don't mind pushing it, but does anyone want to review
> it first?  It sounded like Heikki had at least eyeballed the
> patch, but I'm not sure if he's ready to sign off on it.

It looks good to me.

> diff --git a/doc/src/sgml/contrib-spi.sgml b/doc/src/sgml/contrib-spi.sgml
> index 55d3fac7a69..6fa9479d1b9 100644
> --- a/doc/src/sgml/contrib-spi.sgml
> +++ b/doc/src/sgml/contrib-spi.sgml
> @@ -81,10 +81,12 @@
>    <para>
>     <function>autoinc()</function> is a trigger that stores the next value of
>     a sequence into an integer field.  This has some overlap with the
> -   built-in <quote>serial column</quote> feature, but it is not the same:
> -   <function>autoinc()</function> will override attempts to substitute a
> -   different field value during inserts, and optionally it can be
> -   used to increment the field during updates, too.
> +   built-in <quote>serial column</quote> feature, but it is not the same.
> +   The trigger will replace the field's value only if that value is
> +   initially zero or null (after the action of the SQL statement that
> +   inserted or updated the row).  Also, if the sequence's next value is
> +   zero, <function>nextval()</function> will be called a second time in
> +   order to obtain a non-zero value.
>    </para>

That's a much clearer explanation of the behavior, but now that I read 
that paragraph, I wonder *why* it behaves like that. I'm sure it's just 
historical reasons. But perhaps we should nuke autoinc altogether?

-- 
Heikki Linnakangas
Neon (https://neon.tech)

  7. Re: Move tests of contrib/spi/ out of the core regression tests

    Tom Lane <tgl@sss.pgh.pa.us> — 2025-04-08T21:13:58Z

    Heikki Linnakangas <hlinnaka@iki.fi> writes:
> On 08/04/2025 16:55, Tom Lane wrote:
>> +   The trigger will replace the field's value only if that value is
>> +   initially zero or null (after the action of the SQL statement that
>> +   inserted or updated the row).  Also, if the sequence's next value is
>> +   zero, <function>nextval()</function> will be called a second time in
>> +   order to obtain a non-zero value.

> That's a much clearer explanation of the behavior, but now that I read 
> that paragraph, I wonder *why* it behaves like that. I'm sure it's just 
> historical reasons. But perhaps we should nuke autoinc altogether?

Yeah, we were kind of wondering that about refint.c as well.  But in
the end, none of the stuff in contrib/spi/ has much value as
production code.  It's meant to be examples you can start from and
modify.  In that light, I'm more worried about whether the coding
style is good than whether the definition is useful.

In any case, if we decided to nuke the modules entirely, we'd still
need half of the present patch in order to remove the core tests'
dependency on them.  So I'll go ahead and push this, and then we
have a more self-contained target if we decide we don't want 'em.

			regards, tom lane