On 09/04/2024 20:10, Daniel Gustafsson wrote:
Turning the timeout into a timer and returning undef along with logging a test
failure in case of expiration seems a bit saner (maybe Andrew can suggest an
API which has a better Perl feel to it). Most callsites don't need any changes
to accommodate for this, the attached 0002 implements this timer change and
modify the few sites that need it, converting one to plain query() where the
added complexity of query_until isn't required.
+1. The patch looks good to me. I didn't comb through the tests to check
for bugs of omission, i.e. cases where the caller would need adjustments
because of this, I trust that you found them all.
=item $session->quit
Close the session and clean up resources. Each test run must be closed with
C<quit>. Returns TRUE when the session was cleanly terminated, otherwise
FALSE. A testfailure will be issued in case the session failed to finish.
What does "session failed to finish" mean? Does it mean the same as "not
cleanly terminated", i.e. a test failure is issued whenever this returns
FALSE?
typo: testfailure -> test failure
diff --git a/src/test/recovery/t/031_recovery_conflict.pl
b/src/test/recovery/t/031_recovery_conflict.pl
index d87efa823fd..62936f52d20 100644
--- a/src/test/recovery/t/031_recovery_conflict.pl
+++ b/src/test/recovery/t/031_recovery_conflict.pl
@@ -253,9 +253,7 @@ $res = $psql_standby->query_until(
-- wait for lock held by prepared transaction
SELECT * FROM $table2;
]);
-ok(1,
- "$sect: cursor holding conflicting pin, also waiting for lock,
established"
-);
+isnt($res, undef, "$sect: cursor holding conflicting pin, also waiting for lock,
established");
# just to make sure we're waiting for lock already
ok( $node_standby->poll_query_until(
diff --git a/src/test/recovery/t/037_invalid_database.pl
b/src/test/recovery/t/037_invalid_database.pl
index 6d1c7117964..c8c20077f85 100644
--- a/src/test/recovery/t/037_invalid_database.pl
+++ b/src/test/recovery/t/037_invalid_database.pl
@@ -94,6 +94,7 @@ is($node->psql('postgres', 'DROP DATABASE
regression_invalid'),
my $cancel = $node->background_psql('postgres', on_error_stop => 1);
my $bgpsql = $node->background_psql('postgres', on_error_stop => 0);
my $pid = $bgpsql->query('SELECT pg_backend_pid()');
+isnt($pid, undef, 'Get backend PID');
# create the database, prevent drop database via lock held by a 2PC transaction
ok( $bgpsql->query_safe(
I'm not sure I understand these changes. These can only fail if the
query() or query_until() function times out, right? In that case, the
query() or query_until() would also report a test failure, so these
additional checks after the call seem redundant. They don't do any harm
either, but I wonder why have them in these particular call sites and
not in other query() or query_until() calls.
The tab completion test can use the API call for automatically restart the
timer to reduce the complexity of check_completion a hair. Done in 0001 (but
really not necessary).
+1
Commit Af279ddd1c2 added this sequence to 040_standby_failover_slots_sync.pl in
the recovery tests:
$back_q->query_until(
qr/logical_slot_get_changes/, q(
\echo logical_slot_get_changes
SELECT pg_logical_slot_get_changes('test_slot', NULL, NULL);
));
... <other tests> ...
# Since there are no slots in standby_slot_names, the function
# pg_logical_slot_get_changes should now return, and the session can be
# stopped.
$back_q->quit;
There is no guarantee that pg_logical_slot_get_changes has returned when
reaching this point. This might still work as intended, but the comment is
slightly misleading IMO.
Agreed, it would be good to actually check that it returns.
recovery/t/043_wal_replay_wait.pl calls pg_wal_replay_wait() since 06c418e163e
in a background session which it then skips terminating. Calling ->quit is
mandated by the API, in turn required by IPC::Run. Calling ->quit on the
process makes the test fail from the process having already exited, but we can
call ->finish directly like we do in test_misc/t/005_timeouts.pl. 0003 fixes
this.
Alexander included this fix in commit 3c5db1d6b016 already.
--
Heikki Linnakangas
Neon (https://neon.tech)
