Code review for --no-data-for-failed-tables patch. Instead of trashing

one of the program's core data structures, make use of the existing
ability to selectively exclude TOC items by ID.  Slightly more code but
much less likely to create future maintenance problems.

diff --git a/doc/src/sgml/ref/pg_restore.sgml b/doc/src/sgml/ref/pg_restore.sgml
index 4855282..edbbba8 100644 (file)
--- a/doc/src/sgml/ref/pg_restore.sgml
+++ b/doc/src/sgml/ref/pg_restore.sgml
@@ -1,4 +1,4 @@
-<!-- $PostgreSQL: pgsql/doc/src/sgml/ref/pg_restore.sgml,v 1.62 2006/10/07 20:59:04 petere Exp $ -->
+<!-- $PostgreSQL: pgsql/doc/src/sgml/ref/pg_restore.sgml,v 1.63 2006/10/14 23:07:22 tgl Exp $ -->
 
 <refentry id="APP-PGRESTORE">
  <refmeta>
@@ -396,15 +396,23 @@
      </varlistentry>
 
      <varlistentry>
-      <term><option>--no-data-for-failed-tables</></term>
+      <term><option>--no-data-for-failed-tables</option></term>
       <listitem>
        <para>
-       By default, table data objects are restored even if the
-       associated table could not be successfully created (e. g.
-       because it already exists). With this option, such table
-       data is silently ignored. This is useful for dumping and
-       restoring databases with tables which contain auxiliary data
-       for PostgreSQL extensions (e. g. PostGIS).
+        By default, table data is restored even if the creation command
+        for the table failed (e.g., because it already exists).
+        With this option, data for such a table is skipped.
+        This behavior is useful when the target database may already
+        contain the desired table contents.  For example,
+        auxiliary tables for <productname>PostgreSQL</> extensions
+        such as <productname>PostGIS</> may already be loaded in
+        the target database; specifying this option prevents duplicate
+        or obsolete data from being loaded into them.
+       </para>
+
+       <para>
+        This option is effective only when restoring directly into a
+        database, not when producing SQL script output.
        </para>
       </listitem>
      </varlistentry>

diff --git a/src/bin/pg_dump/pg_backup.h b/src/bin/pg_dump/pg_backup.h
index ae4e295..3ada7cc 100644 (file)
--- a/src/bin/pg_dump/pg_backup.h
+++ b/src/bin/pg_dump/pg_backup.h
@@ -15,7 +15,7 @@
  *
  *
  * IDENTIFICATION
- *             $PostgreSQL: pgsql/src/bin/pg_dump/pg_backup.h,v 1.43 2006/10/04 00:30:05 momjian Exp $
+ *             $PostgreSQL: pgsql/src/bin/pg_dump/pg_backup.h,v 1.44 2006/10/14 23:07:22 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -112,15 +112,12 @@ typedef struct _restoreOptions
        int                     noDataForFailedTables;
        int                     requirePassword;
        int                     exit_on_error;
-
-       bool       *idWanted;
-       bool            limitToList;
        int                     compression;
-
        int                     suppressDumpWarnings;   /* Suppress output of WARNING entries
                                                                                 * to stderr */
        bool            single_txn;
 
+       bool       *idWanted;           /* array showing which dump IDs to emit */
 } RestoreOptions;
 
 /*
@@ -176,8 +173,9 @@ extern void PrintTOCSummary(Archive *AH, RestoreOptions *ropt);
 
 extern RestoreOptions *NewRestoreOptions(void);
 
-/* Rearrange TOC entries */
-extern void SortTocFromFile(Archive *AH, RestoreOptions *ropt);
+/* Rearrange and filter TOC entries */
+extern void SortTocFromFile(Archive *AHX, RestoreOptions *ropt);
+extern void InitDummyWantedList(Archive *AHX, RestoreOptions *ropt);
 
 /* Convenience functions used only when writing DATA */
 extern int     archputs(const char *s, Archive *AH);

diff --git a/src/bin/pg_dump/pg_backup_archiver.c b/src/bin/pg_dump/pg_backup_archiver.c
index 32b6490..272ff5f 100644 (file)
--- a/src/bin/pg_dump/pg_backup_archiver.c
+++ b/src/bin/pg_dump/pg_backup_archiver.c
@@ -15,7 +15,7 @@
  *
  *
  * IDENTIFICATION
- *             $PostgreSQL: pgsql/src/bin/pg_dump/pg_backup_archiver.c,v 1.136 2006/10/04 00:30:05 momjian Exp $
+ *             $PostgreSQL: pgsql/src/bin/pg_dump/pg_backup_archiver.c,v 1.137 2006/10/14 23:07:22 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -279,23 +279,27 @@ RestoreArchive(Archive *AHX, RestoreOptions *ropt)
                        defnDumped = true;
 
                        /*
-                        * If we could not create a table, ignore the respective TABLE
-                        * DATA if -X no-data-for-failed-tables is given
+                        * If we could not create a table and --no-data-for-failed-tables
+                        * was given, ignore the corresponding TABLE DATA
                         */
-                       if (ropt->noDataForFailedTables && AH->lastErrorTE == te && strcmp(te->desc, "TABLE") == 0)
+                       if (ropt->noDataForFailedTables &&
+                               AH->lastErrorTE == te &&
+                               strcmp(te->desc, "TABLE") == 0)
                        {
-                               TocEntry   *tes,
-                                                  *last;
+                               TocEntry   *tes;
 
-                               ahlog(AH, 1, "table %s could not be created, will not restore its data\n", te->tag);
+                               ahlog(AH, 1, "table \"%s\" could not be created, will not restore its data\n",
+                                         te->tag);
 
-                               for (last = te, tes = te->next; tes != AH->toc; last = tes, tes = tes->next)
+                               for (tes = te->next; tes != AH->toc; tes = tes->next)
                                {
-                                       if (strcmp(tes->desc, "TABLE DATA") == 0 && strcmp(tes->tag, te->tag) == 0 &&
-                                               strcmp(tes->namespace ? tes->namespace : "", te->namespace ? te->namespace : "") == 0)
+                                       if (strcmp(tes->desc, "TABLE DATA") == 0 &&
+                                               strcmp(tes->tag, te->tag) == 0 &&
+                                               strcmp(tes->namespace ? tes->namespace : "",
+                                                          te->namespace ? te->namespace : "") == 0)
                                        {
-                                               /* remove this node */
-                                               last->next = tes->next;
+                                               /* mark it unwanted */
+                                               ropt->idWanted[tes->dumpId - 1] = false;
                                                break;
                                        }
                                }
@@ -789,7 +793,6 @@ SortTocFromFile(Archive *AHX, RestoreOptions *ropt)
        /* Allocate space for the 'wanted' array, and init it */
        ropt->idWanted = (bool *) malloc(sizeof(bool) * AH->maxDumpId);
        memset(ropt->idWanted, 0, sizeof(bool) * AH->maxDumpId);
-       ropt->limitToList = true;
 
        /* Set prev entry as head of list */
        tePrev = AH->toc;
@@ -837,6 +840,19 @@ SortTocFromFile(Archive *AHX, RestoreOptions *ropt)
                                         strerror(errno));
 }
 
+/*
+ * Set up a dummy ID filter that selects all dump IDs
+ */
+void
+InitDummyWantedList(Archive *AHX, RestoreOptions *ropt)
+{
+       ArchiveHandle *AH = (ArchiveHandle *) AHX;
+
+       /* Allocate space for the 'wanted' array, and init it to 1's */
+       ropt->idWanted = (bool *) malloc(sizeof(bool) * AH->maxDumpId);
+       memset(ropt->idWanted, 1, sizeof(bool) * AH->maxDumpId);
+}
+
 /**********************
  * 'Convenience functions that look like standard IO functions
  * for writing data when in dump mode.
@@ -2066,8 +2082,8 @@ _tocEntryRequired(TocEntry *te, RestoreOptions *ropt, bool include_acls)
        if (!te->defn || strlen(te->defn) == 0)
                res = res & ~REQ_SCHEMA;
 
-       /* Finally, if we used a list, limit based on that as well */
-       if (ropt->limitToList && !ropt->idWanted[te->dumpId - 1])
+       /* Finally, if there's a per-ID filter, limit based on that as well */
+       if (ropt->idWanted && !ropt->idWanted[te->dumpId - 1])
                return 0;
 
        return res;

diff --git a/src/bin/pg_dump/pg_restore.c b/src/bin/pg_dump/pg_restore.c
index 0c17c64..bd5869a 100644 (file)
--- a/src/bin/pg_dump/pg_restore.c
+++ b/src/bin/pg_dump/pg_restore.c
@@ -34,7 +34,7 @@
  *
  *
  * IDENTIFICATION
- *             $PostgreSQL: pgsql/src/bin/pg_dump/pg_restore.c,v 1.83 2006/10/07 20:59:05 petere Exp $
+ *             $PostgreSQL: pgsql/src/bin/pg_dump/pg_restore.c,v 1.84 2006/10/14 23:07:22 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -338,6 +338,14 @@ main(int argc, char **argv)
 
        if (opts->tocFile)
                SortTocFromFile(AH, opts);
+       else if (opts->noDataForFailedTables)
+       {
+               /*
+                * we implement this option by clearing idWanted entries, so must
+                * create a dummy idWanted array if there wasn't a tocFile
+                */
+               InitDummyWantedList(AH, opts);
+       }
 
        if (opts->tocSummary)
                PrintTOCSummary(AH, opts);