Re: pg_upgade vs config

I wrote:
> It occurs to me that a back-patchable workaround for this would be to
> make get_loadable_libraries sort the library names in order by length
> (and I guess we might as well sort same-length names alphabetically).
> This would for example guarantee that hstore_plpython is probed after
> both hstore and plpython.  Admittedly, this is a kluge of the first
> water.  But I see no prospect of back-patching any real fix, and it
> would definitely be better if pg_upgrade didn't fail on these modules.

I've tested the attached and verified that it allows pg_upgrade'ing
of the hstore_plpython regression DB --- or, if I reverse the sort
order, that it reproducibly fails.  I propose back-patching this
at least as far as 9.5, where the transform modules came in.  It might
be a good idea to go all the way back, just so that the behavior is
predictable.

            regards, tom lane

diff --git a/src/bin/pg_upgrade/function.c b/src/bin/pg_upgrade/function.c
index b432b54..5435bff 100644
*** a/src/bin/pg_upgrade/function.c
--- b/src/bin/pg_upgrade/function.c
***************
*** 15,20 ****
--- 15,44 ----


  /*
+  * qsort comparator for pointers to library names
+  *
+  * We sort first by name length, then alphabetically for names of the same
+  * length.  This is to ensure that, eg, "hstore_plpython" sorts after both
+  * "hstore" and "plpython"; otherwise transform modules will probably fail
+  * their LOAD tests.  (The backend ought to cope with that consideration,
+  * but it doesn't yet, and even when it does it'd be a good idea to have
+  * a predictable order of probing here.)
+  */
+ static int
+ library_name_compare(const void *p1, const void *p2)
+ {
+     const char *str1 = *(const char *const *) p1;
+     const char *str2 = *(const char *const *) p2;
+     int            slen1 = strlen(str1);
+     int            slen2 = strlen(str2);
+
+     if (slen1 != slen2)
+         return slen1 - slen2;
+     return strcmp(str1, str2);
+ }
+
+
+ /*
   * get_loadable_libraries()
   *
   *    Fetch the names of all old libraries containing C-language functions.
*************** get_loadable_libraries(void)
*** 38,47 ****
          PGconn       *conn = connectToServer(&old_cluster, active_db->db_name);

          /*
!          * Fetch all libraries referenced in this DB.  We can't exclude the
!          * "pg_catalog" schema because, while such functions are not
!          * explicitly dumped by pg_dump, they do reference implicit objects
!          * that pg_dump does dump, e.g. CREATE LANGUAGE plperl.
           */
          ress[dbnum] = executeQueryOrDie(conn,
                                          "SELECT DISTINCT probin "
--- 62,68 ----
          PGconn       *conn = connectToServer(&old_cluster, active_db->db_name);

          /*
!          * Fetch all libraries referenced in this DB.
           */
          ress[dbnum] = executeQueryOrDie(conn,
                                          "SELECT DISTINCT probin "
*************** get_loadable_libraries(void)
*** 69,76 ****

              res = executeQueryOrDie(conn,
                                      "SELECT 1 "
!                            "FROM    pg_catalog.pg_proc JOIN pg_namespace "
!                              "        ON pronamespace = pg_namespace.oid "
                                 "WHERE proname = 'plpython_call_handler' AND "
                                      "nspname = 'public' AND "
                                      "prolang = 13 /* C */ AND "
--- 90,98 ----

              res = executeQueryOrDie(conn,
                                      "SELECT 1 "
!                                     "FROM pg_catalog.pg_proc p "
!                                     "    JOIN pg_catalog.pg_namespace n "
!                                     "    ON pronamespace = n.oid "
                                 "WHERE proname = 'plpython_call_handler' AND "
                                      "nspname = 'public' AND "
                                      "prolang = 13 /* C */ AND "
*************** get_loadable_libraries(void)
*** 112,124 ****
      if (found_public_plpython_handler)
          pg_fatal("Remove the problem functions from the old cluster to continue.\n");

-     /* Allocate what's certainly enough space */
-     os_info.libraries = (char **) pg_malloc(totaltups * sizeof(char *));
-
      /*
!      * Now remove duplicates across DBs.  This is pretty inefficient code, but
!      * there probably aren't enough entries to matter.
       */
      totaltups = 0;

      for (dbnum = 0; dbnum < old_cluster.dbarr.ndbs; dbnum++)
--- 134,151 ----
      if (found_public_plpython_handler)
          pg_fatal("Remove the problem functions from the old cluster to continue.\n");

      /*
!      * Now we want to remove duplicates across DBs and sort the library names
!      * into order.  This avoids multiple probes of the same library, and
!      * ensures that libraries are probed in a consistent order, which is
!      * important for reproducible behavior if one library depends on another.
!      *
!      * First transfer all the names into one array, then sort, then remove
!      * duplicates.  Note: we strdup each name in the first loop so that we can
!      * safely clear the PGresults in the same loop.  This is a bit wasteful
!      * but it's unlikely there are enough names to matter.
       */
+     os_info.libraries = (char **) pg_malloc(totaltups * sizeof(char *));
      totaltups = 0;

      for (dbnum = 0; dbnum < old_cluster.dbarr.ndbs; dbnum++)
*************** get_loadable_libraries(void)
*** 131,157 ****
          for (rowno = 0; rowno < ntups; rowno++)
          {
              char       *lib = PQgetvalue(res, rowno, 0);
-             bool        dup = false;
-             int            n;

!             for (n = 0; n < totaltups; n++)
!             {
!                 if (strcmp(lib, os_info.libraries[n]) == 0)
!                 {
!                     dup = true;
!                     break;
!                 }
!             }
!             if (!dup)
!                 os_info.libraries[totaltups++] = pg_strdup(lib);
          }
-
          PQclear(res);
      }

-     os_info.num_libraries = totaltups;
-
      pg_free(ress);
  }


--- 158,191 ----
          for (rowno = 0; rowno < ntups; rowno++)
          {
              char       *lib = PQgetvalue(res, rowno, 0);

!             os_info.libraries[totaltups++] = pg_strdup(lib);
          }
          PQclear(res);
      }

      pg_free(ress);
+
+     if (totaltups > 1)
+     {
+         int            i,
+                     lastnondup;
+
+         qsort((void *) os_info.libraries, totaltups, sizeof(char *),
+               library_name_compare);
+
+         for (i = 1, lastnondup = 0; i < totaltups; i++)
+         {
+             if (strcmp(os_info.libraries[i],
+                        os_info.libraries[lastnondup]) != 0)
+                 os_info.libraries[++lastnondup] = os_info.libraries[i];
+             else
+                 pg_free(os_info.libraries[i]);
+         }
+         totaltups = lastnondup + 1;
+     }
+
+     os_info.num_libraries = totaltups;
  }