Date: Wed, 2 Oct 2019 12:23:37 -0500
From: Justin Pryzby <pryzby@telsasoft.com>
To: pgsql-hackers@lists.postgresql.org
Cc: Bruce Momjian <bruce@momjian.us>, Daniel Gustafsson <daniel@yesql.se>
Subject: format of pg_upgrade loadable_libraries warning
Message-ID: <20191002172337.GC9680@telsasoft.com>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="EVF5PPMfhYS0aIcm"
Content-Disposition: inline
User-Agent: Mutt/1.5.24 (2015-08-30)
Precedence: bulk


--EVF5PPMfhYS0aIcm
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline

Regarding the previous thread and commit here:
https://www.postgresql.org/message-id/flat/20180713162815.GA3835%40momjian.us
https://git.postgresql.org/gitweb/?p=postgresql.git;a=commitdiff;h=60e3bd1d7f92430b24b710ecf0559656eb8ed499

I'm suggesting to reformat the warning, which I found to be misleading:

|could not load library "$libdir/pgfincore": ERROR:  could not access file "$libdir/pgfincore": No such file or directory
|Database: postgres
|Database: too

To me that reads as "error message" followed by successful processing of two,
named database, and not "error message followed by list of databases for which
that error was experienced".  Essentially, the database names are themselves
the "error", and the message is a prefix indicating the library version; but
normally, error-looking things are output without a "prefix", since they
weren't anticipated.

The existing check is optimized to check each library once, but then outputs
each database which would try to load it.  That's an implementation detail, but
adds to confusion, since it shows a single error-looking thing which might
apply to multiple DBs (not obvious to me that it's associated with an DB at
all).  That leads me to believe that after I "DROP EXTENSION" once, I can
reasonably expect the upgrade to complete, which has a good chance of being
wrong, and is exactly what the patch was intended to avoid :(

To reproduce:

$ /usr/pgsql-11/bin/initdb -D ./pgtestlib11
$ /usr/pgsql-12/bin/initdb -D ./pgtestlib12
$ /usr/pgsql-11/bin/pg_ctl -D ./pgtestlib11 -o '-c port=5678 -c unix_socket_directories=/tmp' start
$ psql postgres -h /tmp -p 5678 -c 'CREATE EXTENSION pgfincore' -c 'CREATE DATABASE too'
$ psql too -h /tmp -p 5678 -c 'CREATE EXTENSION pgfincore'
$ /usr/pgsql-11/bin/pg_ctl -D ./pgtestlib11 stop
$ /usr/pgsql-12/bin/pg_upgrade -b /usr/pgsql-11/bin -B /usr/pgsql-12/bin -d ./pgtestlib11 -D pgtestlib12
$ cat loadable_libraries.txt 
Could not load library "$libdir/pgfincore": ERROR:  could not access file "$libdir/pgfincore": No such file or directory
Database: postgres
Database: too

I concede that the situation is clearer if there are multiple libraries causing
errors, especially in overlapping list of databases:

|[pryzbyj@database ~]$ cat loadable_libraries.txt 
|could not load library "$libdir/pg_repack": ERROR:  could not access file "$libdir/pg_repack": No such file or directory
|Database: postgres
|Database: too
|could not load library "$libdir/pgfincore": ERROR:  could not access file "$libdir/pgfincore": No such file or directory
|Database: postgres
|Database: too

I think the list of databases should be formatted to indicate its association
with the preceding error by indentation and verbage, or larger refactoring to
present in a list, like:
"Databases with library which failed to load: %s: %s",
	PQerrorMessage(conn), list_of_dbs_loading_that_lib

Justin

--EVF5PPMfhYS0aIcm
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
 filename="v1-0001-Indent-and-repeat-name-of-library-failing-to-load.patch"

From 513deb19f4741e717f73d0ba6163331107cfb616 Mon Sep 17 00:00:00 2001
From: Justin Pryzby <pryzbyj@telsasoft.com>
Date: Wed, 2 Oct 2019 12:08:37 -0500
Subject: [PATCH v1] Indent and repeat name of library failing to load

---
 src/bin/pg_upgrade/function.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/src/bin/pg_upgrade/function.c b/src/bin/pg_upgrade/function.c
index 0c66d1c..68d3634 100644
--- a/src/bin/pg_upgrade/function.c
+++ b/src/bin/pg_upgrade/function.c
@@ -255,9 +255,16 @@ check_loadable_libraries(void)
 			PQclear(res);
 		}
 
-		if (was_load_failure)
-			fprintf(script, _("Database: %s\n"),
+		if (was_load_failure) {
+			/*
+			 * Having probed that the library failed to load once,
+			 * assume that it won't load in any DB and output each
+			 * DB which would try to load it.
+			 */
+			fprintf(script, _(" - database loading %s: %s\n"),
+					lib,
 					old_cluster.dbarr.dbs[os_info.libraries[libnum].dbnum].db_name);
+		}
 	}
 
 	PQfinish(conn);
-- 
2.7.4


--EVF5PPMfhYS0aIcm--