Thread: pgsql: Add function to pump IPC process until string match
Add function to pump IPC process until string match Refactor the recovery tests to not carry a local duplicated copy of the pump_until function which pumps a process until a defined string is seen on a stream. This reduces duplication, and is in preparation for another patch which will also use this functionality. Reviewed-by: Michael Paquier <michael@paquier.xyz> Discussion https://postgr.es/m/YgynUafCyIu3jIhC@paquier.xyz Branch ------ master Details ------- https://git.postgresql.org/pg/commitdiff/6da65a3f9a9deae4fdcc768c612b0c8f52759f75 Modified Files -------------- src/test/perl/PostgreSQL/Test/Utils.pm | 23 +++++++++++++++ src/test/recovery/t/013_crash_restart.pl | 46 +++++++---------------------- src/test/recovery/t/022_crash_temp_files.pl | 45 +++++----------------------- 3 files changed, 41 insertions(+), 73 deletions(-)
Hi, On 2022-02-23 13:32:03 +0000, Daniel Gustafsson wrote: > Add function to pump IPC process until string match > > Refactor the recovery tests to not carry a local duplicated copy of > the pump_until function which pumps a process until a defined string > is seen on a stream. This reduces duplication, and is in preparation > for another patch which will also use this functionality. I'm a bit disappointed by the moved function not having the diagnostic output that at least the version in 013_crash_restart.pl had. How is one supposed to figure out what caused a timeout with the new central version? Given that timeouts are the only way tests using pump_until() fail, that's not great. Greetings, Andres Freund
> On 30 Mar 2022, at 00:58, Andres Freund <andres@anarazel.de> wrote:
> On 2022-02-23 13:32:03 +0000, Daniel Gustafsson wrote:
>> Add function to pump IPC process until string match
>>
>> Refactor the recovery tests to not carry a local duplicated copy of
>> the pump_until function which pumps a process until a defined string
>> is seen on a stream. This reduces duplication, and is in preparation
>> for another patch which will also use this functionality.
>
> I'm a bit disappointed by the moved function not having the diagnostic output
> that at least the version in 013_crash_restart.pl had. How is one supposed to
> figure out what caused a timeout with the new central version?
Thats my bad then. Since we don't really have diag output in any module I was
going off that "precedent" when moving this over.
> Given that timeouts are the only way tests using pump_until() fail, that's not
> great.
They can also fail on the pumped process crashing/exiting before the timeout
without emitting the expected output.
Would adding back something like the (right now untested) below be what you're
after? Looking at the perldoc I didn't see any other debugging aids we can
emit other than the stream and type of error.
diff --git a/src/test/perl/PostgreSQL/Test/Utils.pm b/src/test/perl/PostgreSQL/Test/Utils.pm
index 15b314d1f8..693f2f02e5 100644
--- a/src/test/perl/PostgreSQL/Test/Utils.pm
+++ b/src/test/perl/PostgreSQL/Test/Utils.pm
@@ -426,8 +426,16 @@ sub pump_until
while (1)
{
last if $$stream =~ /$until/;
- return 0 if ($timeout->is_expired);
- return 0 if (not $proc->pumpable());
+ if ($timeout->is_expired)
+ {
+ diag("pump_until: timeout expired with stream: \"$$stream\"");
+ return 0;
+ }
+ if (not $proc->pumpable())
+ {
+ diag("pump_until: process terminated unexpectedly with stream: \"$$stream\"");
+ return 0
+ }
$proc->pump();
}
return 1;
--
Daniel Gustafsson https://vmware.com/
On 2022-Mar-30, Daniel Gustafsson wrote: > Would adding back something like the (right now untested) below be what you're > after? Looking at the perldoc I didn't see any other debugging aids we can > emit other than the stream and type of error. I think you could also show the $until that wasn't found. The old .pl versions of this did that. -- Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/
Hi, On 2022-03-30 09:10:17 +0200, Daniel Gustafsson wrote: > > On 30 Mar 2022, at 00:58, Andres Freund <andres@anarazel.de> wrote: > > On 2022-02-23 13:32:03 +0000, Daniel Gustafsson wrote: > > >> Add function to pump IPC process until string match > >> > >> Refactor the recovery tests to not carry a local duplicated copy of > >> the pump_until function which pumps a process until a defined string > >> is seen on a stream. This reduces duplication, and is in preparation > >> for another patch which will also use this functionality. > > > > I'm a bit disappointed by the moved function not having the diagnostic output > > that at least the version in 013_crash_restart.pl had. How is one supposed to > > figure out what caused a timeout with the new central version? > > Thats my bad then. Since we don't really have diag output in any module I was > going off that "precedent" when moving this over. We have a few places that manually output diagnostic stuff by printing with # etc. > > Given that timeouts are the only way tests using pump_until() fail, that's not > > great. > > They can also fail on the pumped process crashing/exiting before the timeout > without emitting the expected output. Fair. > Would adding back something like the (right now untested) below be what you're > after? Looking at the perldoc I didn't see any other debugging aids we can > emit other than the stream and type of error. I found showing the regex from before useful. Often there's many pump_until()'s, and the regex makes it easier to figure out what errored out. And it shows quoting problem. Since it only happens when there was a failure, it's not like it'll bloat the log unduly. Greetings, Andres Freund
> On 30 Mar 2022, at 18:09, Andres Freund <andres@anarazel.de> wrote: >> Would adding back something like the (right now untested) below be what you're >> after? Looking at the perldoc I didn't see any other debugging aids we can >> emit other than the stream and type of error. > > I found showing the regex from before useful. Often there's many > pump_until()'s, and the regex makes it easier to figure out what errored > out. And it shows quoting problem. Since it only happens when there was a > failure, it's not like it'll bloat the log unduly. I'll add diag output on failures along the lines of the diff from earlier today but with the regex as well (which Alvarao also requested), but tomorrow morning once coffee has been consumed. -- Daniel Gustafsson https://vmware.com/
On 2022-03-31 00:00:52 +0200, Daniel Gustafsson wrote: > I'll add diag output on failures along the lines of the diff from earlier today > but with the regex as well (which Alvarao also requested), but tomorrow morning > once coffee has been consumed. Thanks for doing that!