Re: Bug in pg_dump - Mailing list pgsql-hackers

Le 17/02/2015 14:44, Michael Paquier a écrit :
> On Tue, Jan 20, 2015 at 8:06 PM, Gilles Darold <gilles.darold@dalibo.com> wrote:
>> Le 19/01/2015 14:41, Robert Haas a écrit :
>>> On Thu, Jan 15, 2015 at 6:26 AM, Gilles Darold <gilles.darold@dalibo.com> wrote:
>>>> I attach a patch that solves the issue in pg_dump, let me know if it might
>>>> be included in Commit Fest or if the three other solutions are a better
>>>> choice.
>>> I think a fix in pg_dump is appropriate, so I'd encourage you to add
>>> this to the next CommitFest.
>>>
>> Ok, thanks Robert. The patch have been added to next CommitFest under
>> the Bug Fixes section.
>>
>> I've also made some review of the patch and more test. I've rewritten
>> some comments and added a test when TableInfo is NULL to avoid potential
>> pg_dump crash.
>>
>> New patch attached: pg_dump.c-extension-FK-v2.patch
> So, I looked at your patch and I have some comments...
>
> The approach taken by the patch looks correct to me as we cannot
> create FK constraints after loading the data in the case of an
> extension, something that can be done with a data-only dump.
>
> Now, I think that the routine hasExtensionMember may impact
> performance unnecessarily in the case of databases with many tables,
> say thousands or more. And we can easily avoid this performance
> problem by checking the content of each object dumped by doing the
> legwork directly in getTableData. Also, most of the NULL pointer
> checks are not needed as most of those code paths would crash if
> tbinfo is NULL, and actually only keeping the one in dumpConstraint is
> fine.

Yes that's exactly what we discuss at Moscow, thanks for removing the
hasExtensionMember() routine. About NULL pointer I'm was not sure that
it could not crash on some other parts so I decided to check it
everywhere. If that's ok to just check in dumpConstraint() that's fine.
> On top of those things, I have written a small extension able to
> reproduce the problem that I have included as a test module in
> src/test/modules. The dump/restore check is done using the TAP tests,
> and I have hacked prove_check to install as well the contents of the
> current folder to be able to use the TAP routines with an extension
> easily. IMO, as we have no coverage of pg_dump with extensions I think
> that it would be useful to ensure that this does not break again in
> the future.

Great ! I did not had time to do that, thank you so much.

> All the hacking I have done during the review results in the set of
> patch attached:
> - 0001 is your patch, Gilles, with some comment fixes as well as the
> performance issue with hasExtensionMember fix
> - 0002 is the modification of prove_check that makes TAP tests usable
> with extensions
> - 0003 is the test module covering the tests needed for pg_dump, at
> least for the problem of this thread.
>
> Gilles, how does that look to you?

Looks great to me, I have tested with the postgis_topology extension
everything works fine.

> (Btw, be sure to generate your patches directly with git next time. :) )

Sorry, some old reminiscence :-)

Thanks for the review.
Best regards,

-- 
Gilles Darold
Consultant PostgreSQL
http://dalibo.com - http://dalibo.org