Thread: Conflicting option checking in pg_restore

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.

cheers ./daniel

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.

Hello Narayanan,

>> 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?
>
> [...]
>
> Wouldn't ropt->single_txn be undefined when called from pg_dump ?

Yes, probably.

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

My point is that given the "Public" comment and that some care is taken to 
put everything in a special struct, I was wondering whether external tools 
may use this function, in which case the check would be left out.

-- 
Fabien.

> 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
externaltools could take advantage of it, and if so it suggests that maybe it is not wise to remove the test. Any
opinionaround? 
>
> Maybe the check could be kept in RestoreArchive with a comment to say it is redundant, but the check could be
performedin 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

> On 28 Oct 2018, at 19:42, Fabien COELHO <coelho@cri.ensmp.fr> wrote:

>>> 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?
>>
>> [...]
>>
>> Wouldn't ropt->single_txn be undefined when called from pg_dump ?
>
> Yes, probably.

pg_dump creates the RestoreOptions struct with NewRestoreOptions() which
allocates it with pg_malloc0(), making single_txn false.

cheers ./daniel

On Sun, Oct 28, 2018 at 10:02:02PM +0100, Daniel Gustafsson wrote:
>> On 28 Oct 2018, at 19:42, Fabien COELHO <coelho@cri.ensmp.fr> wrote:
>>>> 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?

I would still need to see such a tool, and they would need most of the
stuff that pg_dump has already to get the object data.  As far as I know
it is marked as public because plain-text mode of pg_dump uses it.

>>> Wouldn't ropt->single_txn be undefined when called from pg_dump ?
>>
>> Yes, probably.
>
> pg_dump creates the RestoreOptions struct with NewRestoreOptions() which
> allocates it with pg_malloc0(), making single_txn false.

Which is why it would not be a problem for pg_dump.  The cross-option
check you are moving has been added as of 3a819b07, still you are right
that such checks should happen at the beginning so the suggestion of
this thread seems right to me.

Any objections from others?
--
Michael

Hallå Daniel,

> an assertion?

v2 applies, compiles, both local & global make check are ok.

Its CF category could have been "bug fix" or "restructuring".

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

Michaël suggests that there is no issue of external tool using the 
internal function, so I'm fine with this version.

I have switched the patch to ready for committer.

-- 
Fabien.

On Mon, Oct 29, 2018 at 07:11:35AM +0100, Fabien COELHO wrote:
> Michaël suggests that there is no issue of external tool using the internal
> function, so I'm fine with this version.
>
> I have switched the patch to ready for committer.

One catch with this refactoring is that for example this combination
does not result in an error on HEAD, but it does with the patch:
pg_restore -l -C -1

Anyway, the fact that we save the caller from one exit_horribly()
knowing that opening the archive is completely useless makes the move
worth it in my opinion.  RestoreArchive should complain about things
which depend on the opened archive, which is the only thing it does
now.  I would not risk back-patching it though.

At the same time, I have checked the set of TAP tests for pg_restore and
the cross-option checks are all covered, so no need to go crazy on this
side.

For the archive's sake, the original commit 3a819b07 which did the
option check in RestoreArchive comes from here:
https://www.postgresql.org/message-id/496B6B40.1010909@hagander.net

And committed, thanks Daniel and Fabien!
--
Michael