Re: pg_dump versus rules, once again

I wrote:
> We've talked before about replacing this _RETURN-rule business with
> CREATE OR REPLACE VIEW, ie the idea would be for pg_dump to first emit
> a dummy view with the right column names/types, say
> CREATE VIEW vv AS SELECT null::int AS f1, null::text AS f2;
> and then later when it's safe, emit CREATE OR REPLACE VIEW with the view's
> real query.

Here's a proposed patch for that.  It turns out there are some other
kluges that can be gotten rid of while we're at it: no longer need the
idea of reloptions being attached to a rule, for instance.

The changes in pg_backup_archiver.c would have to be back-patched
into all versions supporting --if-exists, so that they don't fail
on dump archives produced by patched versions.  We could possibly
put the rest only into HEAD; I remain of two minds about that.

            regards, tom lane

diff --git a/src/bin/pg_dump/pg_backup_archiver.c b/src/bin/pg_dump/pg_backup_archiver.c
index b938d79..b89bd99 100644
*** a/src/bin/pg_dump/pg_backup_archiver.c
--- b/src/bin/pg_dump/pg_backup_archiver.c
*************** RestoreArchive(Archive *AHX)
*** 521,527 ****
                           * knows how to do it, without depending on
                           * te->dropStmt; use that.  For other objects we need
                           * to parse the command.
-                          *
                           */
                          if (strncmp(te->desc, "BLOB", 4) == 0)
                          {
--- 521,526 ----
*************** RestoreArchive(Archive *AHX)
*** 529,538 ****
                          }
                          else
                          {
-                             char        buffer[40];
-                             char       *mark;
                              char       *dropStmt = pg_strdup(te->dropStmt);
!                             char       *dropStmtPtr = dropStmt;
                              PQExpBuffer ftStmt = createPQExpBuffer();

                              /*
--- 528,535 ----
                          }
                          else
                          {
                              char       *dropStmt = pg_strdup(te->dropStmt);
!                             char       *dropStmtOrig = dropStmt;
                              PQExpBuffer ftStmt = createPQExpBuffer();

                              /*
*************** RestoreArchive(Archive *AHX)
*** 549,566 ****
                              /*
                               * ALTER TABLE..ALTER COLUMN..DROP DEFAULT does
                               * not support the IF EXISTS clause, and therefore
!                              * we simply emit the original command for such
!                              * objects. For other objects, we need to extract
!                              * the first part of the DROP which includes the
!                              * object type. Most of the time this matches
                               * te->desc, so search for that; however for the
                               * different kinds of CONSTRAINTs, we know to
                               * search for hardcoded "DROP CONSTRAINT" instead.
                               */
!                             if (strcmp(te->desc, "DEFAULT") == 0)
                                  appendPQExpBufferStr(ftStmt, dropStmt);
                              else
                              {
                                  if (strcmp(te->desc, "CONSTRAINT") == 0 ||
                                   strcmp(te->desc, "CHECK CONSTRAINT") == 0 ||
                                      strcmp(te->desc, "FK CONSTRAINT") == 0)
--- 546,573 ----
                              /*
                               * ALTER TABLE..ALTER COLUMN..DROP DEFAULT does
                               * not support the IF EXISTS clause, and therefore
!                              * we simply emit the original command for DEFAULT
!                              * objects (modulo the adjustment made above).
!                              *
!                              * If we used CREATE OR REPLACE VIEW as a means of
!                              * quasi-dropping an ON SELECT rule, that should
!                              * be emitted unchanged as well.
!                              *
!                              * For other object types, we need to extract the
!                              * first part of the DROP which includes the
!                              * object type.  Most of the time this matches
                               * te->desc, so search for that; however for the
                               * different kinds of CONSTRAINTs, we know to
                               * search for hardcoded "DROP CONSTRAINT" instead.
                               */
!                             if (strcmp(te->desc, "DEFAULT") == 0 ||
!                                 strncmp(dropStmt, "CREATE OR REPLACE VIEW", 22) == 0)
                                  appendPQExpBufferStr(ftStmt, dropStmt);
                              else
                              {
+                                 char        buffer[40];
+                                 char       *mark;
+
                                  if (strcmp(te->desc, "CONSTRAINT") == 0 ||
                                   strcmp(te->desc, "CHECK CONSTRAINT") == 0 ||
                                      strcmp(te->desc, "FK CONSTRAINT") == 0)
*************** RestoreArchive(Archive *AHX)
*** 570,588 ****
                                               te->desc);

                                  mark = strstr(dropStmt, buffer);
-                                 Assert(mark != NULL);

!                                 *mark = '\0';
!                                 appendPQExpBuffer(ftStmt, "%s%s IF EXISTS%s",
!                                                   dropStmt, buffer,
!                                                   mark + strlen(buffer));
                              }

                              ahprintf(AH, "%s", ftStmt->data);

                              destroyPQExpBuffer(ftStmt);
!
!                             pg_free(dropStmtPtr);
                          }
                      }
                  }
--- 577,604 ----
                                               te->desc);

                                  mark = strstr(dropStmt, buffer);

!                                 if (mark)
!                                 {
!                                     *mark = '\0';
!                                     appendPQExpBuffer(ftStmt, "%s%s IF EXISTS%s",
!                                                       dropStmt, buffer,
!                                                       mark + strlen(buffer));
!                                 }
!                                 else
!                                 {
!                                     /* complain and emit unmodified command */
!                                     write_msg(modulename,
!                                               "WARNING: could not find where to insert IF EXISTS in statement
\"%s\"\n",
!                                               dropStmtOrig);
!                                     appendPQExpBufferStr(ftStmt, dropStmt);
!                                 }
                              }

                              ahprintf(AH, "%s", ftStmt->data);

                              destroyPQExpBuffer(ftStmt);
!                             pg_free(dropStmtOrig);
                          }
                      }
                  }
diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c
index ee1f673..9f59f53 100644
*** a/src/bin/pg_dump/pg_dump.c
--- b/src/bin/pg_dump/pg_dump.c
*************** getTables(Archive *fout, int *numTables)
*** 5484,5490 ****
              tblinfo[i].dobj.dump &= ~DUMP_COMPONENT_ACL;

          tblinfo[i].interesting = tblinfo[i].dobj.dump ? true : false;
!
          tblinfo[i].postponed_def = false;        /* might get set during sort */

          /*
--- 5486,5492 ----
              tblinfo[i].dobj.dump &= ~DUMP_COMPONENT_ACL;

          tblinfo[i].interesting = tblinfo[i].dobj.dump ? true : false;
!         tblinfo[i].dummy_view = false;    /* might get set during sort */
          tblinfo[i].postponed_def = false;        /* might get set during sort */

          /*
*************** getRules(Archive *fout, int *numRules)
*** 6205,6220 ****
          }
          else
              ruleinfo[i].separate = true;
-
-         /*
-          * If we're forced to break a dependency loop by dumping a view as a
-          * table and separate _RETURN rule, we'll move the view's reloptions
-          * to the rule.  (This is necessary because tables and views have
-          * different valid reloptions, so we can't apply the options until the
-          * backend knows it's a view.)  Otherwise the rule's reloptions stay
-          * NULL.
-          */
-         ruleinfo[i].reloptions = NULL;
      }

      PQclear(res);
--- 6207,6212 ----
*************** createViewAsClause(Archive *fout, TableI
*** 13976,13981 ****
--- 13968,14021 ----
  }

  /*
+  * Create a dummy AS clause for a view.  This is used when the real view
+  * definition has to be postponed because of circular dependencies.
+  * We must duplicate the view's external properties -- column names and types
+  * (including collation) -- so that it works for subsequent references.
+  *
+  * This returns a new buffer which must be freed by the caller.
+  */
+ static PQExpBuffer
+ createDummyViewAsClause(Archive *fout, TableInfo *tbinfo)
+ {
+     PQExpBuffer result = createPQExpBuffer();
+     int            j;
+
+     appendPQExpBufferStr(result, "SELECT");
+
+     for (j = 0; j < tbinfo->numatts; j++)
+     {
+         if (j > 0)
+             appendPQExpBufferChar(result, ',');
+         appendPQExpBufferStr(result, "\n    ");
+
+         appendPQExpBuffer(result, "NULL::%s", tbinfo->atttypnames[j]);
+
+         /*
+          * Must add collation if not default for the type, because CREATE OR
+          * REPLACE VIEW won't change it
+          */
+         if (OidIsValid(tbinfo->attcollation[j]))
+         {
+             CollInfo   *coll;
+
+             coll = findCollationByOid(tbinfo->attcollation[j]);
+             if (coll)
+             {
+                 /* always schema-qualify, don't try to be smart */
+                 appendPQExpBuffer(result, " COLLATE %s.",
+                                   fmtId(coll->dobj.namespace->dobj.name));
+                 appendPQExpBufferStr(result, fmtId(coll->dobj.name));
+             }
+         }
+
+         appendPQExpBuffer(result, " AS %s", fmtId(tbinfo->attnames[j]));
+     }
+
+     return result;
+ }
+
+ /*
   * dumpTableSchema
   *      write the declaration (not data) of one user-defined table or view
   */
*************** dumpTableSchema(Archive *fout, TableInfo
*** 14008,14013 ****
--- 14048,14057 ----
      {
          PQExpBuffer result;

+         /*
+          * Note: keep this code in sync with the is_view case in dumpRule()
+          */
+
          reltypename = "VIEW";

          /*
*************** dumpTableSchema(Archive *fout, TableInfo
*** 14024,14040 ****
                                               tbinfo->dobj.catId.oid, false);

          appendPQExpBuffer(q, "CREATE VIEW %s", fmtId(tbinfo->dobj.name));
!         if (nonemptyReloptions(tbinfo->reloptions))
          {
!             appendPQExpBufferStr(q, " WITH (");
!             appendReloptionsArrayAH(q, tbinfo->reloptions, "", fout);
!             appendPQExpBufferChar(q, ')');
          }
-         result = createViewAsClause(fout, tbinfo);
          appendPQExpBuffer(q, " AS\n%s", result->data);
          destroyPQExpBuffer(result);

!         if (tbinfo->checkoption != NULL)
              appendPQExpBuffer(q, "\n  WITH %s CHECK OPTION", tbinfo->checkoption);
          appendPQExpBufferStr(q, ";\n");

--- 14068,14089 ----
                                               tbinfo->dobj.catId.oid, false);

          appendPQExpBuffer(q, "CREATE VIEW %s", fmtId(tbinfo->dobj.name));
!         if (tbinfo->dummy_view)
!             result = createDummyViewAsClause(fout, tbinfo);
!         else
          {
!             if (nonemptyReloptions(tbinfo->reloptions))
!             {
!                 appendPQExpBufferStr(q, " WITH (");
!                 appendReloptionsArrayAH(q, tbinfo->reloptions, "", fout);
!                 appendPQExpBufferChar(q, ')');
!             }
!             result = createViewAsClause(fout, tbinfo);
          }
          appendPQExpBuffer(q, " AS\n%s", result->data);
          destroyPQExpBuffer(result);

!         if (tbinfo->checkoption != NULL && !tbinfo->dummy_view)
              appendPQExpBuffer(q, "\n  WITH %s CHECK OPTION", tbinfo->checkoption);
          appendPQExpBufferStr(q, ";\n");

*************** dumpRule(Archive *fout, RuleInfo *rinfo)
*** 15646,15651 ****
--- 15695,15701 ----
  {
      DumpOptions *dopt = fout->dopt;
      TableInfo  *tbinfo = rinfo->ruletable;
+     bool        is_view;
      PQExpBuffer query;
      PQExpBuffer cmd;
      PQExpBuffer delcmd;
*************** dumpRule(Archive *fout, RuleInfo *rinfo)
*** 15665,15670 ****
--- 15715,15726 ----
          return;

      /*
+      * If it's an ON SELECT rule, we want to print it as a view definition,
+      * instead of a rule.
+      */
+     is_view = (rinfo->ev_type == '1' && rinfo->is_instead);
+
+     /*
       * Make sure we are in proper schema.
       */
      selectSourceSchema(fout, tbinfo->dobj.namespace->dobj.name);
*************** dumpRule(Archive *fout, RuleInfo *rinfo)
*** 15674,15693 ****
      delcmd = createPQExpBuffer();
      labelq = createPQExpBuffer();

!     appendPQExpBuffer(query,
!       "SELECT pg_catalog.pg_get_ruledef('%u'::pg_catalog.oid) AS definition",
!                       rinfo->dobj.catId.oid);
!
!     res = ExecuteSqlQuery(fout, query->data, PGRES_TUPLES_OK);
!
!     if (PQntuples(res) != 1)
      {
!         write_msg(NULL, "query to get rule \"%s\" for table \"%s\" failed: wrong number of rows returned\n",
!                   rinfo->dobj.name, tbinfo->dobj.name);
!         exit_nicely(1);
      }

!     printfPQExpBuffer(cmd, "%s\n", PQgetvalue(res, 0, 0));

      /*
       * Add the command to alter the rules replication firing semantics if it
--- 15730,15779 ----
      delcmd = createPQExpBuffer();
      labelq = createPQExpBuffer();

!     if (is_view)
      {
!         PQExpBuffer result;
!
!         /*
!          * We need OR REPLACE here because we'll be replacing a dummy view.
!          * Otherwise this should look largely like the regular view dump code.
!          */
!         appendPQExpBuffer(cmd, "CREATE OR REPLACE VIEW %s",
!                           fmtId(tbinfo->dobj.name));
!         if (nonemptyReloptions(tbinfo->reloptions))
!         {
!             appendPQExpBufferStr(cmd, " WITH (");
!             appendReloptionsArrayAH(cmd, tbinfo->reloptions, "", fout);
!             appendPQExpBufferChar(cmd, ')');
!         }
!         result = createViewAsClause(fout, tbinfo);
!         appendPQExpBuffer(cmd, " AS\n%s", result->data);
!         destroyPQExpBuffer(result);
!         if (tbinfo->checkoption != NULL)
!             appendPQExpBuffer(cmd, "\n  WITH %s CHECK OPTION",
!                               tbinfo->checkoption);
!         appendPQExpBufferStr(cmd, ";\n");
      }
+     else
+     {
+         /* In the rule case, just print pg_get_ruledef's result verbatim */
+         appendPQExpBuffer(query,
+                     "SELECT pg_catalog.pg_get_ruledef('%u'::pg_catalog.oid)",
+                           rinfo->dobj.catId.oid);

!         res = ExecuteSqlQuery(fout, query->data, PGRES_TUPLES_OK);
!
!         if (PQntuples(res) != 1)
!         {
!             write_msg(NULL, "query to get rule \"%s\" for table \"%s\" failed: wrong number of rows returned\n",
!                       rinfo->dobj.name, tbinfo->dobj.name);
!             exit_nicely(1);
!         }
!
!         printfPQExpBuffer(cmd, "%s\n", PQgetvalue(res, 0, 0));
!
!         PQclear(res);
!     }

      /*
       * Add the command to alter the rules replication firing semantics if it
*************** dumpRule(Archive *fout, RuleInfo *rinfo)
*** 15714,15738 ****
      }

      /*
!      * Apply view's reloptions when its ON SELECT rule is separate.
       */
!     if (nonemptyReloptions(rinfo->reloptions))
      {
!         appendPQExpBuffer(cmd, "ALTER VIEW %s SET (",
                            fmtId(tbinfo->dobj.name));
-         appendReloptionsArrayAH(cmd, rinfo->reloptions, "", fout);
-         appendPQExpBufferStr(cmd, ");\n");
      }
-
-     /*
-      * DROP must be fully qualified in case same name appears in pg_catalog
-      */
-     appendPQExpBuffer(delcmd, "DROP RULE %s ",
-                       fmtId(rinfo->dobj.name));
-     appendPQExpBuffer(delcmd, "ON %s.",
-                       fmtId(tbinfo->dobj.namespace->dobj.name));
-     appendPQExpBuffer(delcmd, "%s;\n",
-                       fmtId(tbinfo->dobj.name));

      appendPQExpBuffer(labelq, "RULE %s",
                        fmtId(rinfo->dobj.name));
--- 15800,15833 ----
      }

      /*
!      * DROP must be fully qualified in case same name appears in pg_catalog
       */
!     if (is_view)
      {
!         /*
!          * We can't DROP a view's ON SELECT rule.  Instead, use CREATE OR
!          * REPLACE VIEW to replace the rule with something with minimal
!          * dependencies.
!          */
!         PQExpBuffer result;
!
!         appendPQExpBuffer(delcmd, "CREATE OR REPLACE VIEW %s.",
!                           fmtId(tbinfo->dobj.namespace->dobj.name));
!         appendPQExpBuffer(delcmd, "%s",
!                           fmtId(tbinfo->dobj.name));
!         result = createDummyViewAsClause(fout, tbinfo);
!         appendPQExpBuffer(delcmd, " AS\n%s;\n", result->data);
!         destroyPQExpBuffer(result);
!     }
!     else
!     {
!         appendPQExpBuffer(delcmd, "DROP RULE %s ",
!                           fmtId(rinfo->dobj.name));
!         appendPQExpBuffer(delcmd, "ON %s.",
!                           fmtId(tbinfo->dobj.namespace->dobj.name));
!         appendPQExpBuffer(delcmd, "%s;\n",
                            fmtId(tbinfo->dobj.name));
      }

      appendPQExpBuffer(labelq, "RULE %s",
                        fmtId(rinfo->dobj.name));
*************** dumpRule(Archive *fout, RuleInfo *rinfo)
*** 15759,15766 ****
                      tbinfo->rolname,
                      rinfo->dobj.catId, 0, rinfo->dobj.dumpId);

-     PQclear(res);
-
      free(tag);
      destroyPQExpBuffer(query);
      destroyPQExpBuffer(cmd);
--- 15854,15859 ----
diff --git a/src/bin/pg_dump/pg_dump.h b/src/bin/pg_dump/pg_dump.h
index 642c4d5..f3e5977 100644
*** a/src/bin/pg_dump/pg_dump.h
--- b/src/bin/pg_dump/pg_dump.h
*************** typedef struct _tableInfo
*** 287,292 ****
--- 287,293 ----
      int            relpages;        /* table's size in pages (from pg_class) */

      bool        interesting;    /* true if need to collect more data */
+     bool        dummy_view;        /* view's real definition must be postponed */
      bool        postponed_def;    /* matview must be postponed into post-data */

      /*
*************** typedef struct _ruleInfo
*** 364,371 ****
      char        ev_enabled;
      bool        separate;        /* TRUE if must dump as separate item */
      /* separate is always true for non-ON SELECT rules */
-     char       *reloptions;        /* options specified by WITH (...) */
-     /* reloptions is only set if we need to dump the options with the rule */
  } RuleInfo;

  typedef struct _triggerInfo
--- 365,370 ----
diff --git a/src/bin/pg_dump/pg_dump_sort.c b/src/bin/pg_dump/pg_dump_sort.c
index 5b96334..9966423 100644
*** a/src/bin/pg_dump/pg_dump_sort.c
--- b/src/bin/pg_dump/pg_dump_sort.c
*************** repairViewRuleLoop(DumpableObject *viewo
*** 786,791 ****
--- 786,792 ----
  {
      /* remove rule's dependency on view */
      removeObjectDependency(ruleobj, viewobj->dumpId);
+     /* flags on the two objects are already set correctly for this case */
  }

  /*
*************** repairViewRuleMultiLoop(DumpableObject *
*** 805,831 ****
  {
      TableInfo  *viewinfo = (TableInfo *) viewobj;
      RuleInfo   *ruleinfo = (RuleInfo *) ruleobj;
-     int            i;

      /* remove view's dependency on rule */
      removeObjectDependency(viewobj, ruleobj->dumpId);
!     /* pretend view is a plain table and dump it that way */
!     viewinfo->relkind = 'r';    /* RELKIND_RELATION */
      /* mark rule as needing its own dump */
      ruleinfo->separate = true;
-     /* move any reloptions from view to rule */
-     if (viewinfo->reloptions)
-     {
-         ruleinfo->reloptions = viewinfo->reloptions;
-         viewinfo->reloptions = NULL;
-     }
      /* put back rule's dependency on view */
      addObjectDependency(ruleobj, viewobj->dumpId);
      /* now that rule is separate, it must be post-data */
      addObjectDependency(ruleobj, postDataBoundId);
-     /* also, any triggers on the view must be dumped after the rule */
-     for (i = 0; i < viewinfo->numTriggers; i++)
-         addObjectDependency(&(viewinfo->triggers[i].dobj), ruleobj->dumpId);
  }

  /*
--- 806,822 ----
  {
      TableInfo  *viewinfo = (TableInfo *) viewobj;
      RuleInfo   *ruleinfo = (RuleInfo *) ruleobj;

      /* remove view's dependency on rule */
      removeObjectDependency(viewobj, ruleobj->dumpId);
!     /* mark view to be printed with a dummy definition */
!     viewinfo->dummy_view = true;
      /* mark rule as needing its own dump */
      ruleinfo->separate = true;
      /* put back rule's dependency on view */
      addObjectDependency(ruleobj, viewobj->dumpId);
      /* now that rule is separate, it must be post-data */
      addObjectDependency(ruleobj, postDataBoundId);
  }

  /*