Date: Mon, 22 Jun 2020 13:40:48 -0500
From: Justin Pryzby <pryzby@telsasoft.com>
To: David Gilman <davidgilman1@gmail.com>
Cc: pgsql-hackers@lists.postgresql.org, Jim Woodworth <jimw54321@gmail.com>
Subject: Re: Warn when parallel restoring a custom dump without data offsets
Message-ID: <20200622184048.GX17995@telsasoft.com>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="K3vkeaB0MlFjg8U+"
Content-Disposition: inline
In-Reply-To: <20200622131951.GM17995@telsasoft.com>
 <20200621194225.GE17995@telsasoft.com>
 <20200613225121.GN14879@telsasoft.com>
User-Agent: Mutt/1.9.4 (2018-02-28)
Precedence: bulk


--K3vkeaB0MlFjg8U+
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline

Adding Jim, since he ask about helping with perl.

You can read the history of the patch here:

https://commitfest.postgresql.org/28/2568/
https://www.postgresql.org/message-id/flat/CALBH9DDuJ+scZc4MEvw5uO-=vRyR2=QF9+Yh=3hPEnKHWfS81A@mail.gmail.com

Some context:

David is adding a test case for a bugfix he made.
We want to test parallel pg_restore of a pg_dump created with a nonseekable FD.
I suggested to make the patch smaller by creating a nonseekable FD using a pipe.

The postgres test code is using IPC::Run, but currently passing a single
argument.  I think we want to change that to pass *multiple* arguments, so we
can use '>' and/or '|'.  I have a patch which partially works, and another patch
which I didn't try very hard to make work, since the first part already took me
a long time..

You'll want to start with a git checkout and do:
time ./configure --enable-tap-tests ...

And apply David's patches from the above thread.  The goal is to make a test
case that fails without his patch and passes with it.  And maybe apply my
patches if they're useful.

I've been running the pg_dump checks like this:
time make -C src/bin/pg_dump check

On Sun, Jun 21, 2020 at 02:42:25PM -0500, Justin Pryzby wrote:
> On Sun, Jun 21, 2020 at 03:18:58PM -0400, David Gilman wrote:
> > Thank you for taking a stab at the perl thing. I took the question to
> > StackOverflow, I haven't yet looped back to try their suggestion but I
> > think there is hope by messing with the Perl references.
> > https://stackoverflow.com/questions/62086173/using-the-right-perl-array-references-with-ipcrun
> 
> I finally got this to work using IPC:Run's '>' redirection operator, but it
> seems like that opens a file which *is* seekable, so doesn't work for this
> purpose.  Since "cat" isn't portable (duh), the next best thing seems to be to
> pipe to perl -pe '' >ouput.  Otherwise maybe your way of adding an
> --disable-seeking option is best.
> 
> See if you can do anything with the attached.

[fixed patch I sent to David offlist]

[and another patch which doesn't work yet]

-- 
Justin

PS. The patches are named *.txt so that the patch tester doesn't try to test
them, as they're known to be incomplete.

--K3vkeaB0MlFjg8U+
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
 filename="0001-Write-to-stdout-instead-of-disable-seeking.txt"

From 737abbc1115ee1360ac950bcf6abe1677f1eaa79 Mon Sep 17 00:00:00 2001
From: Your Name <you@example.com>
Date: Sat, 13 Jun 2020 15:29:48 -0700
Subject: [PATCH 1/3] Write to stdout instead of --disable-seeking

---
 src/bin/pg_dump/t/002_pg_dump.pl |  4 ++--
 src/test/perl/TestLib.pm         | 10 ++++++++--
 2 files changed, 10 insertions(+), 4 deletions(-)

diff --git a/src/bin/pg_dump/t/002_pg_dump.pl b/src/bin/pg_dump/t/002_pg_dump.pl
index 2423ec5b1d..8c89488a12 100644
--- a/src/bin/pg_dump/t/002_pg_dump.pl
+++ b/src/bin/pg_dump/t/002_pg_dump.pl
@@ -153,8 +153,8 @@ my %pgdump_runs = (
 	defaults_custom_format_no_seek_parallel_restore => {
 		test_key => 'defaults',
 		dump_cmd => [
-			'pg_dump', '-Fc', '-Z6', '--no-sync', '--disable-seeking',
-			"--file=$tempdir/defaults_custom_format_no_seek_parallel_restore.dump", 'postgres',
+			[ 'pg_dump', '-Fc', '-Z6', '--no-sync', 'postgres', ],
+			'>', "$tempdir/defaults_custom_format_no_seek_parallel_restore.dump", # stdout disables seeking
 		],
 		restore_cmd => [
 			'pg_restore', '-Fc', '--jobs=2',
diff --git a/src/test/perl/TestLib.pm b/src/test/perl/TestLib.pm
index d579d5c177..dd7b64e4c4 100644
--- a/src/test/perl/TestLib.pm
+++ b/src/test/perl/TestLib.pm
@@ -338,8 +338,14 @@ The return value from the command is passed through.
 
 sub run_log
 {
-	print("# Running: " . join(" ", @{ $_[0] }) . "\n");
-	return IPC::Run::run(@_);
+	my @x = @{$_[0]};
+	if (ref($x[0]) ne '') {
+		print("# Running: " . join(" ", @{ $x[0] }) . " @x[1,scalar(@x)-1]\n");
+		return IPC::Run::run(@x);
+	} else {
+		print("# Running: " . join(" ", @{ $_[0] }) . "\n");
+		return IPC::Run::run(@_);
+	}
 }
 
 =pod
-- 
2.24.2


--K3vkeaB0MlFjg8U+
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment; filename="0002-fix.txt"

From e855b9212f6ea178a3c5f913cec6e933e371e0b1 Mon Sep 17 00:00:00 2001
From: Your Name <you@example.com>
Date: Mon, 15 Jun 2020 03:28:18 -0700
Subject: [PATCH 2/3] fix

---
 src/bin/pg_dump/t/002_pg_dump.pl | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/src/bin/pg_dump/t/002_pg_dump.pl b/src/bin/pg_dump/t/002_pg_dump.pl
index 8c89488a12..d28988b870 100644
--- a/src/bin/pg_dump/t/002_pg_dump.pl
+++ b/src/bin/pg_dump/t/002_pg_dump.pl
@@ -154,6 +154,8 @@ my %pgdump_runs = (
 		test_key => 'defaults',
 		dump_cmd => [
 			[ 'pg_dump', '-Fc', '-Z6', '--no-sync', 'postgres', ],
+			'|',
+			[ 'perl', '-pe', '', ],
 			'>', "$tempdir/defaults_custom_format_no_seek_parallel_restore.dump", # stdout disables seeking
 		],
 		restore_cmd => [
-- 
2.24.2


--K3vkeaB0MlFjg8U+--