Re: IPC::Run::time[r|out] vs our TAP tests - Mailing list pgsql-hackers

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)