Hi Fujii-san,
> I've started reviewing this patch since it's marked as ready for committer.
Thanks!
> + my $pump_timeout =
> + IPC::Run::timer($PostgreSQL::Test::Utils::timeout_default);
>
> This timer is never started or passed to IPC::Run::start(), so it seems
> it can never expire. Right? If so, isn't it better to reuse the existing
> timeout for pump_until()?
I referred the existing code in the same file, like [1].
But you seem right - perl doc [2] describes start() can register a timer object
to the harness and starts it, pump() checks the result. pump() internally calls
start() but calling it alone is not enough because timer must be registered.
It meant existing codes might also be fixed, right?
> + ok(pump_until($slot_user_handle, $pump_timeout, $to_stderr,
> + qr/streaming initiated/),
> + "pg_recvlogical started streaming");
>
> This allows the test to continue even if pump_until() returns false.
> Since streaming is a prerequisite for the subsequent steps, we should
> use or die(), as the old code did?
+1.
> I also found a pre-existing typo: the slots are created with the prefix
> "drop_db_", but their removal is checked using "drop_db". Since
> those names never existed, the checks pass even if the actual slots
> remain. Attached 0002 fixes this.
Good catch, LGTM.
[1]
```
# check that we are decoding pre and post promotion inserted rows
# with pg_recvlogical that has started before the promotion
my $pump_timeout = IPC::Run::timer($PostgreSQL::Test::Utils::timeout_default);
ok(pump_until($handle, $pump_timeout, \$stdout, qr/^.*COMMIT.*COMMIT$/s),
'got 2 COMMIT from pg_recvlogical output');
```
[2]: https://metacpan.org/pod/IPC::Run
Best regards,
Hayato Kuroda
FUJITSU LIMITED
