pg_upgrade cleanup for map struct creation - Mailing list pgsql-hackers

The attached, applied patch clarifies pg_upgrade's creation of the map
file structure.  It also cleans up pg_dump's calling of
pg_upgrade_support functions.

--
  Bruce Momjian  <bruce@momjian.us>        http://momjian.us
  EnterpriseDB                             http://enterprisedb.com

  + It's impossible for everything to be true. +
diff --git a/contrib/pg_upgrade/info.c b/contrib/pg_upgrade/info.c
index 8d566c0..83afb92 100644
*** /tmp/pgdiff.7809/Jqhj6c_info.c    Wed Jan  5 11:35:06 2011
--- contrib/pg_upgrade/info.c    Wed Jan  5 11:29:51 2011
*************** static RelInfo *relarr_lookup_rel_oid(Cl
*** 33,40 ****
   * generates database mappings for "old_db" and "new_db". Returns a malloc'ed
   * array of mappings. nmaps is a return parameter which refers to the number
   * mappings.
-  *
-  * NOTE: Its the Caller's responsibility to free the returned array.
   */
  FileNameMap *
  gen_db_file_maps(DbInfo *old_db, DbInfo *new_db,
--- 33,38 ----
*************** gen_db_file_maps(DbInfo *old_db, DbInfo
*** 45,63 ****
      int            num_maps = 0;

      maps = (FileNameMap *) pg_malloc(sizeof(FileNameMap) *
!                                      new_db->rel_arr.nrels);

!     for (relnum = 0; relnum < new_db->rel_arr.nrels; relnum++)
      {
!         RelInfo    *newrel = &new_db->rel_arr.rels[relnum];
!         RelInfo    *oldrel;

!         /* toast tables are handled by their parent */
!         if (strcmp(newrel->nspname, "pg_toast") == 0)
              continue;

!         oldrel = relarr_lookup_rel_name(&old_cluster, &old_db->rel_arr,
!                                    newrel->nspname, newrel->relname);

          create_rel_filename_map(old_pgdata, new_pgdata, old_db, new_db,
                  oldrel, newrel, maps + num_maps);
--- 43,61 ----
      int            num_maps = 0;

      maps = (FileNameMap *) pg_malloc(sizeof(FileNameMap) *
!                                      old_db->rel_arr.nrels);

!     for (relnum = 0; relnum < old_db->rel_arr.nrels; relnum++)
      {
!         RelInfo    *oldrel = &old_db->rel_arr.rels[relnum];
!         RelInfo    *newrel;

!         /* toast tables are handled by their parents */
!         if (strcmp(oldrel->nspname, "pg_toast") == 0)
              continue;

!         newrel = relarr_lookup_rel_name(&old_cluster, &old_db->rel_arr,
!                                    oldrel->nspname, oldrel->relname);

          create_rel_filename_map(old_pgdata, new_pgdata, old_db, new_db,
                  oldrel, newrel, maps + num_maps);
*************** gen_db_file_maps(DbInfo *old_db, DbInfo
*** 65,116 ****

          /*
           * So much for mapping this relation;  now we need a mapping
!          * for its corresponding toast relation, if any.
           */
          if (oldrel->toastrelid > 0)
          {
!             RelInfo    *new_toast;
!             RelInfo    *old_toast;
!             char        new_name[MAXPGPATH];
!             char        old_name[MAXPGPATH];
!
!             /* construct the new and old relnames for the toast relation */
!             snprintf(old_name, sizeof(old_name), "pg_toast_%u", oldrel->reloid);
!             snprintf(new_name, sizeof(new_name), "pg_toast_%u", newrel->reloid);

-             /* look them up in their respective arrays */
              old_toast = relarr_lookup_rel_oid(&old_cluster, &old_db->rel_arr,
!                                              oldrel->toastrelid);
!             new_toast = relarr_lookup_rel_name(&new_cluster, &new_db->rel_arr,
!                                           "pg_toast", new_name);

-             /* finally create a mapping for them */
              create_rel_filename_map(old_pgdata, new_pgdata, old_db, new_db,
                      old_toast, new_toast, maps + num_maps);
              num_maps++;

              /*
!              * also need to provide a mapping for the index of this toast
               * relation. The procedure is similar to what we did above for
               * toast relation itself, the only difference being that the
               * relnames need to be appended with _index.
               */
-
-             /*
-              * construct the new and old relnames for the toast index
-              * relations
-              */
              snprintf(old_name, sizeof(old_name), "%s_index", old_toast->relname);
!             snprintf(new_name, sizeof(new_name), "pg_toast_%u_index",
!                      newrel->reloid);

-             /* look them up in their respective arrays */
              old_toast = relarr_lookup_rel_name(&old_cluster, &old_db->rel_arr,
                                            "pg_toast", old_name);
              new_toast = relarr_lookup_rel_name(&new_cluster, &new_db->rel_arr,
                                            "pg_toast", new_name);

-             /* finally create a mapping for them */
              create_rel_filename_map(old_pgdata, new_pgdata, old_db,
                      new_db, old_toast, new_toast, maps + num_maps);
              num_maps++;
--- 63,98 ----

          /*
           * So much for mapping this relation;  now we need a mapping
!          * for its corresponding toast relation and toast index, if any.
           */
          if (oldrel->toastrelid > 0)
          {
!             char        old_name[MAXPGPATH], new_name[MAXPGPATH];
!             RelInfo    *old_toast, *new_toast;

              old_toast = relarr_lookup_rel_oid(&old_cluster, &old_db->rel_arr,
!                                               oldrel->toastrelid);
!             new_toast = relarr_lookup_rel_oid(&new_cluster, &new_db->rel_arr,
!                                               newrel->toastrelid);

              create_rel_filename_map(old_pgdata, new_pgdata, old_db, new_db,
                      old_toast, new_toast, maps + num_maps);
              num_maps++;

              /*
!              * We also need to provide a mapping for the index of this toast
               * relation. The procedure is similar to what we did above for
               * toast relation itself, the only difference being that the
               * relnames need to be appended with _index.
               */
              snprintf(old_name, sizeof(old_name), "%s_index", old_toast->relname);
!             snprintf(new_name, sizeof(new_name), "%s_index", new_toast->relname);

              old_toast = relarr_lookup_rel_name(&old_cluster, &old_db->rel_arr,
                                            "pg_toast", old_name);
              new_toast = relarr_lookup_rel_name(&new_cluster, &new_db->rel_arr,
                                            "pg_toast", new_name);

              create_rel_filename_map(old_pgdata, new_pgdata, old_db,
                      new_db, old_toast, new_toast, maps + num_maps);
              num_maps++;
*************** create_rel_filename_map(const char *old_
*** 133,147 ****
                const RelInfo *old_rel, const RelInfo *new_rel,
                FileNameMap *map)
  {
-     map->old_relfilenode = old_rel->relfilenode;
-     map->new_relfilenode = new_rel->relfilenode;
-
-     snprintf(map->old_nspname, sizeof(map->old_nspname), "%s", old_rel->nspname);
-     snprintf(map->new_nspname, sizeof(map->new_nspname), "%s", new_rel->nspname);
-
-     snprintf(map->old_relname, sizeof(map->old_relname), "%s", old_rel->relname);
-     snprintf(map->new_relname, sizeof(map->new_relname), "%s", new_rel->relname);
-
      if (strlen(old_rel->tablespace) == 0)
      {
          /*
--- 115,120 ----
*************** create_rel_filename_map(const char *old_
*** 155,168 ****
      }
      else
      {
!         /*
!          * relation belongs to some tablespace, so use the tablespace location
!          */
          snprintf(map->old_dir, sizeof(map->old_dir), "%s%s/%u", old_rel->tablespace,
                   old_cluster.tablespace_suffix, old_db->db_oid);
          snprintf(map->new_dir, sizeof(map->new_dir), "%s%s/%u", new_rel->tablespace,
                   new_cluster.tablespace_suffix, new_db->db_oid);
      }
  }


--- 128,148 ----
      }
      else
      {
!         /* relation belongs to a tablespace, so use the tablespace location */
          snprintf(map->old_dir, sizeof(map->old_dir), "%s%s/%u", old_rel->tablespace,
                   old_cluster.tablespace_suffix, old_db->db_oid);
          snprintf(map->new_dir, sizeof(map->new_dir), "%s%s/%u", new_rel->tablespace,
                   new_cluster.tablespace_suffix, new_db->db_oid);
      }
+
+     map->old_relfilenode = old_rel->relfilenode;
+     map->new_relfilenode = new_rel->relfilenode;
+
+     /* used only for logging and error reporing */
+     snprintf(map->old_nspname, sizeof(map->old_nspname), "%s", old_rel->nspname);
+     snprintf(map->new_nspname, sizeof(map->new_nspname), "%s", new_rel->nspname);
+     snprintf(map->old_relname, sizeof(map->old_relname), "%s", old_rel->relname);
+     snprintf(map->new_relname, sizeof(map->new_relname), "%s", new_rel->relname);
  }


diff --git a/contrib/pg_upgrade/pg_upgrade.h b/contrib/pg_upgrade/pg_upgrade.h
index 3bbddee..d863155 100644
*** /tmp/pgdiff.7809/LJQmjc_pg_upgrade.h    Wed Jan  5 11:35:06 2011
--- contrib/pg_upgrade/pg_upgrade.h    Wed Jan  5 11:27:02 2011
*************** typedef struct
*** 87,98 ****
  {
      char        old_dir[MAXPGPATH];
      char        new_dir[MAXPGPATH];
!     Oid            old_relfilenode;    /* Relfilenode of the old relation */
!     Oid            new_relfilenode;    /* Relfilenode of the new relation */
!     char        old_nspname[NAMEDATALEN];        /* old name of the namespace */
!     char        old_relname[NAMEDATALEN];        /* old name of the relation */
!     char        new_nspname[NAMEDATALEN];        /* new name of the namespace */
!     char        new_relname[NAMEDATALEN];        /* new name of the relation */
  } FileNameMap;

  /*
--- 87,104 ----
  {
      char        old_dir[MAXPGPATH];
      char        new_dir[MAXPGPATH];
!     /*
!      * old/new relfilenodes might differ for pg_largeobject(_metadata) indexes
!      * due to VACUUM FULL or REINDEX.  Other relfilenodes are preserved.
!      */
!     Oid            old_relfilenode;
!     Oid            new_relfilenode;
!     /* the rest are used only for logging and error reporting */
!     char        old_nspname[NAMEDATALEN];        /* namespaces */
!     char        new_nspname[NAMEDATALEN];
!     /* old/new relnames differ for toast tables and toast indexes */
!     char        old_relname[NAMEDATALEN];
!     char        new_relname[NAMEDATALEN];
  } FileNameMap;

  /*
diff --git a/contrib/pg_upgrade/version_old_8_3.c b/contrib/pg_upgrade/version_old_8_3.c
index 664380b..5b226b2 100644
*** /tmp/pgdiff.7809/HrmZ2a_version_old_8_3.c    Wed Jan  5 11:35:06 2011
--- contrib/pg_upgrade/version_old_8_3.c    Wed Jan  5 11:21:39 2011
*************** old_8_3_rebuild_tsvector_tables(ClusterI
*** 222,229 ****
      {
          PGresult   *res;
          bool        db_used = false;
!         char        old_nspname[NAMEDATALEN] = "",
!                     old_relname[NAMEDATALEN] = "";
          int            ntups;
          int            rowno;
          int            i_nspname,
--- 222,229 ----
      {
          PGresult   *res;
          bool        db_used = false;
!         char        nspname[NAMEDATALEN] = "",
!                     relname[NAMEDATALEN] = "";
          int            ntups;
          int            rowno;
          int            i_nspname,
*************** old_8_3_rebuild_tsvector_tables(ClusterI
*** 283,292 ****
                  }

                  /* Rebuild all tsvector collumns with one ALTER TABLE command */
!                 if (strcmp(PQgetvalue(res, rowno, i_nspname), old_nspname) != 0 ||
!                  strcmp(PQgetvalue(res, rowno, i_relname), old_relname) != 0)
                  {
!                     if (strlen(old_nspname) != 0 || strlen(old_relname) != 0)
                          fprintf(script, ";\n\n");
                      fprintf(script, "ALTER TABLE %s.%s\n",
                           quote_identifier(PQgetvalue(res, rowno, i_nspname)),
--- 283,292 ----
                  }

                  /* Rebuild all tsvector collumns with one ALTER TABLE command */
!                 if (strcmp(PQgetvalue(res, rowno, i_nspname), nspname) != 0 ||
!                  strcmp(PQgetvalue(res, rowno, i_relname), relname) != 0)
                  {
!                     if (strlen(nspname) != 0 || strlen(relname) != 0)
                          fprintf(script, ";\n\n");
                      fprintf(script, "ALTER TABLE %s.%s\n",
                           quote_identifier(PQgetvalue(res, rowno, i_nspname)),
*************** old_8_3_rebuild_tsvector_tables(ClusterI
*** 294,301 ****
                  }
                  else
                      fprintf(script, ",\n");
!                 strlcpy(old_nspname, PQgetvalue(res, rowno, i_nspname), sizeof(old_nspname));
!                 strlcpy(old_relname, PQgetvalue(res, rowno, i_relname), sizeof(old_relname));

                  fprintf(script, "ALTER COLUMN %s "
                  /* This could have been a custom conversion function call. */
--- 294,301 ----
                  }
                  else
                      fprintf(script, ",\n");
!                 strlcpy(nspname, PQgetvalue(res, rowno, i_nspname), sizeof(nspname));
!                 strlcpy(relname, PQgetvalue(res, rowno, i_relname), sizeof(relname));

                  fprintf(script, "ALTER COLUMN %s "
                  /* This could have been a custom conversion function call. */
*************** old_8_3_rebuild_tsvector_tables(ClusterI
*** 304,310 ****
                          quote_identifier(PQgetvalue(res, rowno, i_attname)));
              }
          }
!         if (strlen(old_nspname) != 0 || strlen(old_relname) != 0)
              fprintf(script, ";\n\n");

          PQclear(res);
--- 304,310 ----
                          quote_identifier(PQgetvalue(res, rowno, i_attname)));
              }
          }
!         if (strlen(nspname) != 0 || strlen(relname) != 0)
              fprintf(script, ";\n\n");

          PQclear(res);
diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c
index b0a0dc5..95218ee 100644
*** /tmp/pgdiff.7809/vdmdBd_pg_dump.c    Wed Jan  5 11:35:06 2011
--- src/bin/pg_dump/pg_dump.c    Wed Jan  5 11:21:39 2011
*************** binary_upgrade_set_relfilenodes(PQExpBuf
*** 2354,2387 ****
                      "\n-- For binary upgrade, must preserve relfilenodes\n");

      if (!is_index)
          appendPQExpBuffer(upgrade_buffer,
                            "SELECT binary_upgrade.set_next_heap_relfilenode('%u'::pg_catalog.oid);\n",
                            pg_class_relfilenode);
      else
          appendPQExpBuffer(upgrade_buffer,
                            "SELECT binary_upgrade.set_next_index_relfilenode('%u'::pg_catalog.oid);\n",
                            pg_class_relfilenode);

-     if (OidIsValid(pg_class_reltoastrelid))
-     {
-         /*
-          * One complexity is that the table definition might not require the
-          * creation of a TOAST table, and the TOAST table might have been
-          * created long after table creation, when the table was loaded with
-          * wide data.  By setting the TOAST relfilenode we force creation of
-          * the TOAST heap and TOAST index by the backend so we can cleanly
-          * migrate the files during binary migration.
-          */
-
-         appendPQExpBuffer(upgrade_buffer,
-                           "SELECT binary_upgrade.set_next_toast_relfilenode('%u'::pg_catalog.oid);\n",
-                           pg_class_reltoastrelid);
-
-         /* every toast table has an index */
-         appendPQExpBuffer(upgrade_buffer,
-                           "SELECT binary_upgrade.set_next_index_relfilenode('%u'::pg_catalog.oid);\n",
-                           pg_class_reltoastidxid);
-     }
      appendPQExpBuffer(upgrade_buffer, "\n");

      PQclear(upgrade_res);
--- 2354,2390 ----
                      "\n-- For binary upgrade, must preserve relfilenodes\n");

      if (!is_index)
+     {
          appendPQExpBuffer(upgrade_buffer,
                            "SELECT binary_upgrade.set_next_heap_relfilenode('%u'::pg_catalog.oid);\n",
                            pg_class_relfilenode);
+         /* only tables have toast tables, not indexes */
+         if (OidIsValid(pg_class_reltoastrelid))
+         {
+             /*
+              * One complexity is that the table definition might not require the
+              * creation of a TOAST table, and the TOAST table might have been
+              * created long after table creation, when the table was loaded with
+              * wide data.  By setting the TOAST relfilenode we force creation of
+              * the TOAST heap and TOAST index by the backend so we can cleanly
+              * migrate the files during binary migration.
+              */
+
+             appendPQExpBuffer(upgrade_buffer,
+                               "SELECT binary_upgrade.set_next_toast_relfilenode('%u'::pg_catalog.oid);\n",
+                               pg_class_reltoastrelid);
+
+             /* every toast table has an index */
+             appendPQExpBuffer(upgrade_buffer,
+                               "SELECT binary_upgrade.set_next_index_relfilenode('%u'::pg_catalog.oid);\n",
+                               pg_class_reltoastidxid);
+         }
+     }
      else
          appendPQExpBuffer(upgrade_buffer,
                            "SELECT binary_upgrade.set_next_index_relfilenode('%u'::pg_catalog.oid);\n",
                            pg_class_relfilenode);

      appendPQExpBuffer(upgrade_buffer, "\n");

      PQclear(upgrade_res);