Horton

Thread

Commits

  1. Check availability of module injection_points in TAP tests

  2. Avoid installcheck failure in TAP tests using injection_points

  3. Simplify makefiles exporting twice enable_injection_points

  1. Test 041_checkpoint_at_promote.pl faild in installcheck due to missing injection_points

    Maxim Orlov <orlovmg@gmail.com> — 2024-08-19T15:00:32Z

    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.

  2. Re: Test 041_checkpoint_at_promote.pl faild in installcheck due to missing injection_points

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

    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

  3. Re: Test 041_checkpoint_at_promote.pl faild in installcheck due to missing injection_points

    Maxim Orlov <orlovmg@gmail.com> — 2024-08-20T07:29:42Z

    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.

  4. Re: Test 041_checkpoint_at_promote.pl faild in installcheck due to missing injection_points

    Maxim Orlov <orlovmg@gmail.com> — 2024-08-20T11:40:10Z

     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.

  5. Re: Test 041_checkpoint_at_promote.pl faild in installcheck due to missing injection_points

    Tom Lane <tgl@sss.pgh.pa.us> — 2024-08-20T15:04:07Z

    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

  6. Re: Test 041_checkpoint_at_promote.pl faild in installcheck due to missing injection_points

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

    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

  7. Re: Test 041_checkpoint_at_promote.pl faild in installcheck due to missing injection_points

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2024-08-20T16:30:35Z

    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/

  8. Re: Test 041_checkpoint_at_promote.pl faild in installcheck due to missing injection_points

    Michael Paquier <michael@paquier.xyz> — 2024-08-20T23:25:20Z

    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

  9. Re: Test 041_checkpoint_at_promote.pl faild in installcheck due to missing injection_points

    Michael Paquier <michael@paquier.xyz> — 2024-08-23T03:37:59Z

    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

  10. Re: Test 041_checkpoint_at_promote.pl faild in installcheck due to missing injection_points

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2024-08-23T22:25:41Z

    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/

  11. Re: Test 041_checkpoint_at_promote.pl faild in installcheck due to missing injection_points

    Tom Lane <tgl@sss.pgh.pa.us> — 2024-08-23T22:45:02Z

    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

  12. Re: Test 041_checkpoint_at_promote.pl faild in installcheck due to missing injection_points

    Maxim Orlov <orlovmg@gmail.com> — 2024-08-28T06:14:35Z

    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.

  13. Re: Test 041_checkpoint_at_promote.pl faild in installcheck due to missing injection_points

    Michael Paquier <michael@paquier.xyz> — 2024-09-02T00:53:30Z

    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

  14. Re: Test 041_checkpoint_at_promote.pl faild in installcheck due to missing injection_points

    Michael Paquier <michael@paquier.xyz> — 2024-09-04T00:40:01Z

    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

  15. Re: Test 041_checkpoint_at_promote.pl faild in installcheck due to missing injection_points

    Maxim Orlov <orlovmg@gmail.com> — 2024-09-04T16:05:32Z

    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.

  16. Re: Test 041_checkpoint_at_promote.pl faild in installcheck due to missing injection_points

    Michael Paquier <michael@paquier.xyz> — 2024-09-05T01:59:13Z

    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

  17. Re: Test 041_checkpoint_at_promote.pl faild in installcheck due to missing injection_points

    Maxim Orlov <orlovmg@gmail.com> — 2024-09-05T16:08:58Z

    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.