Re: Conflicting option checking in pg_restore

Hi
On Sun, Oct 28, 2018 at 12:25 PM Fabien COELHO <coelho@cri.ensmp.fr> wrote:

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

The RestoreArchive Method extracts the RestoreOptions from the Archive
Handle.
Please see the relevant code below,

,----
| void
| RestoreArchive(Archive *AHX)
| {
| ArchiveHandle *AH = (ArchiveHandle *) AHX;
| RestoreOptions *ropt = AH->public.ropt;
`----

Wouldn't ropt->single_txn be undefined when called from pg_dump ?

Unless I missed something here, I think it is logical to just move the
relevant code to
pg_restore main.

- Tested the patch on a machine running -Ubuntu 18.04.1 LTS
- Patch applied cleanly.
- The source built post application of the patch.
- make check passed.
,----
| =======================
|  All 187 tests passed.
| =======================
`----

+1 from me.

Thank you,
Narayanan



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