Move tests of contrib/spi/ out of the core regression tests
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]: /messages/by-id/79755a2b18ed4fe5e29da6a87a1e00d1@postgrespro.ru
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]: /messages/by-id/79755a2b18ed4fe5e29da6a87a1e00d1@postgrespro.ru
Attachments:
v1-split-out-contrib-spi-tests.patchtext/x-diff; charset=us-ascii; name=v1-split-out-contrib-spi-tests.patchDownload+332-651
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)
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:
Show quoted text
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)
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
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
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)
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