Horton
Menu

Thread

Commits

Same data as JSON: GET /api/v1/messages/:b64id/commits the thread's linked commits as JSON, with link sources. API reference →

  1. Use command_ok for pg_regress calls in 002_pg_upgrade and 027_stream_regress

  2. perl tap: Show die reason in TAP output

  3. perl tap: Show failed command output

  4. perl tap: Use croak instead of die in our helper modules

  5. pg_regress: Include diffs in TAP output

  1. meson: Make test output much more useful on failure (both in CI and locally)

    Jelte Fennema <postgres@jeltef.nl> — 2026-01-26T10:13:57Z

    I've gotten totally fed up with the amount of clicking I need to do
every time a test fails in CI to find out what exactly failed. The
output that meson gives in its main output is completely useless
currently. Which means I have to go click through CirrusCI its (pretty
terrible) filebrowser UI searching for log files or regression.diffs.

And locally I have the exact same problem when a perl test is failing.
Meson often shows no useful output at all, which means I have to find
some unknown log file somewhere in my build directory. And then I have
to find the useful spot in that log file that actually tells me what the
error was.

So attached are a few small patches that greatly improve the test output
of the most common test failure reasons (at least the ones I tend to run
into myself).

An example CI run with some intentional failures (attached as the
nocfbot patch) can be found here:
https://cirrus-ci.com/task/6592551433535488

  2. Re: meson: Make test output much more useful on failure (both in CI and locally)

    Zsolt Parragi <zsolt.parragi@percona.com> — 2026-01-26T15:36:30Z

    Hello,

This is really useful!

- my $result = run_log($cmd);
- ok($result, $test_name);
+ # Doesn't rely on detecting end of file on the file descriptors,
+ # which can fail, causing the process to hang, notably on Msys
+ # when used with 'pg_ctl start'

The function comments still refer to run_log, for both command_ok and
command_fails

  3. Re: meson: Make test output much more useful on failure (both in CI and locally)

    Andres Freund <andres@anarazel.de> — 2026-01-26T16:42:16Z

    Hi,

On 2026-01-26 11:13:57 +0100, Jelte Fennema-Nio wrote:
> I've gotten totally fed up with the amount of clicking I need to do
> every time a test fails in CI to find out what exactly failed. The
> output that meson gives in its main output is completely useless
> currently.

This doesn't really seem meson specific, right? It just seems about the output
of our own test tooling?

Meson can't really output something that we hide from it...

I agree that what we currently do is pretty unhelpful.


> Subject: [PATCH v1 1/5] meson: Include pg_regress diffs in meson output

This isn't about meson, it's about adding a new argument to pg_regress.


I think we'd need to limit the size of the output here somewhat. If e.g. the
server crashes, you end up with all subsequent tests failing and printing out
a couple hundred kB of pointless diff output.


> @@ -1521,22 +1523,40 @@ results_differ(const char *testname, const char *resultsfile, const char *defaul
>  	 * append to the diffs summary file.
>  	 */
>  
> -	/* Write diff header */
> -	difffile = fopen(difffilename, "a");
> +	difffile = fopen(difffilename, "a+");
>  	if (difffile)
>  	{
> +		fseek(difffile, 0, SEEK_END);
> +		startpos = ftell(difffile);
>  		fprintf(difffile,
>  				"diff %s %s %s\n",
>  				pretty_diff_opts, best_expect_file, resultsfile);
> +		fflush(difffile);
> +
> +		/* Run diff */
> +		snprintf(cmd, sizeof(cmd),
> +				 "diff %s \"%s\" \"%s\" >> \"%s\"",
> +				 pretty_diff_opts, best_expect_file, resultsfile, difffilename);
> +		run_diff(cmd, difffilename);
> +
> +		/* Emit diff as TAP diagnostics if requested */
> +		if (print_diffs)
> +		{
> +			char		line[1024];
> +
> +			fseek(difffile, startpos, SEEK_SET);
> +			while (fgets(line, sizeof(line), difffile))
> +			{
> +				size_t		len = strlen(line);
> +
> +				if (len > 0 && line[len - 1] == '\n')
> +					line[len - 1] = '\0';
> +				diag("%s", line);
> +			}
> +		}
>  		fclose(difffile);
>  	}

I'm a bit confused. Why are we rerunning diffs and appending to the diff file?


> From c1db07a8bba54e6b903766585f1b521ef526de12 Mon Sep 17 00:00:00 2001
> From: Jelte Fennema-Nio <postgres@jeltef.nl>
> Date: Mon, 26 Jan 2026 09:09:11 +0100
> Subject: [PATCH v1 2/5] perl tap: Show failed command output
> 
> This adds the output of failed commands to the TAP output. Before a
> failed libpq_pipeline test would look like this:
> 
>   Failed test 'libpq_pipeline cancel'
>   at /home/jelte/work/postgres-3/src/test/modules/libpq_pipeline/t/001_libpq_pipeline.pl line 55.
> 
> Now you can actually see the reason of the failure:
> 
>   Failed test 'libpq_pipeline cancel'
>   at /home/jelte/work/postgres-3/src/test/modules/libpq_pipeline/t/001_libpq_pipeline.pl line 55.
> ----- command failed -----
> libpq_pipeline -r 700 cancel port=14309 host=/tmp/htMib451qD dbname='postgres' max_protocol_version=latest
> --------- stderr ---------
> test cancellations...
> libpq_pipeline:315: unexpected number of rows received: 1
> --------------------------
> ---
>  src/test/perl/PostgreSQL/Test/Utils.pm | 36 +++++++++++++++++++++++---
>  1 file changed, 32 insertions(+), 4 deletions(-)
> 
> diff --git a/src/test/perl/PostgreSQL/Test/Utils.pm b/src/test/perl/PostgreSQL/Test/Utils.pm
> index ff843eecc6e..bd1e981c6f0 100644
> --- a/src/test/perl/PostgreSQL/Test/Utils.pm
> +++ b/src/test/perl/PostgreSQL/Test/Utils.pm
> @@ -955,8 +955,22 @@ sub command_ok
>  {
>  	local $Test::Builder::Level = $Test::Builder::Level + 1;
>  	my ($cmd, $test_name) = @_;
> -	my $result = run_log($cmd);
> -	ok($result, $test_name);
> +	# Doesn't rely on detecting end of file on the file descriptors,
> +	# which can fail, causing the process to hang, notably on Msys
> +	# when used with 'pg_ctl start'
> +	my $stdoutfile = File::Temp->new();
> +	my $stderrfile = File::Temp->new();

I'm afraid that the creation of this many additional tempfiles would slow down
the tests on some platforms...

> +	my $result = IPC::Run::run $cmd, '>' => $stdoutfile, '2>' => $stderrfile;
> +	ok($result, $test_name) or do
> +	{
> +		my $stdout = slurp_file($stdoutfile);
> +		my $stderr = slurp_file($stderrfile);
> +		diag("----- command failed -----");
> +		diag(join(" ", @$cmd));
> +		diag("--------- stdout ---------"), diag($stdout) if $stdout;
> +		diag("--------- stderr ---------"), diag($stderr) if $stderr;
> +		diag("--------------------------");
> +	};

Outputting the entire file seems to again have the danger of completely
swamping the output.

>  	return;
>  }
>  
> @@ -972,8 +986,22 @@ sub command_fails
>  {
>  	local $Test::Builder::Level = $Test::Builder::Level + 1;
>  	my ($cmd, $test_name) = @_;
> -	my $result = run_log($cmd);
> -	ok(!$result, $test_name);
> +	# Doesn't rely on detecting end of file on the file descriptors,
> +	# which can fail, causing the process to hang, notably on Msys
> +	# when used with 'pg_ctl start'

Huh? Any more details?

> +	my $stdoutfile = File::Temp->new();
> +	my $stderrfile = File::Temp->new();
> +	my $result = IPC::Run::run $cmd, '>' => $stdoutfile, '2>' => $stderrfile;
> +	ok(!$result, $test_name) or do
> +	{
> +		my $stdout = slurp_file($stdoutfile);
> +		my $stderr = slurp_file($stderrfile);
> +		diag("-- command succeeded unexpectedly --");
> +		diag(join(" ", @$cmd));
> +		diag("-------------- stdout --------------"), diag($stdout) if $stdout;
> +		diag("-------------- stderr --------------"), diag($stderr) if $stderr;
> +		diag("------------------------------------");
> +	};

I don't think it makes sense to duplicate the logic for this multiple times.


> From c88f1fbf6462da56a1f0739dd83a6284d6bdd389 Mon Sep 17 00:00:00 2001
> From: Jelte Fennema-Nio <postgres@jeltef.nl>
> Date: Mon, 26 Jan 2026 09:32:15 +0100
> Subject: [PATCH v1 3/5] perl tap: Show die reason in TAP output
>...
> diff --git a/src/test/perl/PostgreSQL/Test/Utils.pm b/src/test/perl/PostgreSQL/Test/Utils.pm
> index bd1e981c6f0..df328d0f571 100644
> --- a/src/test/perl/PostgreSQL/Test/Utils.pm
> +++ b/src/test/perl/PostgreSQL/Test/Utils.pm
> @@ -206,6 +206,14 @@ INIT
>  	# test may still fail, but it's more likely to report useful facts.
>  	$SIG{PIPE} = 'IGNORE';
>  
> +	# Emit die messages as TAP diagnostics so they appear in test output.
> +	$SIG{__DIE__} = sub {
> +		return if $^S;    # Ignore dies inside eval
> +		my $msg = shift;
> +		chomp $msg;
> +		diag("die: $msg");
> +	};
> +

This looked familiar - turns out I had complained about this in the past and
then forgotten about it :(

https://www.postgresql.org/message-id/20220222181924.eehi7o4pmneeb4hm%40alap3.anarazel.de

At least at the time I found it necessary to separately test for $^S, to avoid
triggering the logic for syntax errors.  Not sure why I concluded that though.

In a reply Andrew suggested to use done_testing(), which avoids the pointless
  "Tests were run but no plan was declared and done_testing() was not seen."
which does seem like an improvement.


> From a6b324678293d65f3a417b8f66889b3f3f6a9c29 Mon Sep 17 00:00:00 2001
> From: Jelte Fennema-Nio <postgres@jeltef.nl>
> Date: Mon, 26 Jan 2026 10:04:44 +0100
> Subject: [PATCH v1 4/5] perl tap: Include caller in die messages of psql
> 
> This way by looking at the error output you can actually figure out
> where it occured, instead of just knowing that is in the very often
> called psql function.

I think this is too localized a fix. Right now every die in a .pm file has
this issue.  The easiest fix would probably be to just replace all uses of die
in .pm files with croak (which we already, inconsistently, use).

stderr:
# die: error running SQL: 'psql:<stdin>:1: ERROR:  syntax error at or near "I"
# LINE 1: I am the worst
#         ^'
# while running 'psql --no-psqlrc --no-align --tuples-only --quiet --dbname port=20878 host=/tmp/K5V48mzFx9 dbname='postgres' --file - --variable ON_ERROR_STOP=1' with sql 'I am the worst' at /home/andres/src/postgresql/src/bin/scripts/t/020_createdb.pl line 19.

Note that the failure now is reported as coming from the actual test file.


We could also use Test::More->builder->croak(), but that's a heck of a lot
longer, and would require us to be more liberal about sprinkling

	local $Test::Builder::Level = $Test::Builder::Level + 1;
around. I don't see an improvement in the output either.

In contrast Carp.pm just looks for a caller outside of the current package. If
we are worried about errors across Cluster.pm/Utils.pm, we could just fix that
in one place by using @CARP_NOT.

Maybe there's a way to do that more centrally?

Greetings,

Andres Freund

  4. Re: meson: Make test output much more useful on failure (both in CI and locally)

    Jelte Fennema <postgres@jeltef.nl> — 2026-01-26T17:46:28Z

    On Mon, 26 Jan 2026 at 17:42, Andres Freund <andres@anarazel.de> wrote:
> This doesn't really seem meson specific, right? It just seems about the output
> of our own test tooling?

Yes, it's general improvements. But the problem is worse in meson than
for because that really only shows the TAP output. The Makefile based
builds will show a bit more information currently.

> I think we'd need to limit the size of the output here somewhat. If e.g. the
> server crashes, you end up with all subsequent tests failing and printing out
> a couple hundred kB of pointless diff output.

Fair enough.

> I'm a bit confused. Why are we rerunning diffs and appending to the diff file?

Now I'm confused. We're running the same amount of diffs as before.
Did you miss the removal of the /* Run diff */ below the block that
you quoted?

> I'm afraid that the creation of this many additional tempfiles would slow down
> the tests on some platforms...
< snip>
> > @@ -972,8 +986,22 @@ sub command_fails
> >  {
> >       local $Test::Builder::Level = $Test::Builder::Level + 1;
> >       my ($cmd, $test_name) = @_;
> > -     my $result = run_log($cmd);
> > -     ok(!$result, $test_name);
> > +     # Doesn't rely on detecting end of file on the file descriptors,
> > +     # which can fail, causing the process to hang, notably on Msys
> > +     # when used with 'pg_ctl start'
>
> Huh? Any more details?

I copy pasted that comment and the pipe to tempfiles from
command_like_safe. I did this after I initially tried to pipe to
variables, but that made the test that we run in SanityCheck get stuck
forever.

I don't get why this is needed either.

> > +     my $stdoutfile = File::Temp->new();
> > +     my $stderrfile = File::Temp->new();
> > +     my $result = IPC::Run::run $cmd, '>' => $stdoutfile, '2>' => $stderrfile;
> > +     ok(!$result, $test_name) or do
> > +     {
> > +             my $stdout = slurp_file($stdoutfile);
> > +             my $stderr = slurp_file($stderrfile);
> > +             diag("-- command succeeded unexpectedly --");
> > +             diag(join(" ", @$cmd));
> > +             diag("-------------- stdout --------------"), diag($stdout) if $stdout;
> > +             diag("-------------- stderr --------------"), diag($stderr) if $stderr;
> > +             diag("------------------------------------");
> > +     };
>
> I don't think it makes sense to duplicate the logic for this multiple times.

It seemed a bit much to make it generic over the input, as it would
need to remove dashes accordingly. Especially since it only has two
copies.

> This looked familiar - turns out I had complained about this in the past and
> then forgotten about it :(
>
> https://www.postgresql.org/message-id/20220222181924.eehi7o4pmneeb4hm%40alap3.anarazel.de
>
> At least at the time I found it necessary to separately test for $^S, to avoid
> triggering the logic for syntax errors.  Not sure why I concluded that though.
>
> In a reply Andrew suggested to use done_testing(), which avoids the pointless
>   "Tests were run but no plan was declared and done_testing() was not seen."
> which does seem like an improvement.

I'll take a look at doing that.

> I think this is too localized a fix. Right now every die in a .pm file has
> this issue.  The easiest fix would probably be to just replace all uses of die
> in .pm files with croak (which we already, inconsistently, use).

I didn't even know that function existed (due to my pretty much
non-existent knowlegde of perl).

  5. Re: meson: Make test output much more useful on failure (both in CI and locally)

    Jelte Fennema <postgres@jeltef.nl> — 2026-01-27T14:48:55Z

    On Mon Jan 26, 2026 at 5:42 PM CET, Andres Freund wrote:
> I agree that what we currently do is pretty unhelpful.

Attached is v2, which I think fixes all the issues that you mentioned.

  6. Re: meson: Make test output much more useful on failure (both in CI and locally)

    Nazir Bilal Yavuz <byavuz81@gmail.com> — 2026-02-03T10:20:26Z

    Hi,

Thank you for working on this! I think this is a nice improvement.

On Tue, 27 Jan 2026 at 17:48, Jelte Fennema-Nio <postgres@jeltef.nl> wrote:
>
> On Mon Jan 26, 2026 at 5:42 PM CET, Andres Freund wrote:
> > I agree that what we currently do is pretty unhelpful.
>
> Attached is v2, which I think fixes all the issues that you mentioned.

v2-0001:

This doesn't really read the first 80 lines of the combined diff.
Lines with more than 1023 characters will be counted and printed as
multiple lines. I don't think this is a problem but clearing this
would be helpful. Other than that, it looks good to me.

v2-0002:

I am just curious if test failure can cause file descriptors not being
properly closed and then the IPC::Run:run() call will hang
indefinitely like the pg_ctl case. Other than that, it looks good to
me.

v2-0003 and v2-0004 look good to me.

-- 
Regards,
Nazir Bilal Yavuz
Microsoft

  7. Re: meson: Make test output much more useful on failure (both in CI and locally)

    Jelte Fennema <postgres@jeltef.nl> — 2026-02-06T08:44:12Z

    On Tue, 3 Feb 2026 at 11:20, Nazir Bilal Yavuz <byavuz81@gmail.com> wrote:
> v2-0001:
>
> This doesn't really read the first 80 lines of the combined diff.
> Lines with more than 1023 characters will be counted and printed as
> multiple lines. I don't think this is a problem but clearing this
> would be helpful. Other than that, it looks good to me.

I was annoyed by this, so I actually fixed it (manually tested by
changing the buffer to 10 bytes). This meant I needed to introduce
DIAG_DETAIL and DIAG_END, like we already had NOTE_DETAIL and NOTE_END.

> v2-0002:
>
> I am just curious if test failure can cause file descriptors not being
> properly closed and then the IPC::Run:run() call will hang
> indefinitely like the pg_ctl case. Other than that, it looks good to
> me.

No, this issue only happens because for the 'pg_ctl restart' process,
its the child postmaster process outlives the pg_ctl process, WHILE
still having the stdout/stderr file descriptor open. As long as all the
subprocesses exit, the file descriptor will be closed by the OS
automatically. So, this problem really only occurs in these daemon
spawning situations. Which for us in practice is only for pg_ctl
start/restart.

  8. Re: meson: Make test output much more useful on failure (both in CI and locally)

    Andrew Dunstan <andrew@dunslane.net> — 2026-03-02T18:25:08Z

    On 2026-02-06 Fr 3:44 AM, Jelte Fennema-Nio wrote:
> On Tue, 3 Feb 2026 at 11:20, Nazir Bilal Yavuz <byavuz81@gmail.com> 
> wrote:
>> v2-0001:
>>
>> This doesn't really read the first 80 lines of the combined diff.
>> Lines with more than 1023 characters will be counted and printed as
>> multiple lines. I don't think this is a problem but clearing this
>> would be helpful. Other than that, it looks good to me.
>
> I was annoyed by this, so I actually fixed it (manually tested by
> changing the buffer to 10 bytes). This meant I needed to introduce
> DIAG_DETAIL and DIAG_END, like we already had NOTE_DETAIL and NOTE_END.
>
>> v2-0002:
>>
>> I am just curious if test failure can cause file descriptors not being
>> properly closed and then the IPC::Run:run() call will hang
>> indefinitely like the pg_ctl case. Other than that, it looks good to
>> me.
>
> No, this issue only happens because for the 'pg_ctl restart' process,
> its the child postmaster process outlives the pg_ctl process, WHILE
> still having the stdout/stderr file descriptor open. As long as all the
> subprocesses exit, the file descriptor will be closed by the OS
> automatically. So, this problem really only occurs in these daemon
> spawning situations. Which for us in practice is only for pg_ctl
> start/restart.
>



I've had a brief look at these. They generally look OK - I didn't see 
any obvious issues.


cheers


andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

  9. Re: meson: Make test output much more useful on failure (both in CI and locally)

    Zsolt Parragi <zsolt.parragi@percona.com> — 2026-03-04T22:56:09Z

    - my $result = run_log($cmd);
- ok($result, $test_name);
+ my ($stdout, $stderr);
+ my $result = IPC::Run::run $cmd, '>' => \$stdout, '2>' => \$stderr;
+ ok($result, $test_name) or do
+ {
+ diag("---------- command failed ----------");
+ _diag_command_output($cmd, $stdout, $stderr);
+ };

Previously this printed a line about running the command even on
success, and the new version removes that. Was this intentional? It's
not mentioned in the commit message and that seems like a useful
feature to me.

For example:

Before:
grep "# Run" build/testrun/initdb/001_initdb/log/regress_log_001_initdb | wc -l
35

After:
grep "# Run" build/testrun/initdb/001_initdb/log/regress_log_001_initdb | wc -l
11

  10. Re: meson: Make test output much more useful on failure (both in CI and locally)

    Jelte Fennema <postgres@jeltef.nl> — 2026-03-17T10:23:26Z

    On Wed Mar 4, 2026 at 11:56 PM CET, Zsolt Parragi wrote:
> Previously this printed a line about running the command even on
> success, and the new version removes that. Was this intentional? It's
> not mentioned in the commit message and that seems like a useful
> feature to me.

Fixed this now. I also fixed two off-by-one errors in counting the
number of outputted lines (found by local AI review).

And I also changed the logic to write to the diff file to close and
re-open, because I realized that on Windows the new logic would create
these errors when trying to write to the diffs file:

[09:00:49.251] stderr:
[09:00:49.251] The process cannot access the file because it is being used by another process.
[09:00:49.251] The process cannot access the file because it is being used by another process.
[09:00:49.251] The process cannot access the file because it is being used by another process.
[09:00:49.251] The process cannot access the file because it is being used by another process.
[09:00:49.251] # 4 of 239 tests failed.
[09:00:49.251] # The differences that caused some tests to fail can be viewed in the file "C:/cirrus/build/testrun/regress/regress/regression.diffs".
[09:00:49.251] # A copy of the test summary that you see above is saved in the file "C:/cirrus/build/testrun/regress/regress/regression.out".
[09:00:49.251] 

Finally I added a new commit which starts to use command_ok in
002_pg_upgrade.pl and 027_stream_regress.pl to get the new nicer output.

  11. Re: meson: Make test output much more useful on failure (both in CI and locally)

    Corey Huinker <corey.huinker@gmail.com> — 2026-03-19T19:44:17Z

    >
>
> Finally I added a new commit which starts to use command_ok in
> 002_pg_upgrade.pl and 027_stream_regress.pl to get the new nicer output.
>

I tried this out, and am very pleased with the results.

I added an obvious error to a regression output file and, now I see:

# -------------- stderr --------------
# # diff -U3
/home/corey/src/postgres/src/test/regress/expected/stats_import.out
/home/corey/src/postgres/build/testrun/recovery/027_stream_regress/data/results/stats_import.out
# # --- /home/corey/src/postgres/src/test/regress/expected/stats_import.out
2026-03-19 15:20:35.413217825 -0400
# # +++
/home/corey/src/postgres/build/testrun/recovery/027_stream_regress/data/results/stats_import.out
2026-03-19 15:20:57.565988863 -0400
# # @@ -25,8 +25,6 @@
# #      31
# #  (1 row)
# #
# # --- OBVIOUS ERROR IS OBVIOUS
# # -
# #  -- Create a view that is used purely for the type based on
pg_statistic.
# #  CREATE VIEW stats_import.pg_statistic_flat_t AS
# #    SELECT
# # 1 of 243 tests failed.
# # The differences that caused some tests to fail can be viewed in the
file
"/home/corey/src/postgres/build/testrun/recovery/027_stream_regress/data/regression.diffs".
# # A copy of the test summary that you see above is saved in the file
"/home/corey/src/postgres/build/testrun/recovery/027_stream_regress/data/regression.out".
# ------------------------------------
# Looks like you failed 1 test of 11.

(test program exited with status code 1)
―――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――


That's great, because ofttimes I want to find the diff -U3 line, replace
the command with "meld" (a graphical diff tool) and then examine the
differences from there.

v4-0001:

+ * useful information is actually in the first few lines

nitpick: comment sentence needs a closing '.'

v4-0002, v4-0003, v4-0004: look good to me

v4-0005: the commit message is probably too bulky to be used as-is, but the
committer was going to change that anyway and right now too much
information is way better than not enough.

I can't wait to use this.

  12. Re: meson: Make test output much more useful on failure (both in CI and locally)

    Jelte Fennema <postgres@jeltef.nl> — 2026-03-20T08:19:19Z

    On Thu Mar 19, 2026 at 8:44 PM CET, Corey Huinker wrote:
>>
>>
>> Finally I added a new commit which starts to use command_ok in
>> 002_pg_upgrade.pl and 027_stream_regress.pl to get the new nicer output.
>>
>
> I tried this out, and am very pleased with the results.

Thanks for the review.

> nitpick: comment sentence needs a closing '.'

Fixed

> v4-0005: the commit message is probably too bulky to be used as-is, but the
> committer was going to change that anyway and right now too much
> information is way better than not enough.

Shortened it using your example run.

> I can't wait to use this.

Me too :) I've also added the Author/Reviewed-By/Discussion footers to
the commits to make the committers job easier.

  13. Re: meson: Make test output much more useful on failure (both in CI and locally)

    Corey Huinker <corey.huinker@gmail.com> — 2026-03-20T18:13:13Z

    >
> Shortened it using your example run.
>

Glad I was able to help.


> > I can't wait to use this.
>
> Me too :) I've also added the Author/Reviewed-By/Discussion footers to
> the commits to make the committers job easier.
>

I've held off on doing that in my proposed commits so as not to be
presumptuous, but I can see where having it available would be a
convenience for the committer. This will be a good test of that.

Applies clean to master, passes tests. Ship it.

  14. Re: meson: Make test output much more useful on failure (both in CI and locally)

    Andrew Dunstan <andrew@dunslane.net> — 2026-04-02T12:25:58Z

    On 2026-03-20 Fr 2:13 PM, Corey Huinker wrote:
>
>     Shortened it using your example run.
>
>
> Glad I was able to help.
>
>     > I can't wait to use this.
>
>     Me too :) I've also added the Author/Reviewed-By/Discussion footers to
>     the commits to make the committers job easier.
>
>
> I've held off on doing that in my proposed commits so as not to be 
> presumptuous, but I can see where having it available would be a 
> convenience for the committer. This will be a good test of that.
>
> Applies clean to master, passes tests. Ship it.


Committed with minor tidy up. The main change was to add a @CARP_NOT 
setting in Utils.pm, so that croak() would look back past Cluster.pm to 
the TAP script caller.


cheers


andrew


--
Andrew Dunstan
EDB:https://www.enterprisedb.com

  15. Re: meson: Make test output much more useful on failure (both in CI and locally)

    Jelte Fennema <postgres@jeltef.nl> — 2026-04-02T13:39:43Z

    On Thu, 2 Apr 2026 at 14:26, Andrew Dunstan <andrew@dunslane.net> wrote:
> Committed with minor tidy up. The main change was to add a @CARP_NOT setting in Utils.pm, so that croak() would look back past Cluster.pm to the TAP script caller.

Thanks!

  16. Re: meson: Make test output much more useful on failure (both in CI and locally)

    Peter Eisentraut <peter@eisentraut.org> — 2026-04-07T13:15:23Z

    On 02.04.26 14:25, Andrew Dunstan wrote:
> 
> On 2026-03-20 Fr 2:13 PM, Corey Huinker wrote:
>>
>>     Shortened it using your example run.
>>
>>
>> Glad I was able to help.
>>
>>     > I can't wait to use this.
>>
>>     Me too :) I've also added the Author/Reviewed-By/Discussion footers to
>>     the commits to make the committers job easier.
>>
>>
>> I've held off on doing that in my proposed commits so as not to be 
>> presumptuous, but I can see where having it available would be a 
>> convenience for the committer. This will be a good test of that.
>>
>> Applies clean to master, passes tests. Ship it.
> 
> 
> Committed with minor tidy up. The main change was to add a @CARP_NOT 
> setting in Utils.pm, so that croak() would look back past Cluster.pm to 
> the TAP script caller.

I would like to register a vote against this new behavior:

     pg_regress: Include diffs in TAP output

     When pg_regress fails it is often tedious to find the actual diffs,
     especially in CI where you must navigate a file browser.  Emit the 
first
     80 lines of the combined regression.diffs as TAP diagnostics so the
     failure reason is visible directly in the test output.

I find this annoying.

What happens, and this is admittedly my particular experience, is that 
the diff lines are wider than the terminal width, and so 80 lines in the 
file might end up being 200 lines on screen, and then the summary of the 
test failure disappears from the screen and the diff output is garbled 
and useless, and so the whole output is now less useful than before.

I could see this maybe being useful if the entire diff file is, say, 
less than 50 lines.  But I don't see how seeing a truncated diff by 
default can be useful.

The commit message makes reference to "especially on CI".  Maybe this 
new behavior should be triggered by being on CI, or the output not being 
a terminal, or something like that.

  17. Re: meson: Make test output much more useful on failure (both in CI and locally)

    Jelte Fennema <postgres@jeltef.nl> — 2026-04-07T14:25:47Z

    On Tue, 7 Apr 2026 at 15:15, Peter Eisentraut <peter@eisentraut.org> wrote:
> I could see this maybe being useful if the entire diff file is, say,
> less than 50 lines.  But I don't see how seeing a truncated diff by
> default can be useful.

The idea behind that is that if it's a big diff it's often because the
server crashed. And the top of the diff will then tell you which query
crashed the server. Similarly if some important command like CREATE
TABLE failed, the diff after that is also useless.

Are you saying that 80 diff lines are too many? And to limit it to 40
or 50 instead by default. Could you try changing the number locally
and if that would address the annoyance? Or would it only be solved by
having no diff output at all? And would being able to configure the
amount of lines using a env variable help?

> The commit message makes reference to "especially on CI".  Maybe this
> new behavior should be triggered by being on CI, or the output not being
> a terminal, or something like that.

I definitely want the diff in my terminal too, when testing locally.
There are far too many regression.diffs file in my build directory.

  18. Re: meson: Make test output much more useful on failure (both in CI and locally)

    Peter Eisentraut <peter@eisentraut.org> — 2026-04-08T23:47:44Z

    On 07.04.26 16:25, Jelte Fennema-Nio wrote:
> On Tue, 7 Apr 2026 at 15:15, Peter Eisentraut <peter@eisentraut.org> wrote:
>> I could see this maybe being useful if the entire diff file is, say,
>> less than 50 lines.  But I don't see how seeing a truncated diff by
>> default can be useful.
> 
> The idea behind that is that if it's a big diff it's often because the
> server crashed. And the top of the diff will then tell you which query
> crashed the server.

That is not my experience.  Often times, when a test out of a group of 
20 crashes, then the crashing test will hang for a while while the core 
file is being written out, while the other 19 tests fail immediately 
because of the server restart.  And so the top of the diff will just 
show all the other tests aborting, not the actual crashing test.

> Are you saying that 80 diff lines are too many? And to limit it to 40
> or 50 instead by default. Could you try changing the number locally
> and if that would address the annoyance? Or would it only be solved by
> having no diff output at all? And would being able to configure the
> amount of lines using a env variable help?

80 is too many.  I have a reasonably big monitor, and my echo $LINES 
says 60.  On the laptop screen it's 40.  And you need to leave some room 
so that the summary of the test run can be shown, so like 10 lines.  So 
I think 25 or 30 is about the maximum that would be reasonable.  (Or you 
actually take getenv("LINES") into account.)

But again, one of the problems is that the diff lines could be longer 
than the terminal width, and then these computations no longer work.  If 
you look at the diffs with "less" or similar, then you can manage that. 
If you could print the start of the diff while truncating it to the 
terminal line length, then that could work.


Independent of that, there appears to be a technical problem with the 
implementation.  Consider a recent failed build on the buildfarm:

https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=dodo&dt=2026-04-07%2012%3A45%3A07

The buildfarm output actually handles this quite nicely: The bottom of 
the output contains the test result summary, and if you want to know 
more detail, you click on the links right below.

But note that the additional diff output appears somewhere in the middle 
of the test run.  There appears to be a problem synchronizing multiple 
output streams.

(But also note that this new feature is possibly redundant with what the 
buildfarm output already provides, so it might be worth configuring this 
differently for the buildfarm client.)

  19. Re: meson: Make test output much more useful on failure (both in CI and locally)

    Andrew Dunstan <andrew@dunslane.net> — 2026-04-09T09:21:21Z

    On 2026-04-07 Tu 9:15 AM, Peter Eisentraut wrote:
> On 02.04.26 14:25, Andrew Dunstan wrote:
>>
>> On 2026-03-20 Fr 2:13 PM, Corey Huinker wrote:
>>>
>>>     Shortened it using your example run.
>>>
>>>
>>> Glad I was able to help.
>>>
>>>     > I can't wait to use this.
>>>
>>>     Me too :) I've also added the Author/Reviewed-By/Discussion 
>>> footers to
>>>     the commits to make the committers job easier.
>>>
>>>
>>> I've held off on doing that in my proposed commits so as not to be 
>>> presumptuous, but I can see where having it available would be a 
>>> convenience for the committer. This will be a good test of that.
>>>
>>> Applies clean to master, passes tests. Ship it.
>>
>>
>> Committed with minor tidy up. The main change was to add a @CARP_NOT 
>> setting in Utils.pm, so that croak() would look back past Cluster.pm 
>> to the TAP script caller.
>
> I would like to register a vote against this new behavior:
>
>     pg_regress: Include diffs in TAP output
>
>     When pg_regress fails it is often tedious to find the actual diffs,
>     especially in CI where you must navigate a file browser.  Emit the 
> first
>     80 lines of the combined regression.diffs as TAP diagnostics so the
>     failure reason is visible directly in the test output.
>
> I find this annoying.
>
> What happens, and this is admittedly my particular experience, is that 
> the diff lines are wider than the terminal width, and so 80 lines in 
> the file might end up being 200 lines on screen, and then the summary 
> of the test failure disappears from the screen and the diff output is 
> garbled and useless, and so the whole output is now less useful than 
> before.
>
> I could see this maybe being useful if the entire diff file is, say, 
> less than 50 lines.  But I don't see how seeing a truncated diff by 
> default can be useful.
>
> The commit message makes reference to "especially on CI".  Maybe this 
> new behavior should be triggered by being on CI, or the output not 
> being a terminal, or something like that.


How about something like this, which would only trigger the behaviour if 
an environment variable is set. Also adds that env setting to 
cirrus.tasks.yml.


cheers


andrew


--
Andrew Dunstan
EDB: https://www.enterprisedb.com

  20. Re: meson: Make test output much more useful on failure (both in CI and locally)

    Alexander Lakhin <exclusion@gmail.com> — 2026-04-24T17:00:00Z

    Hello hackers,

07.04.2026 16:15, Peter Eisentraut wrote:
> On 02.04.26 14:25, Andrew Dunstan wrote:
>> Committed with minor tidy up. The main change was to add a @CARP_NOT setting in Utils.pm, so that croak() would look 
>> back past Cluster.pm to the TAP script caller.
>
> I would like to register a vote against this new behavior:

I've noticed another disadvantage of this log processing: [1] shows the
misc_functions test's failure, but we can't see timing of the test anymore:
# ok 27        + line                                     6713 ms
# ... 210 lines omitted ...
# ok 223       + reloptions                              18169 ms

Previously, a test report for the same failure contained:
ok 139       + merge                                   19699 ms
not ok 140   + misc_functions                          18003 ms
ok 141       + sysviews                                 7925 ms

[1] https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=drongo&dt=2026-04-24%2001%3A26%3A26

Best regards,10
Alexander

  21. Re: meson: Make test output much more useful on failure (both in CI and locally)

    Jelte Fennema <postgres@jeltef.nl> — 2026-05-17T20:15:55Z

    On Thu Apr 9, 2026 at 11:21 AM CEST, Andrew Dunstan wrote:
> How about something like this, which would only trigger the behaviour if 
> an environment variable is set. Also adds that env setting to 
> cirrus.tasks.yml.

That approach has the problem that then we don't see the diffs anymore
when pg_regress is called from TAP tests. Attached is a patch that I
think addresses the problems with the original version.

P.S. For PG20 I intend to submit a patchset that improves the output of
pg_regress in case of backend crashes. I have a bunch of ideas, but most
of them require some changes to psql and libpq too.

  22. Re: meson: Make test output much more useful on failure (both in CI and locally)

    Jelte Fennema <postgres@jeltef.nl> — 2026-05-18T15:26:05Z

    On Fri Apr 24, 2026 at 7:00 PM CEST, Alexander Lakhin wrote:
> I've noticed another disadvantage of this log processing: [1] shows the
> misc_functions test's failure, but we can't see timing of the test anymore:
> # ok 27        + line                                     6713 ms
> # ... 210 lines omitted ...
> # ok 223       + reloptions                              18169 ms
>
> Previously, a test report for the same failure contained:
> ok 139       + merge                                   19699 ms
> not ok 140   + misc_functions                          18003 ms
> ok 141       + sysviews                                 7925 ms

I didn't address this in my v6 (yet). Primarily because this isn't
related to the pg_regress changes, but rather to my change that started
using command_ok in these tests to run pg_regress. I'm thinking a bit if
we can make this nicer, one way would be to have command_ok take an
optional argument that allows disabling truncation in places where we
know the output won't be huge.

  23. Re: meson: Make test output much more useful on failure (both in CI and locally)

    Andrew Dunstan <andrew@dunslane.net> — 2026-05-24T22:18:40Z

    On 2026-05-17 Su 4:15 PM, Jelte Fennema-Nio wrote:
> On Thu Apr 9, 2026 at 11:21 AM CEST, Andrew Dunstan wrote:
>> How about something like this, which would only trigger the behaviour 
>> if an environment variable is set. Also adds that env setting to 
>> cirrus.tasks.yml.
>
> That approach has the problem that then we don't see the diffs anymore
> when pg_regress is called from TAP tests. Attached is a patch that I
> think addresses the problems with the original version.


Why can't the TAP sets set it?


cheers


andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

  24. Re: meson: Make test output much more useful on failure (both in CI and locally)

    Jelte Fennema <postgres@jeltef.nl> — 2026-05-25T06:20:16Z

    On Mon, May 25, 2026, 00:18 Andrew Dunstan <andrew@dunslane.net> wrote:

> Why can't the TAP sets set it?
>

The TAP tests could, but then they'd be set indiscriminately for TAP tests.
Which means Peter's screen will still be flooded when he runs the TAP
tests.

In general I'd rather make the feature work well enough by default that
people don't want to disable it.

>

  25. Re: meson: Make test output much more useful on failure (both in CI and locally)

    Christoph Berg <myon@debian.org> — 2026-05-26T19:19:49Z

    Re: Jelte Fennema-Nio
> 5. Reduce the default diff truncation limit to 50.

Could we also get a "show everything" mode, perhaps based on an env var?

Debian's build and test machinery doesn't provide means to get at
files from the build/test run, so all evidence needs to be in the
build log. The debian/rules file for the PG server currently has this
contraption:

override_dh_auto_test-arch:
ifeq (, $(findstring nocheck, $(DEB_BUILD_OPTIONS)))
	# when tests fail, print newest log files
	# initdb doesn't like LANG and LC_ALL to contradict, unset LANG and LC_CTYPE here
	# temp-install wants to be invoked from a top-level make, unset MAKELEVEL here
	# tell pg_upgrade to create its sockets in /tmp to avoid too long paths
	unset LANG LC_CTYPE MAKELEVEL; ulimit -c unlimited; \
	if ! make -C build check-world \
	  PGSOCKETDIR="/tmp" \
	  PG_TEST_EXTRA='ssl' \
	  PROVE_FLAGS="--verbose"; \
	then \
	    for l in `find build -name 'regression.*' -o -name '*.log' -o -name '*_log_*' | perl -we 'print map { "$$_\n"; } sort { (stat $$a)[9] <=> (stat $$b)[9] } map { chomp; $$_; } <>' | tail -n 10`; do \
		echo "******** $$l ********"; \
		cat $$l; \
	    done; \
	    for c in `find build -name 'core*'`; do \
	        echo "******** $$c ********"; \
	        gdb -batch -ex 'bt full' build/tmp_install/usr/lib/postgresql/$(MAJOR_VER)/bin/postgres $$c || :; \
	    done; \
	    $(TEST_FAIL_COMMAND); \
	fi
ifeq ($(WITH_PG_BSD_INDENT),y)
	$(MAKE) -C build/src/tools/pg_bsd_indent test DESTDIR=$(CURDIR)/debian/tmp
endif
endif

The part with the TAP output files is ugly, and the "sort files by
mtime and show the 10 newest ones" even more so. I would welcome
anything that allowed me to simplify this.

Thanks,
Christoph