Clean up some aspects of pg_dump/pg_restore item-selection logic.

Ensure that CREATE DATABASE and related commands are issued when, and
only when, --create is specified.  Previously there were scenarios
where using selective-dump switches would prevent --create from having
any effect.  For example, it would fail to do anything in pg_restore
if the archive file had been made by a selective dump, because there
would be no TOC entry for the database.

Since we don't issue \connect either if we don't issue CREATE DATABASE,
this could result in unexpectedly restoring objects into the wrong
database.

Also fix pg_restore's selective restore logic so that when an object is
selected to be restored, we also restore its ACL, comment, and security
label if any.  Previously there was no way to get the latter properties
except through tedious mucking about with a -L file.  If, for some
reason, you don't want these properties, you can match the old behavior
by adding --no-acl etc.

While at it, try to make _tocEntryRequired() a little better organized
and better documented.

Discussion: https://postgr.es/m/32668.1516848577@sss.pgh.pa.us

diff --git a/doc/src/sgml/ref/pg_restore.sgml b/doc/src/sgml/ref/pg_restore.sgml
index a2ebf75ebb50bbc944820bf3db062b413eddc688..ee756159f60cab8c7a73b53c6c0f83b997e469f0 100644 (file)
--- a/doc/src/sgml/ref/pg_restore.sgml
+++ b/doc/src/sgml/ref/pg_restore.sgml
@@ -446,6 +446,11 @@
          flag of <application>pg_dump</application>.  There is not currently
          any provision for wild-card matching in <application>pg_restore</application>,
          nor can you include a schema name within its <option>-t</option>.
+         And, while <application>pg_dump</application>'s <option>-t</option>
+         flag will also dump subsidiary objects (such as indexes) of the
+         selected table(s),
+         <application>pg_restore</application>'s <option>-t</option>
+         flag does not include such subsidiary objects.
         </para>
        </note>
 
@@ -564,7 +569,7 @@
       <listitem>
        <para>
         Use conditional commands (i.e. add an <literal>IF EXISTS</literal>
-        clause) when cleaning database objects.  This option is not valid
+        clause) to drop database objects.  This option is not valid
         unless <option>--clean</option> is also specified.
        </para>
       </listitem>

diff --git a/src/bin/pg_dump/pg_backup_archiver.c b/src/bin/pg_dump/pg_backup_archiver.c
index fb0379377bf106d0b4173b78bbcf0112e0d1a9b8..94c511c936be4e0dc46749290581bd004e4dbdf0 100644 (file)
--- a/src/bin/pg_dump/pg_backup_archiver.c
+++ b/src/bin/pg_dump/pg_backup_archiver.c
@@ -70,7 +70,7 @@ static void _selectOutputSchema(ArchiveHandle *AH, const char *schemaName);
 static void _selectTablespace(ArchiveHandle *AH, const char *tablespace);
 static void processEncodingEntry(ArchiveHandle *AH, TocEntry *te);
 static void processStdStringsEntry(ArchiveHandle *AH, TocEntry *te);
-static teReqs _tocEntryRequired(TocEntry *te, teSection curSection, RestoreOptions *ropt);
+static teReqs _tocEntryRequired(TocEntry *te, teSection curSection, ArchiveHandle *AH);
 static RestorePass _tocEntryRestorePass(TocEntry *te);
 static bool _tocEntryIsACL(TocEntry *te);
 static void _disableTriggersIfNecessary(ArchiveHandle *AH, TocEntry *te);
@@ -312,7 +312,7 @@ ProcessArchiveRestoreOptions(Archive *AHX)
        if (te->section != SECTION_NONE)
            curSection = te->section;
 
-       te->reqs = _tocEntryRequired(te, curSection, ropt);
+       te->reqs = _tocEntryRequired(te, curSection, AH);
    }
 
    /* Enforce strict names checking */
@@ -488,9 +488,8 @@ RestoreArchive(Archive *AHX)
             * In createDB mode, issue a DROP *only* for the database as a
             * whole.  Issuing drops against anything else would be wrong,
             * because at this point we're connected to the wrong database.
-            * Conversely, if we're not in createDB mode, we'd better not
-            * issue a DROP against the database at all.  (The DATABASE
-            * PROPERTIES entry, if any, works like the DATABASE entry.)
+            * (The DATABASE PROPERTIES entry, if any, should be treated like
+            * the DATABASE entry.)
             */
            if (ropt->createDB)
            {
@@ -498,12 +497,6 @@ RestoreArchive(Archive *AHX)
                    strcmp(te->desc, "DATABASE PROPERTIES") != 0)
                    continue;
            }
-           else
-           {
-               if (strcmp(te->desc, "DATABASE") == 0 ||
-                   strcmp(te->desc, "DATABASE PROPERTIES") == 0)
-                   continue;
-           }
 
            /* Otherwise, drop anything that's selected and has a dropStmt */
            if (((te->reqs & (REQ_SCHEMA | REQ_DATA)) != 0) && te->dropStmt)
@@ -752,25 +745,6 @@ restore_toc_entry(ArchiveHandle *AH, TocEntry *te, bool is_parallel)
 
    AH->currentTE = te;
 
-   /* Work out what, if anything, we want from this entry */
-   reqs = te->reqs;
-
-   /*
-    * Ignore DATABASE and related entries unless createDB is specified.  We
-    * must check this here, not in _tocEntryRequired, because !createDB
-    * should not prevent emitting these entries to an archive file.
-    */
-   if (!ropt->createDB &&
-       (strcmp(te->desc, "DATABASE") == 0 ||
-        strcmp(te->desc, "DATABASE PROPERTIES") == 0 ||
-        (strcmp(te->desc, "ACL") == 0 &&
-         strncmp(te->tag, "DATABASE ", 9) == 0) ||
-        (strcmp(te->desc, "COMMENT") == 0 &&
-         strncmp(te->tag, "DATABASE ", 9) == 0) ||
-        (strcmp(te->desc, "SECURITY LABEL") == 0 &&
-         strncmp(te->tag, "DATABASE ", 9) == 0)))
-       reqs = 0;
-
    /* Dump any relevant dump warnings to stderr */
    if (!ropt->suppressDumpWarnings && strcmp(te->desc, "WARNING") == 0)
    {
@@ -780,6 +754,9 @@ restore_toc_entry(ArchiveHandle *AH, TocEntry *te, bool is_parallel)
            write_msg(modulename, "warning from original dump file: %s\n", te->copyStmt);
    }
 
+   /* Work out what, if anything, we want from this entry */
+   reqs = te->reqs;
+
    defnDumped = false;
 
    /*
@@ -1191,7 +1168,7 @@ PrintTOCSummary(Archive *AHX)
        if (te->section != SECTION_NONE)
            curSection = te->section;
        if (ropt->verbose ||
-           (_tocEntryRequired(te, curSection, ropt) & (REQ_SCHEMA | REQ_DATA)) != 0)
+           (_tocEntryRequired(te, curSection, AH) & (REQ_SCHEMA | REQ_DATA)) != 0)
        {
            char       *sanitized_name;
            char       *sanitized_schema;
@@ -2824,16 +2801,42 @@ StrictNamesCheck(RestoreOptions *ropt)
    }
 }
 
+/*
+ * Determine whether we want to restore this TOC entry.
+ *
+ * Returns 0 if entry should be skipped, or some combination of the
+ * REQ_SCHEMA and REQ_DATA bits if we want to restore schema and/or data
+ * portions of this TOC entry, or REQ_SPECIAL if it's a special entry.
+ */
 static teReqs
-_tocEntryRequired(TocEntry *te, teSection curSection, RestoreOptions *ropt)
+_tocEntryRequired(TocEntry *te, teSection curSection, ArchiveHandle *AH)
 {
    teReqs      res = REQ_SCHEMA | REQ_DATA;
+   RestoreOptions *ropt = AH->public.ropt;
 
    /* ENCODING and STDSTRINGS items are treated specially */
    if (strcmp(te->desc, "ENCODING") == 0 ||
        strcmp(te->desc, "STDSTRINGS") == 0)
        return REQ_SPECIAL;
 
+   /*
+    * DATABASE and DATABASE PROPERTIES also have a special rule: they are
+    * restored in createDB mode, and not restored otherwise, independently of
+    * all else.
+    */
+   if (strcmp(te->desc, "DATABASE") == 0 ||
+       strcmp(te->desc, "DATABASE PROPERTIES") == 0)
+   {
+       if (ropt->createDB)
+           return REQ_SCHEMA;
+       else
+           return 0;
+   }
+
+   /*
+    * Process exclusions that affect certain classes of TOC entries.
+    */
+
    /* If it's an ACL, maybe ignore it */
    if (ropt->aclsSkip && _tocEntryIsACL(te))
        return 0;
@@ -2842,11 +2845,11 @@ _tocEntryRequired(TocEntry *te, teSection curSection, RestoreOptions *ropt)
    if (ropt->no_publications && strcmp(te->desc, "PUBLICATION") == 0)
        return 0;
 
-   /* If it's security labels, maybe ignore it */
+   /* If it's a security label, maybe ignore it */
    if (ropt->no_security_labels && strcmp(te->desc, "SECURITY LABEL") == 0)
        return 0;
 
-   /* If it's a subcription, maybe ignore it */
+   /* If it's a subscription, maybe ignore it */
    if (ropt->no_subscriptions && strcmp(te->desc, "SUBSCRIPTION") == 0)
        return 0;
 
@@ -2870,65 +2873,118 @@ _tocEntryRequired(TocEntry *te, teSection curSection, RestoreOptions *ropt)
            return 0;
    }
 
-   /* Check options for selective dump/restore */
-   if (ropt->schemaNames.head != NULL)
-   {
-       /* If no namespace is specified, it means all. */
-       if (!te->namespace)
-           return 0;
-       if (!(simple_string_list_member(&ropt->schemaNames, te->namespace)))
-           return 0;
-   }
-
-   if (ropt->schemaExcludeNames.head != NULL &&
-       te->namespace &&
-       simple_string_list_member(&ropt->schemaExcludeNames, te->namespace))
+   /* Ignore it if rejected by idWanted[] (cf. SortTocFromFile) */
+   if (ropt->idWanted && !ropt->idWanted[te->dumpId - 1])
        return 0;
 
-   if (ropt->selTypes)
+   /*
+    * Check options for selective dump/restore.
+    */
+   if (strcmp(te->desc, "ACL") == 0 ||
+       strcmp(te->desc, "COMMENT") == 0 ||
+       strcmp(te->desc, "SECURITY LABEL") == 0)
    {
-       if (strcmp(te->desc, "TABLE") == 0 ||
-           strcmp(te->desc, "TABLE DATA") == 0 ||
-           strcmp(te->desc, "VIEW") == 0 ||
-           strcmp(te->desc, "FOREIGN TABLE") == 0 ||
-           strcmp(te->desc, "MATERIALIZED VIEW") == 0 ||
-           strcmp(te->desc, "MATERIALIZED VIEW DATA") == 0 ||
-           strcmp(te->desc, "SEQUENCE") == 0 ||
-           strcmp(te->desc, "SEQUENCE SET") == 0)
+       /* Database properties react to createDB, not selectivity options. */
+       if (strncmp(te->tag, "DATABASE ", 9) == 0)
        {
-           if (!ropt->selTable)
-               return 0;
-           if (ropt->tableNames.head != NULL && (!(simple_string_list_member(&ropt->tableNames, te->tag))))
+           if (!ropt->createDB)
                return 0;
        }
-       else if (strcmp(te->desc, "INDEX") == 0)
+       else if (ropt->schemaNames.head != NULL ||
+                ropt->schemaExcludeNames.head != NULL ||
+                ropt->selTypes)
        {
-           if (!ropt->selIndex)
-               return 0;
-           if (ropt->indexNames.head != NULL && (!(simple_string_list_member(&ropt->indexNames, te->tag))))
+           /*
+            * In a selective dump/restore, we want to restore these dependent
+            * TOC entry types only if their parent object is being restored.
+            * Without selectivity options, we let through everything in the
+            * archive.  Note there may be such entries with no parent, eg
+            * non-default ACLs for built-in objects.
+            *
+            * This code depends on the parent having been marked already,
+            * which should be the case; if it isn't, perhaps due to
+            * SortTocFromFile rearrangement, skipping the dependent entry
+            * seems prudent anyway.
+            *
+            * Ideally we'd handle, eg, table CHECK constraints this way too.
+            * But it's hard to tell which of their dependencies is the one to
+            * consult.
+            */
+           if (te->nDeps != 1 ||
+               TocIDRequired(AH, te->dependencies[0]) == 0)
                return 0;
        }
-       else if (strcmp(te->desc, "FUNCTION") == 0 ||
-                strcmp(te->desc, "PROCEDURE") == 0)
+   }
+   else
+   {
+       /* Apply selective-restore rules for standalone TOC entries. */
+       if (ropt->schemaNames.head != NULL)
        {
-           if (!ropt->selFunction)
+           /* If no namespace is specified, it means all. */
+           if (!te->namespace)
                return 0;
-           if (ropt->functionNames.head != NULL && (!(simple_string_list_member(&ropt->functionNames, te->tag))))
+           if (!simple_string_list_member(&ropt->schemaNames, te->namespace))
                return 0;
        }
-       else if (strcmp(te->desc, "TRIGGER") == 0)
+
+       if (ropt->schemaExcludeNames.head != NULL &&
+           te->namespace &&
+           simple_string_list_member(&ropt->schemaExcludeNames, te->namespace))
+           return 0;
+
+       if (ropt->selTypes)
        {
-           if (!ropt->selTrigger)
-               return 0;
-           if (ropt->triggerNames.head != NULL && (!(simple_string_list_member(&ropt->triggerNames, te->tag))))
+           if (strcmp(te->desc, "TABLE") == 0 ||
+               strcmp(te->desc, "TABLE DATA") == 0 ||
+               strcmp(te->desc, "VIEW") == 0 ||
+               strcmp(te->desc, "FOREIGN TABLE") == 0 ||
+               strcmp(te->desc, "MATERIALIZED VIEW") == 0 ||
+               strcmp(te->desc, "MATERIALIZED VIEW DATA") == 0 ||
+               strcmp(te->desc, "SEQUENCE") == 0 ||
+               strcmp(te->desc, "SEQUENCE SET") == 0)
+           {
+               if (!ropt->selTable)
+                   return 0;
+               if (ropt->tableNames.head != NULL &&
+                   !simple_string_list_member(&ropt->tableNames, te->tag))
+                   return 0;
+           }
+           else if (strcmp(te->desc, "INDEX") == 0)
+           {
+               if (!ropt->selIndex)
+                   return 0;
+               if (ropt->indexNames.head != NULL &&
+                   !simple_string_list_member(&ropt->indexNames, te->tag))
+                   return 0;
+           }
+           else if (strcmp(te->desc, "FUNCTION") == 0 ||
+                    strcmp(te->desc, "AGGREGATE") == 0 ||
+                    strcmp(te->desc, "PROCEDURE") == 0)
+           {
+               if (!ropt->selFunction)
+                   return 0;
+               if (ropt->functionNames.head != NULL &&
+                   !simple_string_list_member(&ropt->functionNames, te->tag))
+                   return 0;
+           }
+           else if (strcmp(te->desc, "TRIGGER") == 0)
+           {
+               if (!ropt->selTrigger)
+                   return 0;
+               if (ropt->triggerNames.head != NULL &&
+                   !simple_string_list_member(&ropt->triggerNames, te->tag))
+                   return 0;
+           }
+           else
                return 0;
        }
-       else
-           return 0;
    }
 
    /*
-    * Check if we had a dataDumper. Indicates if the entry is schema or data
+    * Determine whether the TOC entry contains schema and/or data components,
+    * and mask off inapplicable REQ bits.  If it had a dataDumper, assume
+    * it's both schema and data.  Otherwise it's probably schema-only, but
+    * there are exceptions.
     */
    if (!te->hadDumper)
    {
@@ -2952,6 +3008,10 @@ _tocEntryRequired(TocEntry *te, teSection curSection, RestoreOptions *ropt)
            res = res & ~REQ_DATA;
    }
 
+   /* If there's no definition command, there's no schema component */
+   if (!te->defn || !te->defn[0])
+       res = res & ~REQ_SCHEMA;
+
    /*
     * Special case: <Init> type with <Max OID> tag; this is obsolete and we
     * always ignore it.
@@ -2963,12 +3023,12 @@ _tocEntryRequired(TocEntry *te, teSection curSection, RestoreOptions *ropt)
    if (ropt->schemaOnly)
    {
        /*
-        * The sequence_data option overrides schema-only for SEQUENCE SET.
+        * The sequence_data option overrides schemaOnly for SEQUENCE SET.
         *
-        * In binary-upgrade mode, even with schema-only set, we do not mask
-        * out large objects.  Only large object definitions, comments and
-        * other information should be generated in binary-upgrade mode (not
-        * the actual data).
+        * In binary-upgrade mode, even with schemaOnly set, we do not mask
+        * out large objects.  (Only large object definitions, comments and
+        * other metadata should be generated in binary-upgrade mode, not the
+        * actual data, but that need not concern us here.)
         */
        if (!(ropt->sequence_data && strcmp(te->desc, "SEQUENCE SET") == 0) &&
            !(ropt->binary_upgrade &&
@@ -2986,14 +3046,6 @@ _tocEntryRequired(TocEntry *te, teSection curSection, RestoreOptions *ropt)
    if (ropt->dataOnly)
        res = res & REQ_DATA;
 
-   /* Mask it if we don't have a schema contribution */
-   if (!te->defn || strlen(te->defn) == 0)
-       res = res & ~REQ_SCHEMA;
-
-   /* 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_dump.c b/src/bin/pg_dump/pg_dump.c
index b534fb7b95daec5bcdd2d0b14be0f77900c768d0..c34d9907299dbbc90ce49dd211f7e7ddf4cb7688 100644 (file)
--- a/src/bin/pg_dump/pg_dump.c
+++ b/src/bin/pg_dump/pg_dump.c
@@ -631,6 +631,13 @@ main(int argc, char **argv)
    compressLevel = 0;
 #endif
 
+   /*
+    * If emitting an archive format, we always want to emit a DATABASE item,
+    * in case --create is specified at pg_restore time.
+    */
+   if (!plainText)
+       dopt.outputCreateDB = 1;
+
    /*
     * On Windows we can only have at most MAXIMUM_WAIT_OBJECTS (= 64 usually)
     * parallel jobs because that's the maximum limit for the
@@ -841,7 +848,7 @@ main(int argc, char **argv)
    dumpStdStrings(fout);
 
    /* The database items are always next, unless we don't want them at all */
-   if (dopt.include_everything && !dopt.dataOnly)
+   if (dopt.outputCreateDB)
        dumpDatabase(fout);
 
    /* Now the rearrangeable objects. */