Re: Conflicting option checking in pg_restore

From: Daniel Gustafsson <daniel(at)yesql(dot)se>
To: Fabien COELHO <coelho(at)cri(dot)ensmp(dot)fr>
Cc: PostgreSQL mailing lists <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: Conflicting option checking in pg_restore
Date: 2018-10-28 20:52:13
Message-ID: 7CD41AA7-A128-49AC-8E96-DFB9169C3A96@yesql.se
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

> 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

Attachment Content-Type Size
pg_restore_options-v2.patch application/octet-stream 2.8 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Daniel Gustafsson 2018-10-28 21:02:02 Re: Conflicting option checking in pg_restore
Previous Message Dmitry Dolgov 2018-10-28 20:41:14 Re: Pluggable Storage - Andres's take