Test 041_checkpoint_at_promote.pl faild in installcheck due to missing injection_points
Hi!
After rebasing one of my old patches, I'm hit to a problem with the
installcheck test for 041_checkpoint_at_promote.pl.
At first, I thought it was something wrong with my patch, although it
doesn't relate to this part of the Postgres.
Then I decided to do the same run but on current master and got the same
result.
Here is my configure:
SRC="../postgres"
TRG=`pwd`
LINUX_CONFIGURE_FEATURES="
--without-llvm
--with-tcl --with-tclconfig=/usr/lib/tcl8.6/ --with-perl
--with-python --with-gssapi --with-pam --with-ldap --with-selinux
--with-systemd --with-uuid=ossp --with-libxml --with-libxslt
--with-zstd
--with-ssl=openssl
"
$SRC/configure \
-C \
--prefix=$TRG/"pgsql" \
--enable-debug --enable-tap-tests --enable-depend --enable-cassert \
--enable-injection-points --enable-nls \
$LINUX_CONFIGURE_FEATURES \
CC="ccache clang" CXX="ccache clang++" \
CFLAGS="-Og -ggdb -fsanitize-recover=all" \
CXXFLAGS="-Og -ggdb -fsanitize-recover=all"
And here is my run:
$ time make PROVE_TESTS=t/041_checkpoint_at_promote.pl installcheck -C
src/test/recovery
...
# Postmaster PID for node "standby1" is 820439
error running SQL: 'psql:<stdin>:1: ERROR: extension "injection_points" is
not available
DETAIL: Could not open extension control file
"/home/omg/proj/build/pgsql/share/extension/injection_points.control": No
such file or directory.
HINT: The extension must first be installed on the system where PostgreSQL
is running.'
while running 'psql -XAtq -d port=17154 host=/tmp/xkTLcw1tDb
dbname='postgres' -f - -v ON_ERROR_STOP=1' with sql 'CREATE EXTENSION
injection_points;' at
/home/omg/proj/build/../postgres/src/test/perl/PostgreSQL/Test/Cluster.pm
line 2140.
# Postmaster PID for node "master" is 820423
...
Cleary, Postgres can't find injection_points extension.
Am I doing something wrong, or it is a problem with injection points extension
itself?
--
Best regards,
Maxim Orlov.
Maxim Orlov <orlovmg@gmail.com> writes:
After rebasing one of my old patches, I'm hit to a problem with the
installcheck test for 041_checkpoint_at_promote.pl.
src/test/recovery/README points out that
Also, to use "make installcheck", you must have built and installed
contrib/pg_prewarm, contrib/pg_stat_statements and contrib/test_decoding
in addition to the core code.
I suspect this needs some additional verbiage about also installing
src/test/modules/injection_points if you've enabled injection points.
(I think we haven't noticed because most people just use "make check"
instead.)
regards, tom lane
On Mon, 19 Aug 2024 at 19:10, Tom Lane <tgl@sss.pgh.pa.us> wrote:
src/test/recovery/README points out that
Also, to use "make installcheck", you must have built and installed
contrib/pg_prewarm, contrib/pg_stat_statements and contrib/test_decoding
in addition to the core code.I suspect this needs some additional verbiage about also installing
src/test/modules/injection_points if you've enabled injection points.(I think we haven't noticed because most people just use "make check"
instead.)
OK, many thanks for a comprehensive explanation!
--
Best regards,
Maxim Orlov.
Shame on me, I didn't mention one thing in the original email. Actually,
the problem starts for me with "make installcheck-world".
And only then I've run a specific test 041_checkpoint_at_promote.pl. I.e.
the whole sequence of the commands are:
configure --enable_injection_points ...
make world
make install-world
initdb ...
pg_ctl ... start
make installcheck-world
And all the tests passes perfectly, for example, in REL_15_STABLE because
there is no opt like enable_injection_points.
But this is not the case if we are dealing with the current master branch.
So, my point here: installcheck-world doesn't
work on the current master branch until I explicitly install
injection_points extension. In my view, it's a bit wired, since
neither test_decoding, pg_stat_statements or pg_prewarm demand it.
--
Best regards,
Maxim Orlov.
Maxim Orlov <orlovmg@gmail.com> writes:
So, my point here: installcheck-world doesn't
work on the current master branch until I explicitly install
injection_points extension. In my view, it's a bit wired, since
neither test_decoding, pg_stat_statements or pg_prewarm demand it.
Ugh. The basic issue here is that "make install-world" doesn't
install anything from underneath src/test/modules, which I recall
as being an intentional decision. Rather than poking a hole in
that policy for injection_points, I wonder if we should move it
to contrib.
regards, tom lane
I wrote:
Ugh. The basic issue here is that "make install-world" doesn't
install anything from underneath src/test/modules, which I recall
as being an intentional decision. Rather than poking a hole in
that policy for injection_points, I wonder if we should move it
to contrib.
... which would also imply writing documentation and so forth,
and it'd mean that injection_points starts to show up in end-user
installations. (That would happen with the alternative choice of
hacking install-world to include src/test/modules/injection_points,
too.) While you could argue that that'd be helpful for extension
authors who'd like to use injection_points in their own tests, I'm
not sure that it's where we want to go with that module. It's only
meant as test scaffolding, and I don't think we've analyzed the
implications of some naive user installing it.
We do, however, need to preserve the property that installcheck
works after install-world. I'm starting to think that maybe
the 041 test should be hacked to silently skip if it doesn't
find injection_points available. (We could then remove some of
the makefile hackery that's supporting the current behavior.)
Probably the same needs to happen in each other test script
that's using injection_points --- I imagine that Maxim's
test is simply failing here first.
regards, tom lane
On 2024-Aug-20, Tom Lane wrote:
We do, however, need to preserve the property that installcheck
works after install-world. I'm starting to think that maybe
the 041 test should be hacked to silently skip if it doesn't
find injection_points available.
Yeah, I like this option. Injection points require to be explicitly
enabled in configure, so skipping that test when injection_points can't
be found seems reasonable.
This also suggests that EXTRA_INSTALL should have injection_points only
when the option is enabled.
I've been curious about what exactly does this Makefile line
export enable_injection_points enable_injection_points
achieve.
--
Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/
On Tue, Aug 20, 2024 at 12:10:08PM -0400, Tom Lane wrote:
... which would also imply writing documentation and so forth,
and it'd mean that injection_points starts to show up in end-user
installations. (That would happen with the alternative choice of
hacking install-world to include src/test/modules/injection_points,
too.) While you could argue that that'd be helpful for extension
authors who'd like to use injection_points in their own tests, I'm
not sure that it's where we want to go with that module. It's only
meant as test scaffolding, and I don't think we've analyzed the
implications of some naive user installing it.
The original line of thoughts here is that if I were to write a test
that relies on injection points for a critical bug fix, then I'd
rather not have to worry about the hassle of maintaining user-facing
documentation to get the work done. The second line is that we should
be able to tweak the module or even break its ABI as much as we want
in stable branches to accomodate to the tests, which is what a test
module is good for. The same ABI argument kind of stands as well for
the backend portion, but we'll see where it goes.
We do, however, need to preserve the property that installcheck
works after install-world. I'm starting to think that maybe
the 041 test should be hacked to silently skip if it doesn't
find injection_points available. (We could then remove some of
the makefile hackery that's supporting the current behavior.)
Probably the same needs to happen in each other test script
that's using injection_points --- I imagine that Maxim's
test is simply failing here first.
Yeah, we could do that.
--
Michael
On Tue, Aug 20, 2024 at 12:30:35PM -0400, Alvaro Herrera wrote:
Yeah, I like this option. Injection points require to be explicitly
enabled in configure, so skipping that test when injection_points can't
be found seems reasonable.
My apologies for the delay in doing something here.
The simplest thing would be to scan pg_available_extensions after the
first node is started, like in the attached. What do you think?
This also suggests that EXTRA_INSTALL should have injection_points only
when the option is enabled.
If the switch is disabled, the path is just ignored, so I don't see
any harm in keeping it listed all the time.
I've been curious about what exactly does this Makefile line
export enable_injection_points enable_injection_points
achieve.
Without this line, the TAP tests would not be able to know if
injection points are enabled or not, no? Well, it is true that we
could also do something like a scan of pg_config.h in the installation
path, but this is consistent with what ./configure uses.
--
Michael
Attachments:
0001-Implement-workaround-to-avoid-installcheck-failure.patchtext/x-diff; charset=us-asciiDownload
From 7441e32d8d7a4737a1c37dbb9e8b8f1a51a5f5c3 Mon Sep 17 00:00:00 2001
From: Michael Paquier <michael@paquier.xyz>
Date: Fri, 23 Aug 2024 12:33:27 +0900
Subject: [PATCH] Implement workaround to avoid installcheck failure
---
src/test/recovery/t/041_checkpoint_at_promote.pl | 11 +++++++++++
1 file changed, 11 insertions(+)
diff --git a/src/test/recovery/t/041_checkpoint_at_promote.pl b/src/test/recovery/t/041_checkpoint_at_promote.pl
index 5aa05b456c..905662353d 100644
--- a/src/test/recovery/t/041_checkpoint_at_promote.pl
+++ b/src/test/recovery/t/041_checkpoint_at_promote.pl
@@ -35,6 +35,17 @@ restart_after_crash = on
]);
$node_primary->start;
+# Check if the extension injection_points is available, as it may be
+# possible that this script is run with installcheck, where the module
+# would not be installed by default.
+my $result = $node_primary->safe_psql('postgres',
+ "SELECT count(*) > 0 FROM pg_available_extensions WHERE name = 'injection_points';"
+);
+if ($result eq 'f')
+{
+ plan skip_all => 'Extension injection_points not installed';
+}
+
my $backup_name = 'my_backup';
$node_primary->backup($backup_name);
--
2.45.2
On 2024-Aug-23, Michael Paquier wrote:
On Tue, Aug 20, 2024 at 12:30:35PM -0400, Alvaro Herrera wrote:
Yeah, I like this option. Injection points require to be explicitly
enabled in configure, so skipping that test when injection_points can't
be found seems reasonable.My apologies for the delay in doing something here.
The simplest thing would be to scan pg_available_extensions after the
first node is started, like in the attached. What do you think?
Hmm, I think you could test whether "--enable-injection-points" is in
the pg_config output in $node->config_data('--configure'). You don't
need to have started the node for that.
This also suggests that EXTRA_INSTALL should have injection_points only
when the option is enabled.If the switch is disabled, the path is just ignored, so I don't see
any harm in keeping it listed all the time.
Okay.
I've been curious about what exactly does this Makefile line
export enable_injection_points enable_injection_points
achieve.Without this line, the TAP tests would not be able to know if
injection points are enabled or not, no? Well, it is true that we
could also do something like a scan of pg_config.h in the installation
path, but this is consistent with what ./configure uses.
Right, I figured out afterwards that what that does is export the
make-level variable as an environment variable.
--
Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/
Alvaro Herrera <alvherre@alvh.no-ip.org> writes:
On 2024-Aug-23, Michael Paquier wrote:
I've been curious about what exactly does this Makefile line
export enable_injection_points enable_injection_points
achieve.
Without this line, the TAP tests would not be able to know if
injection points are enabled or not, no?
Right, I figured out afterwards that what that does is export the
make-level variable as an environment variable.
It exports it twice, though, which is pretty confusing.
regards, tom lane
By the way, we have the same kind of "problem" with the meson build. As we
are deliberately
not want to install src/test/modules, after b6a0d469cae and 0d237aeebaee we
must add step
"meson compile install-test-files" in order to "meson test -q --setup
running" to be successful.
To be honest, this step is not obvious. Especially than there was no such
step before. But
docs and https://wiki.postgresql.org/wiki/Meson are completely silenced
about it.
--
Best regards,
Maxim Orlov.
On Fri, Aug 23, 2024 at 06:45:02PM -0400, Tom Lane wrote:
It exports it twice, though, which is pretty confusing.
Right. I am not sure what was my state of mind back then, but this
pattern has spread a bit.
REL_17_STABLE is frozen for a few more days, so I'll address all the
items of this thread that once the release of this week is tagged: the
export duplicates and the installcheck issue. These are staged on a
local branch for now.
--
Michael
On Mon, Sep 02, 2024 at 09:53:30AM +0900, Michael Paquier wrote:
REL_17_STABLE is frozen for a few more days, so I'll address all the
items of this thread that once the release of this week is tagged: the
export duplicates and the installcheck issue. These are staged on a
local branch for now.
There are two TAP tests in REL_17_STABLE that use the module
injection_points, and we have 6 of them on HEAD. For now, I have
applied a patch that addresses the problems for v17 to avoid any
problems with the upcoming release, fixing the two tests that exist in
REL_17_STABLE.
For HEAD, I'd like to be slightly more ambitious and propose a routine
in Cluster.pm that scans for available extensions, as of the attached.
This simplifies the injection point test in libpq, as the
injection_point is one portion of the test so we don't need the check
based on the environment variable. There is no need for checks in the
TAP tests injection_points's 001_stats.pl and test_slru's
001_multixact.pl as these modules disable installcheck.
Any thoughts about the attached? This makes installcheck work here
with and without the configure switch.
--
Michael
Attachments:
0001-Check-availability-of-module-injection_points-in-TAP.patchtext/x-diff; charset=us-asciiDownload
From 74deadf240a521ef774dbb540f411e2bccb5b421 Mon Sep 17 00:00:00 2001
From: Michael Paquier <michael@paquier.xyz>
Date: Wed, 4 Sep 2024 09:38:36 +0900
Subject: [PATCH] Check availability of module injection_points in TAP tests
This fixes various defects with installcheck for TAP tests that expect
the module to exist in an installation, but it is not installed by
default with installcheck.
The check is refactored as a new routine in Cluster.pm.
---
src/interfaces/libpq/Makefile | 2 +-
src/interfaces/libpq/meson.build | 1 -
.../libpq/t/005_negotiate_encryption.pl | 7 ++++--
src/test/modules/test_misc/t/005_timeouts.pl | 5 +----
.../test_misc/t/006_signal_autovacuum.pl | 9 ++++++++
src/test/perl/PostgreSQL/Test/Cluster.pm | 22 +++++++++++++++++++
.../recovery/t/041_checkpoint_at_promote.pl | 5 +----
7 files changed, 39 insertions(+), 12 deletions(-)
diff --git a/src/interfaces/libpq/Makefile b/src/interfaces/libpq/Makefile
index 27f8499d8a..c1bf33dbdc 100644
--- a/src/interfaces/libpq/Makefile
+++ b/src/interfaces/libpq/Makefile
@@ -15,7 +15,7 @@ subdir = src/interfaces/libpq
top_builddir = ../../..
include $(top_builddir)/src/Makefile.global
-export with_ssl with_gssapi with_krb_srvnam enable_injection_points
+export with_ssl with_gssapi with_krb_srvnam
PGFILEDESC = "PostgreSQL Access Library"
diff --git a/src/interfaces/libpq/meson.build b/src/interfaces/libpq/meson.build
index 7623aeadab..ed2a4048d1 100644
--- a/src/interfaces/libpq/meson.build
+++ b/src/interfaces/libpq/meson.build
@@ -121,7 +121,6 @@ tests += {
't/005_negotiate_encryption.pl',
],
'env': {
- 'enable_injection_points': get_option('injection_points') ? 'yes' : 'no',
'with_ssl': ssl_library,
'with_gssapi': gssapi.found() ? 'yes' : 'no',
'with_krb_srvnam': 'postgres',
diff --git a/src/interfaces/libpq/t/005_negotiate_encryption.pl b/src/interfaces/libpq/t/005_negotiate_encryption.pl
index 73f0056f10..06d67de2db 100644
--- a/src/interfaces/libpq/t/005_negotiate_encryption.pl
+++ b/src/interfaces/libpq/t/005_negotiate_encryption.pl
@@ -90,8 +90,6 @@ my $kerberos_enabled =
$ENV{PG_TEST_EXTRA} && $ENV{PG_TEST_EXTRA} =~ /\bkerberos\b/;
my $ssl_supported = $ENV{with_ssl} eq 'openssl';
-my $injection_points_supported = $ENV{enable_injection_points} eq 'yes';
-
###
### Prepare test server for GSSAPI and SSL authentication, with a few
### different test users and helper functions. We don't actually
@@ -151,6 +149,11 @@ if ($ssl_supported != 0)
$node->start;
+# Check if the extension injection_points is available, as it may be
+# possible that this script is run with installcheck, where the module
+# would not be installed by default.
+my $injection_points_supported = $node->check_extension('injection_points');
+
$node->safe_psql('postgres', 'CREATE USER localuser;');
$node->safe_psql('postgres', 'CREATE USER testuser;');
$node->safe_psql('postgres', 'CREATE USER ssluser;');
diff --git a/src/test/modules/test_misc/t/005_timeouts.pl b/src/test/modules/test_misc/t/005_timeouts.pl
index 53e44016e3..d9b7219121 100644
--- a/src/test/modules/test_misc/t/005_timeouts.pl
+++ b/src/test/modules/test_misc/t/005_timeouts.pl
@@ -28,10 +28,7 @@ $node->start;
# Check if the extension injection_points is available, as it may be
# possible that this script is run with installcheck, where the module
# would not be installed by default.
-my $result = $node->safe_psql('postgres',
- "SELECT count(*) > 0 FROM pg_available_extensions WHERE name = 'injection_points';"
-);
-if ($result eq 'f')
+if (!$node->check_extension('injection_points'))
{
plan skip_all => 'Extension injection_points not installed';
}
diff --git a/src/test/modules/test_misc/t/006_signal_autovacuum.pl b/src/test/modules/test_misc/t/006_signal_autovacuum.pl
index 929253f754..aaea569c10 100644
--- a/src/test/modules/test_misc/t/006_signal_autovacuum.pl
+++ b/src/test/modules/test_misc/t/006_signal_autovacuum.pl
@@ -25,6 +25,15 @@ $node->init;
# This ensures a quick worker spawn.
$node->append_conf('postgresql.conf', 'autovacuum_naptime = 1');
$node->start;
+
+# Check if the extension injection_points is available, as it may be
+# possible that this script is run with installcheck, where the module
+# would not be installed by default.
+if (!$node->check_extension('injection_points'))
+{
+ plan skip_all => 'Extension injection_points not installed';
+}
+
$node->safe_psql('postgres', 'CREATE EXTENSION injection_points;');
$node->safe_psql(
diff --git a/src/test/perl/PostgreSQL/Test/Cluster.pm b/src/test/perl/PostgreSQL/Test/Cluster.pm
index fe6ebf10f7..143dc8c101 100644
--- a/src/test/perl/PostgreSQL/Test/Cluster.pm
+++ b/src/test/perl/PostgreSQL/Test/Cluster.pm
@@ -2837,6 +2837,28 @@ sub lsn
=pod
+=item $node->check_extension(extension_name)
+
+Scan pg_available_extensions to check that an extension is available in an
+installation.
+
+Returns 1 if the extension is available, 0 otherwise.
+
+=cut
+
+sub check_extension
+{
+ my ($self, $extension_name) = @_;
+
+ my $result = $self->safe_psql('postgres',
+ "SELECT count(*) > 0 FROM pg_available_extensions WHERE name = '$extension_name';"
+ );
+
+ return $result eq 't' ? 1 : 0;
+}
+
+=pod
+
=item $node->wait_for_event(wait_event_name, backend_type)
Poll pg_stat_activity until backend_type reaches wait_event_name.
diff --git a/src/test/recovery/t/041_checkpoint_at_promote.pl b/src/test/recovery/t/041_checkpoint_at_promote.pl
index 905662353d..3c21d18e3a 100644
--- a/src/test/recovery/t/041_checkpoint_at_promote.pl
+++ b/src/test/recovery/t/041_checkpoint_at_promote.pl
@@ -38,10 +38,7 @@ $node_primary->start;
# Check if the extension injection_points is available, as it may be
# possible that this script is run with installcheck, where the module
# would not be installed by default.
-my $result = $node_primary->safe_psql('postgres',
- "SELECT count(*) > 0 FROM pg_available_extensions WHERE name = 'injection_points';"
-);
-if ($result eq 'f')
+if (!$node_primary->check_extension('injection_points'))
{
plan skip_all => 'Extension injection_points not installed';
}
--
2.45.2
On Wed, 4 Sept 2024 at 03:40, Michael Paquier <michael@paquier.xyz> wrote:
Any thoughts about the attached? This makes installcheck work here
with and without the configure switch.
Works for me with configure build. Meson build, obviously, still need extra
"meson compile install-test-files" step
as expected. From your patch, I see that you used safe_psql call to check
for availability of the injection_points
extension. Are there some hidden, non-obvious reasons for it? It's much
simpler to check output of the
pg_config as Álvaro suggested above, does it? And we don't need active node
for this. Or I miss something?
And one other thing I must mention. I don't know why, but my pg_config from
meson build show empty configure
despite the fact, I make pass the same options in both cases.
autotools:
$ ./pg_config --configure
'--enable-debug' '--enable-tap-tests' '--enable-depend' ....
meson:
$ ./pg_config --configure
--
Best regards,
Maxim Orlov.
On Wed, Sep 04, 2024 at 07:05:32PM +0300, Maxim Orlov wrote:
Works for me with configure build. Meson build, obviously, still need extra
"meson compile install-test-files" step
as expected. From your patch, I see that you used safe_psql call to check
for availability of the injection_points
extension.
Are there some hidden, non-obvious reasons for it? It's much
simpler to check output of the
pg_config as Álvaro suggested above, does it? And we don't need active node
for this. Or I miss something?
Even if the code is compiled with injection points enabled, it's still
going to be necessary to check if the module exists in the
installation or not. And it may or may not be around.
One thing that we could do, rather than relying on the environment
variable for the compile-time check, would be to scan pg_config.h for
"USE_INJECTION_POINTS 1". If it is set, we could skip the use of these
environment variables. That's really kind of the same thing for
with_ssl or similar depending on the dependencies that exist. So
that's switching one thing to the other. I am not sure that's worth
switching at this stage. It does not change the need of a runtime
check to make sure that the module is installed, though.
Another thing that we could do, rather than query
pg_available_extension, is implementing an equivalent on the perl
side, scanning an installation tree for a module .so or equivalent,
but I've never been much a fan of the extra maintenance burden these
duplications introduce, copying what the backend is able to handle
already in a more transparent way for the TAP routines.
And one other thing I must mention. I don't know why, but my pg_config from
meson build show empty configure
despite the fact, I make pass the same options in both cases.autotools:
$ ./pg_config --configure
'--enable-debug' '--enable-tap-tests' '--enable-depend' ....meson:
$ ./pg_config --configure
Yes, ./configure does not apply to meson, so I'm not sure what we
should do here, except perhaps inventing a new option switch that
reports the options that have been used with the meson command, or
something like that.
--
Michael
On Thu, 5 Sept 2024 at 04:59, Michael Paquier <michael@paquier.xyz> wrote:
Even if the code is compiled with injection points enabled, it's still
going to be necessary to check if the module exists in the
installation or not. And it may or may not be around.
OK, thanks for an explanation, I get it.
--
Best regards,
Maxim Orlov.