Date: Wed, 11 Jan 2017 09:36:43 -0500
From: Stephen Frost <sfrost@snowman.net>
To: Ashutosh Bapat <ashutosh.bapat@enterprisedb.com>
Cc: pgsql-hackers <pgsql-hackers@postgresql.org>
Subject: Re: pg_restore accepts -j -1
Message-ID: <20170111143643.GK18360@tamriel.snowman.net>
References: <20170110044815.GC18360@tamriel.snowman.net>
 <CAFjFpRdSP_N8ZL=sXGvmZR1hF=PuieLyK3Xsvh=jr1v2Kf-70w@mail.gmail.com>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="xncXys66qXuJqdVV"
Content-Disposition: inline
In-Reply-To: 
 <CAFjFpRdSP_N8ZL=sXGvmZR1hF=PuieLyK3Xsvh=jr1v2Kf-70w@mail.gmail.com>
User-Agent: Mutt/1.5.21 (2010-09-15)
Precedence: bulk
Sender: pgsql-hackers-owner@postgresql.org


--xncXys66qXuJqdVV
Content-Type: multipart/mixed; boundary="bk1rFQtr5T/2h+/m"
Content-Disposition: inline


--bk1rFQtr5T/2h+/m
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

Ashutosh,

* Ashutosh Bapat (ashutosh.bapat@enterprisedb.com) wrote:
> On Tue, Jan 10, 2017 at 10:18 AM, Stephen Frost <sfrost@snowman.net> wrot=
e:
> > For reasons which seem likely to be entirely unintentional, pg_restore
> > will accept a '-1' for -j:
> >
> > pg_restore -j -1
> >
> > This seems to result in the parallel state being NULL and so things
> > don't outright break, but it hardly seems likely to be what the user was
> > asking for- my guess is that they actually wanted "parallel, single
> > transaction", which we don't actually support:
> >
> > -> pg_restore -j 2 -1
> > pg_restore: cannot specify both --single-transaction and multiple jobs
> >
> > We also don't accept -1 for pg_dump:
> >
> > -> pg_dump -j -1
> > pg_dump: invalid number of parallel jobs
> >
> > If I'm missing something, please let me know, otherwise I'll plan to put
> > the same check into pg_restore which exists in pg_dump.
>=20
> Both the code blocks were added by 9e257a18, but I don't see any
> description of why they are different in pg_dump.c and pg_restore.c.

Right.

> In fact per comments in pg_restore.c, that condition should be same as
> pg_dump.c. I am not sure whether it's just for windows specific
> condition or the whole block. But I don't see any reason not to
> replicate the same conditions in pg_restore.c

I'm pretty sure that comment is about the Windows-specific check of
MAXIMUM_WAIT_OBJECTS, but I don't think there's any reason to accept a
zero or negative value for numWorkers.

Attached patch adds the same check to pg_restore that's in pg_dump
already.  Looks like it should back-patch to 9.3 pretty cleanly and I'll
add a similar check for 9.2.

Any thoughts about adding the Windows-specific MAXIMUM_WAIT_OBJECTS
check to 9.2 pg_restore..?  It certainly looks entirely straight-forward
to do, and though I don't recall hearing anyone complaining about trying
to run pg_restore on Windows with lots of jobs and having it fall over,
it might avoid a bit of frustration for anyone who does try.

Thanks!

Stephen

--bk1rFQtr5T/2h+/m
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment; filename="check_numworkers_pg_restore.patch"
Content-Transfer-Encoding: quoted-printable

diff --git a/src/bin/pg_dump/pg_restore.c b/src/bin/pg_dump/pg_restore.c
new file mode 100644
index b21fd26..b69082d
*** a/src/bin/pg_dump/pg_restore.c
--- b/src/bin/pg_dump/pg_restore.c
*************** main(int argc, char **argv)
*** 402,407 ****
--- 402,413 ----
  	if (opts->tocFile)
  		SortTocFromFile(AH);
 =20
+ 	if (numWorkers <=3D 0)
+ 	{
+ 		fprintf(stderr, _("%s: invalid number of parallel jobs\n"), progname);
+ 		exit(1);
+ 	}
+=20
  	/* See comments in pg_dump.c */
  #ifdef WIN32
  	if (numWorkers > MAXIMUM_WAIT_OBJECTS)

--bk1rFQtr5T/2h+/m--

--xncXys66qXuJqdVV
Content-Type: application/pgp-signature; name="signature.asc"
Content-Description: Digital signature

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1

iQIcBAEBCgAGBQJYdkL7AAoJEO1sijiDR2RVj30QAK/mTlgAZQKM8aF7/hdEKIfo
IXPq1hdFStslicabAL8R3jSL2uz8GxMA00fEWoK0X9z3T00OE9g/afgAE00CKMEz
zY1pdWOJKCoAphZ/En+vhSMafAvdnaYukyQVjv+1DUOrpZKiubZ7bA+OMSOifdgm
1ogeZcjO+DqEKDXGAOJvpRiGXEnQKNFbgmVRJzj0BNmcz78CN+kyLl5tLRTFiGOl
GFWp37EH0cGMPr8uLLiDUBdlRWst8RfGAPgVKUonxS6rDUGYbquIzxLZTLVkD62r
IHFJ8hqZV4YYDk2EbWAPWChdrM9gRW5H2xBMzuBvK7v79YLxjFRZrG3kN0ab184n
VY3/85GcjdziJSiIwSIH2TEu88LswaaS0ETpldBrQ8EN9oI+TS+FJC6kRUaGq3xH
YhpT74wr06qKwCC4rvsBC+bVLdjx6U+SL8EY3OuhAECWNJ1sf+tx0y6Fw+2a03Cd
Ww4GxIaVhmK4L2k8gp0hjPecB5qAt5bupZMfmJnXo3WbPKhpcQf+izcz8Is+RC8O
B53hVk2lYng/kzEoU1iJNH5PyjL2vDooFWb9ricBjntTFoS+52CoFqBhnzMK9lnw
5bCQNF04rzXdqN6Y0vaOlljbp++PThbAu5w2wkE7xAi7ysnCjKMb9RJ0os/owuQt
LNkNjKqyHFUwAGhb70mx
=p9k6
-----END PGP SIGNATURE-----

--xncXys66qXuJqdVV--