Re: [HACKERS] pg_restore accepts -j -1

[Stephen Frost]

Ashutosh,

* Ashutosh Bapat (ashutosh.ba...@enterprisedb.com) wrote:
> On Tue, Jan 10, 2017 at 10:18 AM, Stephen Frost  wrote:
> > 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.
> 
> 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.
> 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

Ok, I've pushed the change.

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] pg_restore accepts -j -1

[Stephen Frost]

Ashutosh,

* Stephen Frost (sfr...@snowman.net) wrote:
> 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.

After playing with this, it seems entirely wrong to wait until after we
try to open the archive before we finish checking the validity of the
options, so I moved these checks up to a more sensible location.

> 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.

The more I think about it, the more it seems like this is something we
should have back-patched when we added that check, so I'll go ahead and
add that check to 9.2 also.

Updated patch attached.

I'll plan to push these changes later on this afternoon.

Thanks!

Stephen
diff --git a/src/bin/pg_dump/pg_restore.c b/src/bin/pg_dump/pg_restore.c
new file mode 100644
index b21fd26..239b0d8
*** a/src/bin/pg_dump/pg_restore.c
--- b/src/bin/pg_dump/pg_restore.c
*** main(int argc, char **argv)
*** 330,335 
--- 330,351 
  		exit_nicely(1);
  	}
  
+ 	if (numWorkers <= 0)
+ 	{
+ 		fprintf(stderr, _("%s: invalid number of parallel jobs\n"), progname);
+ 		exit(1);
+ 	}
+ 
+ 	/* See comments in pg_dump.c */
+ #ifdef WIN32
+ 	if (numWorkers > MAXIMUM_WAIT_OBJECTS)
+ 	{
+ 		fprintf(stderr, _("%s: maximum number of parallel jobs is %d\n"),
+ progname, MAXIMUM_WAIT_OBJECTS);
+ 		exit(1);
+ 	}
+ #endif
+ 
  	/* Can't do single-txn mode with multiple connections */
  	if (opts->single_txn && numWorkers > 1)
  	{
*** main(int argc, char **argv)
*** 402,417 
  	if (opts->tocFile)
  		SortTocFromFile(AH);
  
- 	/* See comments in pg_dump.c */
- #ifdef WIN32
- 	if (numWorkers > MAXIMUM_WAIT_OBJECTS)
- 	{
- 		fprintf(stderr, _("%s: maximum number of parallel jobs is %d\n"),
- progname, MAXIMUM_WAIT_OBJECTS);
- 		exit(1);
- 	}
- #endif
- 
  	AH->numWorkers = numWorkers;
  
  	if (opts->tocSummary)
--- 418,423 


signature.asc
Description: Digital signature

Re: [HACKERS] pg_restore accepts -j -1

[Stephen Frost]

Ashutosh,

* Ashutosh Bapat (ashutosh.ba...@enterprisedb.com) wrote:
> On Tue, Jan 10, 2017 at 10:18 AM, Stephen Frost  wrote:
> > 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.
> 
> 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
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);
  
+ 	if (numWorkers <= 0)
+ 	{
+ 		fprintf(stderr, _("%s: invalid number of parallel jobs\n"), progname);
+ 		exit(1);
+ 	}
+ 
  	/* See comments in pg_dump.c */
  #ifdef WIN32
  	if (numWorkers > MAXIMUM_WAIT_OBJECTS)


signature.asc
Description: Digital signature

Re: [HACKERS] pg_restore accepts -j -1

[Ashutosh Bapat]

On Tue, Jan 10, 2017 at 10:18 AM, Stephen Frost  wrote:
> Greetings,
>
> 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.

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.
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

-- 
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

[HACKERS] pg_restore accepts -j -1

[Stephen Frost]

Greetings,

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.

Thanks!

Stephen


signature.asc
Description: Digital signature