Обсуждение: pgsql: injection_points: Remove portions related to custom pgstats

injection_points: Remove portions related to custom pgstats

The test module injection_points has been used as a landing spot to
provide coverage for the custom pgstats APIs, for both fixed-sized and
variable-sized stats kinds.  Some recent work related to pgstats is
proving that this structure makes the implementation of new tests
harder.

This commit removes the code related to pgstats from injection_points,
and an equivalent will be reintroduced as a separate test module in a
follow-up commit.  This removal is done in its own commit for clarity.

Using injection_points for this test coverage was perhaps not the best
way to design things, but this was good enough while working on the
first flavor of the custom pgstats APIs.  Using a new test module will
make easier the introduction of new tests, and we will not need to worry
about the impact of new changes related to custom pgstats could have
with the internals of injection_points.

Author: Sami Imseih <samimseih@gmail.com>
Discussion: https://postgr.es/m/CAA5RZ0sJgO6GAwgFxmzg9MVP=rM7Us8KKcWpuqxe-f5qxmpE0g@mail.gmail.com

Branch
------
master

Details
-------
https://git.postgresql.org/pg/commitdiff/d52c24b0f808a6644fd839c507625eafd6bb9f12

Modified Files
--------------
doc/src/sgml/xfunc.sgml                            |   6 -
src/test/modules/injection_points/Makefile         |   4 -
.../injection_points/injection_points--1.0.sql     |  43 ----
.../modules/injection_points/injection_points.c    |  48 -----
.../modules/injection_points/injection_stats.c     | 228 ---------------------
.../modules/injection_points/injection_stats.h     |  35 ----
.../injection_points/injection_stats_fixed.c       | 214 -------------------
src/test/modules/injection_points/meson.build      |  12 --
src/test/modules/injection_points/t/001_stats.pl   | 103 ----------
src/tools/pgindent/typedefs.list                   |   4 -
10 files changed, 697 deletions(-)

Michael Paquier <michael@paquier.xyz> writes:
> injection_points: Remove portions related to custom pgstats

BF member crake is unhappy with this, because it broke cross-version
update tests [1]:

pg_restore: while PROCESSING TOC:
pg_restore: from TOC entry 230; 1255 55098 FUNCTION injection_points_stats_drop() buildfarm
pg_restore: error: could not execute query: ERROR:  could not find function "injection_points_stats_drop" in file
"/home/andrew/bf/root/saves.crake/HEAD/lib/postgresql/injection_points.so"
Command was: CREATE FUNCTION "public"."injection_points_stats_drop"() RETURNS "void"
    LANGUAGE "c" STRICT
    AS '$libdir/injection_points', 'injection_points_stats_drop';

I think that this is really a bug in the Xversion-update test, because
I don't see why we'd be holding src/test/modules libraries to the
standard of can-be-upgraded-across-versions.  But maybe there's
something I'm missing?

            regards, tom lane

[1]
https://buildfarm.postgresql.org/cgi-bin/show_stage_log.pl?nm=crake&dt=2025-12-08%2003%3A47%3A03&stg=xversion-upgrade-REL_18_STABLE-HEAD

On Sun, Dec 07, 2025 at 11:15:13PM -0500, Tom Lane wrote:
> I think that this is really a bug in the Xversion-update test, because
> I don't see why we'd be holding src/test/modules libraries to the
> standard of can-be-upgraded-across-versions.  But maybe there's
> something I'm missing?

Yes, I've noticed that a couple of minutes ago, trying to make heads
and tails out of it.  I thought that the tets modules libraries were
not getting pulled in the cross-version upgrades but here we are.

From what I can see, this specific failure is caused by
src/test/modules/gin/ where the module has forgotten to drop the
extension injection_points, but there is a total of three modules that
could have hit the problem, as of:
- src/test/modules/nbtree/ (could be hit in the future as it uses
injection_points).
- src/test/modules/gin/
- src/test/modules/typcache/

And I was wondering whether I should just plant more items in
AdjustUpgrade.pm, like in the attached.  We need that for gin when
dealing with an old server of at least v17, and for typcache with v18.
The nbtree test is so recent that it does not matter.
TestUpgradeXversion.pm has an ON_ERROR_STOP=1 so the version check is
required.
--
Michael

Michael Paquier <michael@paquier.xyz> writes:
> On Sun, Dec 07, 2025 at 11:15:13PM -0500, Tom Lane wrote:
>> I think that this is really a bug in the Xversion-update test, because
>> I don't see why we'd be holding src/test/modules libraries to the
>> standard of can-be-upgraded-across-versions.  But maybe there's
>> something I'm missing?

> And I was wondering whether I should just plant more items in
> AdjustUpgrade.pm, like in the attached.

Nah, hold off till we have a discussion about whether the test is
correct to expect compatibility.  We can afford to have a couple
of BF animals unhappy for a day or two while we come to a decision.

            regards, tom lane

On Sun, Dec 07, 2025 at 11:57:34PM -0500, Tom Lane wrote:
> Nah, hold off till we have a discussion about whether the test is
> correct to expect compatibility.  We can afford to have a couple
> of BF animals unhappy for a day or two while we come to a decision.

By the way, while looking at the buildfarm failures, I have noticed
that something is wrong in my patch.  Looking Makefile.global.in,
USE_MODULE_DB is built based on REGRESS for the tests of
src/test/modules/gin/.  However things are inconsistent between meson
and ./configure.

For example, this host uses configure/make:
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=skimmer&dt=2025-12-08%2006%3A08%3A23

The database name is contrib_regression_gin_incomplete_splits, which
points to the test gin_incomplete_split in src/test/modules/gin/,
meaning that my patch would not work.

In the meson case, we got this failure:
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=crake&dt=2025-12-08%2006%3A27%3A04
And here, the database name is regression_gin.

When I looked at the buildfarm failures, I saw the regression_gin
first, wrote a patch based on that.  But the second one based on
contrib_regression_gin_incomplete_splits was really puzzling me, until
I've noticed that the difference was configure vs meson for the two
hosts.  So it seems to me that we have a second issue here, where
meson relies on a "name" while configure uses the first value of
"REGRESS".  AdjustUpgrade.pm cannot know that it's dealing with one
build method or the other.  In order to make my previous patch work,
we could use a DROP DATABASE IF EXISTS on both database names.  That
would work, but that feels grotty for the purpose of fixing things,
especially as I don't see a point in keeping the test libraries across
major upgrades..
--
Michael

On 08/12/2025 10:41, Michael Paquier wrote:
> On Sun, Dec 07, 2025 at 11:57:34PM -0500, Tom Lane wrote:
>> Nah, hold off till we have a discussion about whether the test is
>> correct to expect compatibility.  We can afford to have a couple
>> of BF animals unhappy for a day or two while we come to a decision.
> 
> By the way, while looking at the buildfarm failures, I have noticed
> that something is wrong in my patch.  Looking Makefile.global.in,
> USE_MODULE_DB is built based on REGRESS for the tests of
> src/test/modules/gin/.  However things are inconsistent between meson
> and ./configure.
> 
> For example, this host uses configure/make:
> https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=skimmer&dt=2025-12-08%2006%3A08%3A23
> 
> The database name is contrib_regression_gin_incomplete_splits, which
> points to the test gin_incomplete_split in src/test/modules/gin/,
> meaning that my patch would not work.
> 
> In the meson case, we got this failure:
> https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=crake&dt=2025-12-08%2006%3A27%3A04
> And here, the database name is regression_gin.
> 
> When I looked at the buildfarm failures, I saw the regression_gin
> first, wrote a patch based on that.  But the second one based on
> contrib_regression_gin_incomplete_splits was really puzzling me, until
> I've noticed that the difference was configure vs meson for the two
> hosts.  So it seems to me that we have a second issue here, where
> meson relies on a "name" while configure uses the first value of
> "REGRESS".  AdjustUpgrade.pm cannot know that it's dealing with one
> build method or the other.  In order to make my previous patch work,
> we could use a DROP DATABASE IF EXISTS on both database names.  That
> would work, but that feels grotty for the purpose of fixing things,
> especially as I don't see a point in keeping the test libraries across
> major upgrades..

Hmm, using the first value of REGRESS is surprising. How about we change 
the autoconf logic to match what meson does? As attached.

In the meson scripts, you can specify the database name explicitly by 
setting 'dbname'. It's only used in a few subdirs though.

This in src/test/interfaces/ecpg/test/meson.build looks weird:

>     'dbname': 'ecpg1_regression,ecpg2_regression',

As far as I can tell, dbname is not a list, so I don't understand how 
that works.

- Heikki

Heikki Linnakangas <hlinnaka@iki.fi> writes:
> Hmm, using the first value of REGRESS is surprising. How about we change 
> the autoconf logic to match what meson does? As attached.

Unless you want to back-patch that as far as 9.2, it's going to make
matters worse not better for predictability of the DB names in the
cross-version upgrade tests.  I think it might be better to make the
meson logic match the makefiles.

Having said that, I think what we should be doing is adding whichever
DB names we decide are problematic to the existing logic at about line
89 in AdjustUpgrade.pm:

    # remove dbs of modules known to cause pg_upgrade to fail
    # anything not builtin and incompatible should clean up its own db
    foreach my $bad_module ('adminpack', 'test_ddl_deparse', 'tsearch2')
    {
        if ($dbnames{"contrib_regression_$bad_module"})
        {
            _add_st($result, 'postgres',
                "drop database contrib_regression_$bad_module");
            delete($dbnames{"contrib_regression_$bad_module"});
        }
        if ($dbnames{"regression_$bad_module"})
        {
            _add_st($result, 'postgres',
                "drop database regression_$bad_module");
            delete($dbnames{"regression_$bad_module"});
        }
    }

Perhaps another alternative is to add "DROP EXTENSION
injection_points" at the end of the test scripts in the
problematic modules.

            regards, tom lane

On 09/12/2025 19:40, Tom Lane wrote:
> Perhaps another alternative is to add "DROP EXTENSION
> injection_points" at the end of the test scripts in the
> problematic modules.

It's a little tricky if a module has more than one test that uses the 
extension. Namely src/test/modules/nbtree which I added recently has two 
test scripts, nbtree_incomplete_splits.sql and 
nbtree_half_dead_pages.sql, and both of them do "create extension if not 
exists injection_points;". They run concurrently, so they race on which 
one creates the extension first. We can't do the same for DROP EXTENSION 
at the end.

Maybe that's not a great design in the first place, though. Perhaps that 
module should use a schedule file:

# create injection_points extension before the actual tests
test: test_setup

test: nbtree_incomplete_splits nbtree_half_dead_pages

# drop injection_points extension because it cannot be pg_upgraded.
test: test_clean

- Heikki

On Tue, Dec 09, 2025 at 10:04:43PM +0200, Heikki Linnakangas wrote:
> It's a little tricky if a module has more than one test that uses the
> extension. Namely src/test/modules/nbtree which I added recently has two
> test scripts, nbtree_incomplete_splits.sql and nbtree_half_dead_pages.sql,
> and both of them do "create extension if not exists injection_points;". They
> run concurrently, so they race on which one creates the extension first. We
> can't do the same for DROP EXTENSION at the end.
>
> Maybe that's not a great design in the first place, though. Perhaps that
> module should use a schedule file:
>
> # create injection_points extension before the actual tests
> test: test_setup
>
> test: nbtree_incomplete_splits nbtree_half_dead_pages
>
> # drop injection_points extension because it cannot be pg_upgraded.
> test: test_clean

Are the dumps of the old versions used in the buildfarm runs updated
with each run?  If yes, forcing a DROP EXTENSION in the problematic
test modules has my favors over the dirtier tricks that would be
required in AdjustUpgrade for the various database name patterns that
depend on the build method.  If no, well, AdjustUpgrade it is with
some DROP DATABASE IF EXISTS.
--
Michael

Michael Paquier <michael@paquier.xyz> writes:
> Are the dumps of the old versions used in the buildfarm runs updated
> with each run?

They are updated when a new BF run is made for that branch, so
back-patching the DROP EXTENSION commands should do the trick.

            regards, tom lane

On Tue, Dec 09, 2025 at 05:49:24PM -0500, Tom Lane wrote:
> They are updated when a new BF run is made for that branch, so
> back-patching the DROP EXTENSION commands should do the trick.

Okay, good to know.  Let's do that then.

Encouraging a setup and a cleanup step seems like a good thing here,
as people like copy-pasting contents when creating a module.  Should I
just go ahead and make that happen?  The buildfarm breakage is still
on me based on the commit maths.

By the way, REGRESS in modules/nbtree/Makefile is missing
nbtree_half_dead_pages..  meson.build has both tests.
--
Michael

On 10/12/2025 00:56, Michael Paquier wrote:
> On Tue, Dec 09, 2025 at 05:49:24PM -0500, Tom Lane wrote:
>> They are updated when a new BF run is made for that branch, so
>> back-patching the DROP EXTENSION commands should do the trick.
> 
> Okay, good to know.  Let's do that then.
> 
> Encouraging a setup and a cleanup step seems like a good thing here,
> as people like copy-pasting contents when creating a module.  Should I
> just go ahead and make that happen?  The buildfarm breakage is still
> on me based on the commit maths.

Yes please. I'm also happy to help if you'd like. I'm eagerly waiting to 
see if the "Widen MultiXactOffset to 64 bits" commit caused any new 
cross-version pg_ugprade test failures.

> By the way, REGRESS in modules/nbtree/Makefile is missing
> nbtree_half_dead_pages..  meson.build has both tests.

That one is on me..

- Heikki

On Wed, Dec 10, 2025 at 01:04:31AM +0200, Heikki Linnakangas wrote:
> Yes please. I'm also happy to help if you'd like. I'm eagerly waiting to see
> if the "Widen MultiXactOffset to 64 bits" commit caused any new
> cross-version pg_ugprade test failures.

Okay, I'll see about doing something.

By the way, there is also a consequence in using the first test name
in the REGRESS list for the database name: we cannot use the same name
for the "setup" phase of all these test modules, or the gin, typcache
and nbtree modules would conflict with each other because they would
use the same database name.

By the way, it seems to me that even under meson the two nbtree tests
do not compete with each other: they are executed in a serialized
fashion.  Parallel mode can be enforced with a schedule, but it's just
to say that a DROP EXTENSION in each SQL file should work as well.

>> By the way, REGRESS in modules/nbtree/Makefile is missing
>> nbtree_half_dead_pages..  meson.build has both tests.
>
> That one is on me..

I am not going to touch this line, but I don't mind doing something
about it as I am touching the same area.  Another thing: the nbtree
module is missing a .gitignore.
--
Michael

On Wed, Dec 10, 2025 at 08:43:10AM +0900, Michael Paquier wrote:
> I am not going to touch this line, but I don't mind doing something
> about it as I am touching the same area.  Another thing: the nbtree
> module is missing a .gitignore.

All that should be now addressed with 1d7b00dc14b8 (down to v17) and
06817fc8a4b7 (HEAD only for nbtree).  Let's see what the buildfarm
thinks.
--
Michael

On Wed, Dec 10, 2025 at 01:05:39PM +0900, Michael Paquier wrote:
> All that should be now addressed with 1d7b00dc14b8 (down to v17) and
> 06817fc8a4b7 (HEAD only for nbtree).  Let's see what the buildfarm
> thinks.

And crake is now green.  Thanks all for the discussion.
--
Michael

On 10/12/2025 07:53, Michael Paquier wrote:
> On Wed, Dec 10, 2025 at 01:05:39PM +0900, Michael Paquier wrote:
>> All that should be now addressed with 1d7b00dc14b8 (down to v17) and
>> 06817fc8a4b7 (HEAD only for nbtree).  Let's see what the buildfarm
>> thinks.
> 
> And crake is now green.  Thanks all for the discussion.

Thanks Michael!

- Heikki