pgsql: Add function to pump IPC process until string match

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!