Horton

Thread

Commits

  1. Consolidate cross-option checks in pg_restore

  2. Throw an error when using -C and -1 at the same time in pg_restore.

  1. Conflicting option checking in pg_restore

    Daniel Gustafsson <daniel@yesql.se> — 2018-10-27T21:01:06Z

    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

  2. Re: Conflicting option checking in pg_restore

    Fabien COELHO <coelho@cri.ensmp.fr> — 2018-10-28T06:54:53Z

    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.

  3. Re: Conflicting option checking in pg_restore

    Narayanan V <vnarayanan.email@gmail.com> — 2018-10-28T15:41:01Z

    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.

  4. Re: Conflicting option checking in pg_restore

    Fabien COELHO <coelho@cri.ensmp.fr> — 2018-10-28T18:42:27Z

    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.

  5. Re: Conflicting option checking in pg_restore

    Daniel Gustafsson <daniel@yesql.se> — 2018-10-28T20:52:13Z

    > 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

  6. Re: Conflicting option checking in pg_restore

    Daniel Gustafsson <daniel@yesql.se> — 2018-10-28T21:02:02Z

    > 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

  7. Re: Conflicting option checking in pg_restore

    Michael Paquier <michael@paquier.xyz> — 2018-10-29T00:02:24Z

    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

  8. Re: Conflicting option checking in pg_restore

    Fabien COELHO <coelho@cri.ensmp.fr> — 2018-10-29T06:11:35Z

    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.

  9. Re: Conflicting option checking in pg_restore

    Michael Paquier <michael@paquier.xyz> — 2018-10-30T02:39:31Z

    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