Re: Conflicting option checking in pg_restore - Mailing list pgsql-hackers

From Fabien COELHO
Subject Re: Conflicting option checking in pg_restore
Date
Msg-id alpine.DEB.2.21.1810280735310.3739@lancre
Whole thread Raw
In response to Conflicting option checking in pg_restore  (Daniel Gustafsson <daniel@yesql.se>)
Responses Re: Conflicting option checking in pg_restore
Re: Conflicting option checking in pg_restore
List pgsql-hackers
Hallå Daniel,

> Checking for conflicting options in pg_restore was mostly done in main() with
> one check deferred until RestoreArchive().  Reading the git history makes it
> seem like it simply happened, without the disjoint checking being intentional.
> Am I reading it right that we can consolidate all the option checking to
> main()?  The attached patch does that, and also rewords the error message to
> make it similar to the other option checks.

Patch applies cleanly, compiles, both global and local "make check" ok.

As there are no test changes, this is not tested. I'd suggest to add it to 
"src/bin/pg_dump/t/001_basic.pl".

There is a possible catch:

Function RestoreArchive is called both from pg_dump & pg_restore, so now 
the sanity check is not performed for the former (which does not have the 
-1 option, though). Moreover, the function is noted "Public", which may 
suggest that external tools could take advantage of it, and if so it 
suggests that maybe it is not wise to remove the test. Any opinion around?

Maybe the check could be kept in RestoreArchive with a comment to say it 
is redundant, but the check could be performed in pg_restore as well for 
the sake of having an better and more homogeneous error message.

-- 
Fabien.

pgsql-hackers by date:

Previous
From: Amit Kapila
Date:
Subject: Re: [RFC] Removing "magic" oids
Next
From: Dilip Kumar
Date:
Subject: Re: CVE-2017-7484-induced bugs, or, btree cmp functions are not leakproof?