Subject: Re: Minor meson gripe

From: Andres Freund <[email-masked]>
Date: Thu, 9 Feb 2023 12:56:05 -0800

Hi,

On 2023-02-09 11:01:31 -0800, Peter Geoghegan wrote:
> Currently, meson has a test suite named "setup". According to the
> Wiki, this is needed to get something equivalent to "make check", by
> running "meson test -v --suite setup --suite regress".

Yep.

> Some questions about this:
>
> * Isn't it confusing that we have a suite by that name, given that we
> also need to use the unrelated --setup flag for some nearby testing
> recipes?

Hm. I don't find it particularly confusing, but I don't think I'm a good judge
of that, too close.

> * Why do we actually need a "setup" suite?
>
> Offhand it appears that a simple "meson test -v --suite regress" works
> just as well. Have I missed something?

It'll work, but only if you have run setup before. And it'll not use changed C
code.

The setup suite creates the installation in tmp_install/. So if you haven't
run the tests before, it'll fail due to that missing. If you have run it
before, but have changed code, it'll not get used.

The background for the issue is that while meson test supports dependencies
for each test, and will build exactly the required dependencies if you run
individual tests with meson test, it unfortunately also adds all the test
dependencies to the default ninja target.

That's mostly for historical reasons, because initially meson didn't support
dependencies for tests. There's recent work on changing that though.

Creating the temp installation every time you run 'ninja' would not be
nice. On slower machines it can take quite a while.

I think medium term we should just stop requiring a temporary install to run
tests, it's substantial, unnecessary, overhead, and it requires us to build
way too much to run basic tests. It'd not take a whole lot to make that work:

- a search path for finding extensions, which'd be very useful for other
  reasons as well

- a way to tell 'postgres', 'initdb' etc, which use find_other_exec(), that
  they should use PATH
  
- a way to tell initdb where to find things like postgres.bki, postgres where
  it can find timezone data, etc.

Greetings,

Andres Freund

---

From: Peter Geoghegan <[email-masked]>
Date: Thu, 9 Feb 2023 15:34:34 -0800

> It'll work, but only if you have run setup before. And it'll not use changed C

I see. It's not that confusing on its own, but it does cause confusion
once you consider how things fit together. Suppose I want to do the
equivalent of running the amcheck tests -- the tests that run when
"make check" runs from contrib/amcheck with an autoconf build. That
looks like this with our current meson workflow:

meson test -v --suite setup --suite amcheck

Now consider what I have to run to get the equivalent of a "make
installcheck" run from the contrib/amcheck directory:

meson test -v --setup running --suite amcheck-running

Notice that I have to specify "--suite setup" in the first example,
whereas I have to specify "--setup running" in the second example
instead -- at the same point in. Also notice the rest of the details
almost match. This makes it quite natural to wonder if "--suite setup"
is related to "--setup running" in some way, which is not the case at
all. They're two wholly unrelated concepts.

Why not change the suite name to tmp_install? That immediately reminds
me of what's really going on here, since I'm used to seeing that
directory name. And it clashes with "--suite setup" in a way that
seems useful.

--
Peter Geoghegan

---

From: Andres Freund <[email-masked]>
Date: Thu, 9 Feb 2023 16:33:09 -0800

Hi,

> directory name. And it clashes with "--suite setup" in a way that

The individual test is actually named tmp_install. I thought it might be
useful to add further things to the setup "stage", hence the more general
name.

I e.g. have a not-quite-done patch that creates a "template initdb", which
pg_regress and tap tests automatically use (except if non-default options are
required), which quite noticably reduces test times (*).  But something needs to
create the template initdb, and initdb doesn't run without an installation, so
we need to run it after the temp_install.

Of course we could wrap that into one "test", but it seemed nicer to see if
you fail during installation, or during initdb. So for that I added a separate
test, that is also part of the setup suite.

Of course we could still name the suite tmp_install (or to be consistent with
the confusing make naming, have a temp-install target, which creates the
tmp_install directory :)). I guess that'd still be less confusing?

I'm not at all wedded to the "setup" name.

Greetings,

Andres Freund

* approximate test time improvements:

  local:
  688.67user 154.44system 1:08.29elapsed 1234%CPU (0avgtext+0avgdata 138984maxresident)k
  ->
  172.37user 109.43system 1:00.12elapsed 468%CPU (0avgtext+0avgdata 139168maxresident)k

  The 4x reduction in CPU cycles is pretty bonkers.  To bad wall clock time
  doesn't improve that much - we end up waiting for isolationtester,
  pg_upgrade tests to finish.

  CI freebsd: 06:30 -> 05:00
  CI linux, sanitize-address: 5:40->4:30
  CI linux, sanitize-undefined,alignment: 3:00->2:15
  CI windows 12:20 -> 10:00
  CI macos: 10:20 -> 08:00

  I expect it to very substantially speed up the valgrind builfarm animal. By
  far the most cycles are spent below initdb.

  https://github.com/anarazel/postgres/tree/initdb-caching

---

From: Peter Geoghegan <[email-masked]>
Date: Thu, 9 Feb 2023 17:00:48 -0800

> useful to add further things to the setup "stage", hence the more general

I did notice that, but only after sitting with my initial confusion for a while.

> test, that is also part of the setup suite.

But what are the chances that the setup / tmp_install "test" will
actually fail? It's almost a test in name only.

> tmp_install directory :)). I guess that'd still be less confusing?

Yes, that definitely seems like an improvement. I don't care about the
tiny inconsistency that this creates.

I wonder if this could be addressed by adding another custom test
setup, like --setup running, used whenever you want to just run one or
two tests against an ad-hoc temporary installation? Offhand it seems
as if add_test_setup() could support that requirement?

-- 
Peter Geoghegan

---

From: Andres Freund <[email-masked]>
Date: Thu, 9 Feb 2023 17:17:00 -0800

Hi,

> actually fail? It's almost a test in name only.

I've seen more failures than I'd like. Permission errors, conflicting names,
and similar things. But mainly that was a reference to running initdb, which
I've broken temporarily many a time.

> tiny inconsistency that this creates.

Then lets do that - feel free to push something, or send something for
review. Otherwise I'll try to get to it, but I owe a few people work before
this...

> as if add_test_setup() could support that requirement?

What precisely do you mean with "ad-hoc temporary installation"?

I was wondering about adding a different setup that'd use the "real"
installation to run tests. But perhaps that's something different than what
you have in mind?

The only restriction I see wrt add_test_setup() is that it's not entirely
trivial to use a "runtime-variable" path to an installation.

Greetings,

Andres Freund

---

From: Peter Geoghegan <[email-masked]>
Date: Thu, 9 Feb 2023 17:42:34 -0800

> I've broken temporarily many a time.

We've all temporarily broken initdb literally thousands of times, I'm sure.

> review. Otherwise I'll try to get to it, but I owe a few people work before

I'll try to get to it soon. Note that I've been adding new stuff to
the meson Wiki page, in the hope of saving other people the trouble of
figuring some of these details out for themselves. You might want to
take a look at that at some point.

> What precisely do you mean with "ad-hoc temporary installation"?

I mean "what make check does".

> installation to run tests. But perhaps that's something different than what

I wasn't thinking about the installation. Actually, I was, but the
thought went no further than "I wish I didn't have to think about the
installation". I liked that autoconf had "make check" and "make
installcheck" variants that worked in a low context way.

It's great that "meson test" runs all of the tests very quickly --
that should be maintained, even at some cost elsewhere. And it would
be nice to do away with the tmp_install thing. But as long as we have
it, it would be nice to make the way that we run a subset of test
suites against a running server similar to the way that we run a
subset of test suites against a throwaway installation (ala "make
check").

> trivial to use a "runtime-variable" path to an installation.

I personally have no problem with that, though of course I could have
easily overlooked something.

-- 
Peter Geoghegan