> Could you add the patch to the CF app? > > https://commitfest.postgresql.org/20/
Done. >> 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. Thanks for reviewing! > 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”. Excellent point, I’ve added a test in the attached revision. > 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. Perhaps, we don’t really test for all other potential misconfigurations of the options so I can go either way. It’s also a cheap enough operation. Do you think it should be a check like today or an assertion? cheers ./daniel
pg_restore_options-v2.patch
Description: Binary data
