Re: Adding an ignore list to pg_restore, patch take #3

Hi all,

thanks for the feedback. I updated the patch now.

Alvaro Herrera [2006-02-25 13:47 -0300]:
> > I improved the patch now to only ignore TABLE DATA for existing tables
> > if '-X ignore-existing-tables' is specified. I also updated the
> > documentation.
>
> Is this really an appropiate description for the behavior?  What happens
> if the table is not created for some other reason?  Consider for example
> a table using a datatype that couldn't be created.

Right. However, if the table is not present at all, then it makes even
less sense to attempt to restore its data. Therefore I consider this
mainly a documentation issue. I changed the option to
-X no-data-for-failed-tables and described it as

    By default, table data objects are restored even if the associated
    table could not be successfully created (e. g.  because it already
    exists). [...]

Tom Lane [2006-02-25 12:18 -0500]:
> Martin Pitt <mpitt@debian.org> writes:
> > Martin Pitt [2006-02-19 14:39 +0100]:
> >> Since this changes the behaviour of pg_restore, this should probably
> >> become an option, e. g. -D / --ignore-existing-table-data. I'll do
> >> this if you agree to the principle of the current patch.
>
> > I improved the patch now to only ignore TABLE DATA for existing tables
> > if '-X ignore-existing-tables' is specified. I also updated the
> > documentation.
>
> This patch is unbelievably ugly and probably vulnerable to coredumps.
> Please use a cleaner way of disabling the subsequent load than tromping
> all over the TOC datastructure, ie, not this:
>
> > +                                            strcpy (tes->desc, "IGNOREDATA");

It should not segfault, but I agree that this is a bit hackish. The
updated patch completely removes the TABLE DATA node from the linked
list. It does not free its memory, though; I did not find a
free_tocentry() or similar function. However, pg_restore is no daemon,
and without the new option the memory would be allocated, too, so it
does not make much difference. Can anyone give me a hint how to
properly free the struct?

> BTW, I'm pretty sure it fails for tables with same names in different
> schemas, too.

Right, sorry for that. I fixed that, too.

Bruce Momjian [2006-02-28 19:54 -0500]:
> I will clean it up before applying.

Thank you. I hope the updated patch makes that a little bit easier.

> Your patch has been added to the PostgreSQL unapplied patches list at:
>
>     http://momjian.postgresql.org/cgi-bin/pgpatches
>
> It will be applied as soon as one of the PostgreSQL committers reviews
> and approves it.

Great, thanks!

Martin

P.S. I also updated the test script to create two namespaces with
identidal table names.
http://people.debian.org/~mpitt/test-pg_restore-existing.sh
--
Martin Pitt        http://www.piware.de
Ubuntu Developer   http://www.ubuntu.com
Debian Developer   http://www.debian.org

In a world without walls and fences, who needs Windows and Gates?