On Wed, 21 Jul 2021 at 23:50, Michael Paquier <michael@paquier.xyz> wrote:
> Hacking on that, I am finishing with the attached. It is less
> ambitious, still very useful as it removes a dozen of custom error
> messages in favor of the two ones introduced in option_utils.c. On
> top of that this reduces a bit the code:
> 15 files changed, 156 insertions(+), 169 deletions(-)
>
> What do you think?
This is just a driveby review, but I think that it's good to see no
increase in the number of lines of code to make these improvements.
It's also good to see the added consistency introduced by this patch.
I didn't test the patch, so this is just from reading through.
I wondered about the TAP tests here:
command_fails_like(
[ 'pg_dump', '-j', '-1' ],
qr/\Qpg_dump: error: -j\/--jobs must be in range 0..2147483647\E/,
'pg_dump: invalid number of parallel jobs');
command_fails_like(
[ 'pg_restore', '-j', '-1', '-f -' ],
qr/\Qpg_restore: error: -j\/--jobs must be in range 0..2147483647\E/,
'pg_restore: invalid number of parallel jobs');
I see both of these are limited to 64 on windows. Won't those fail on Windows?
I also wondered if it would be worth doing #define MAX_JOBS somewhere
away from the option parsing code. This part is pretty ugly:
/*
* On Windows we can only have at most MAXIMUM_WAIT_OBJECTS
* (= 64 usually) parallel jobs because that's the maximum
* limit for the WaitForMultipleObjects() call.
*/
if (!option_parse_int(optarg, "-j/--jobs", 0,
#ifdef WIN32
MAXIMUM_WAIT_OBJECTS,
#else
INT_MAX,
#endif
&numWorkers))
exit(1);
break;
David
