Greetings, everyone!
If you call node->psql in not-array context, with on_error_die => 1,
but without passing stderr, you will get the following error
and the test will die, but not the way we expect:
Use of uninitialized value in concatenation (.) or string at
/path/to/source_code/src/test/perl/PostgreSQL/Test/Cluster.pm line 2258
This is because $$stderr is not defined in this case
and warnings became FATAL some time ago.
The code string in question for clarity:
...
die
"error running SQL: '$$stderr'\n..."
if $ret == 3;
Minimal reproduction is:
$node->psql('postgres', q{SELEC 1}, on_error_die => 1);
This can be reproduced at current master (30c15987)
Undefined $$stderr also should break dying on recieving a signal, here:
# We always die on signal.
if (defined $ret)
{
... die (".... $$stderr ...");
One of the ways to fix this is to initialize $$stderr with some value
to avoid Perl error (Use of uninitialized value) and replace it with
existing error: "error running SQL: '$$stderr'\n ..."
With this approach we don't lose any useful error messages in
regress_log_*
The proposed patch is attached (0001)
---------------------------------------------------------------------------
Another question I've stumbled upon when trying to fix the
aforementioned
issue is the following: Does redirecting IPC::Run::run streams work with
Postgres Perl module SimpleTee? I've tried to use it to "tee" STDERR to
both $$stderr and to test's regress_log_* file. I'm not sure I'm doing
this right, but I have not found a way to use SimpleTee with
IPC::Run::run
Is there a way to use them together?
I've tried something like (0002)
Regards, Oleg Tselebrovskiy
diff --git a/src/test/modules/test_misc/meson.build b/src/test/modules/test_misc/meson.build
index 9c50de7efb0..dff852de839 100644
--- a/src/test/modules/test_misc/meson.build
+++ b/src/test/modules/test_misc/meson.build
@@ -16,6 +16,7 @@ tests += {
't/005_timeouts.pl',
't/006_signal_autovacuum.pl',
't/007_catcache_inval.pl',
+ 't/008_psql_on_error_die.pl',
],
},
}
diff --git a/src/test/modules/test_misc/t/008_psql_on_error_die.pl b/src/test/modules/test_misc/t/008_psql_on_error_die.pl
new file mode 100644
index 00000000000..3590c9f6601
--- /dev/null
+++ b/src/test/modules/test_misc/t/008_psql_on_error_die.pl
@@ -0,0 +1,30 @@
+
+# Copyright (c) 2025, PostgreSQL Global Development Group
+
+# Tests that check that calling node->psql with on_error_die => 1 and without
+# passing $stderr will result in intuitive exception
+
+use strict;
+use warnings FATAL => 'all';
+use PostgreSQL::Test::Cluster;
+use PostgreSQL::Test::Utils;
+use Test::More;
+
+my $node = PostgreSQL::Test::Cluster->new('main');
+$node->init;
+$node->start;
+
+eval
+{
+ $node->psql('postgres', q{SELEC 1}, on_error_die => 1);
+};
+
+if ($@)
+{
+ print $@;
+ like($@, qr/<not captured>/, "Expected <not captured>");
+ unlike($@, qr/Use of uninitialized value in concatenation/,
+ "This is a 'warnings FATAL' output, we don't want this");
+}
+
+done_testing();
diff --git a/src/test/perl/PostgreSQL/Test/Cluster.pm b/src/test/perl/PostgreSQL/Test/Cluster.pm
index 1c11750ac1d..bc2eade8e8e 100644
--- a/src/test/perl/PostgreSQL/Test/Cluster.pm
+++ b/src/test/perl/PostgreSQL/Test/Cluster.pm
@@ -2235,6 +2235,14 @@ sub psql
chomp $$stderr;
}
+ # With use warnings FATAL => 'all' we could fail during the following dies
+ # due to undefined $$stderr (wantarray is false and stderr wasn't passed).
+ # Initialize it with a placeholder
+ if (!defined($$stderr))
+ {
+ $$stderr = "<not captured>";
+ }
+
# See http://perldoc.perl.org/perlvar.html#%24CHILD_ERROR
# We don't use IPC::Run::Simple to limit dependencies.
#
diff --git a/src/test/perl/PostgreSQL/Test/Cluster.pm b/src/test/perl/PostgreSQL/Test/Cluster.pm
index 1c11750ac1d..c1ba9de9723 100644
--- a/src/test/perl/PostgreSQL/Test/Cluster.pm
+++ b/src/test/perl/PostgreSQL/Test/Cluster.pm
@@ -2143,11 +2143,9 @@ sub psql
'--dbname' => $psql_connstr,
'--file' => '-');
- # If the caller wants an array and hasn't passed stdout/stderr
- # references, allocate temporary ones to capture them so we
- # can return them. Otherwise we won't redirect them at all.
- if (wantarray)
- {
+ # Always define undef stdout and stderr. This is needed for wantarray case,
+ # and stderr is used with params{on_error_die}, even if not undefined
+
if (!defined($stdout))
{
my $temp_stdout = "";
@@ -2158,7 +2156,15 @@ sub psql
my $temp_stderr = "";
$stderr = \$temp_stderr;
}
- }
+
+ my $fh_stdout = File::Temp->new();
+ my $fh_stderr = File::Temp->new();
+
+ open(my $orig_stdout, '>&', \*STDOUT) or die $!;
+ open(my $orig_stderr, '>&', \*STDERR) or die $!;
+
+ tie *fh_stdout, "PostgreSQL::Test::SimpleTee", $orig_stdout;
+ tie *fh_stderr, "PostgreSQL::Test::SimpleTee", $orig_stderr;
$params{on_error_stop} = 1 unless defined $params{on_error_stop};
$params{on_error_die} = 0 unless defined $params{on_error_die};
@@ -2191,8 +2197,8 @@ sub psql
local $@;
eval {
my @ipcrun_opts = (\@psql_params, '<' => \$sql);
- push @ipcrun_opts, '>' => $stdout if defined $stdout;
- push @ipcrun_opts, '2>' => $stderr if defined $stderr;
+ push @ipcrun_opts, '>' => $fh_stdout;
+ push @ipcrun_opts, '2>' => $fh_stderr;
push @ipcrun_opts, $timeout if defined $timeout;
IPC::Run::run @ipcrun_opts;
@@ -2225,6 +2231,9 @@ sub psql
}
};
+ $$stdout = PostgreSQL::Test::Utils::slurp_file($fh_stdout);
+ $$stderr = PostgreSQL::Test::Utils::slurp_file($fh_stderr);
+
if (defined $$stdout)
{
chomp $$stdout;
