Re: Bug in pg_dump

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.

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.

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?
(Btw, be sure to generate your patches directly with git next time. :) )

Regards,
--
Michael