format of pg_upgrade loadable_libraries warning

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
7 messages Options
Reply | Threaded
Open this post in threaded view
|

format of pg_upgrade loadable_libraries warning

Justin Pryzby
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

v1-0001-Indent-and-repeat-name-of-library-failing-to-load.patch (1K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: format of pg_upgrade loadable_libraries warning

Bruce Momjian
On Wed, Oct  2, 2019 at 12:23:37PM -0500, Justin Pryzby wrote:

> 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 :(
Understood.  This is a general problem with the way pg_upgrade displays
errors and the databases/objects associated with them.  The attached
patch fixes the output text to say "in database", e.g.:

  Could not load library "$libdir/pgfincore": ERROR:  could not access file "$libdir/pgfincore": No such file or directory
  in database: postgres
  in database: too

Would intenting help too?  I am inclined to fix this only head, and not
to backpatch the change.

--
  Bruce Momjian  <[hidden email]>        http://momjian.us
  EnterpriseDB                             http://enterprisedb.com

+ As you are, so once was I.  As I am, so you will be. +
+                      Ancient Roman grave inscription +

error-found.diff (3K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: format of pg_upgrade loadable_libraries warning

Justin Pryzby
On Fri, Oct 04, 2019 at 05:37:46PM -0400, Bruce Momjian wrote:

> On Wed, Oct  2, 2019 at 12:23:37PM -0500, Justin Pryzby wrote:
> > 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:
>
> Understood.  This is a general problem with the way pg_upgrade displays
> errors and the databases/objects associated with them.  The attached
> patch fixes the output text to say "in database", e.g.:
>
>   Could not load library "$libdir/pgfincore": ERROR:  could not access file "$libdir/pgfincore": No such file or directory
>   in database: postgres
>   in database: too
>
> Would intenting help too?  I am inclined to fix this only head, and not
> to backpatch the change.

Yes, indenting would also help.

I would argue to include in 12.1, since 12 is what most everyone will use for
upgrades, and patch for .1 will help people upgrading for 11 of the next 12
months.  (But, your patch is more general than mine).

Thanks,
Justin


Reply | Threaded
Open this post in threaded view
|

Re: format of pg_upgrade loadable_libraries warning

Bruce Momjian
On Fri, Oct  4, 2019 at 05:40:08PM -0500, Justin Pryzby wrote:

> On Fri, Oct 04, 2019 at 05:37:46PM -0400, Bruce Momjian wrote:
> > On Wed, Oct  2, 2019 at 12:23:37PM -0500, Justin Pryzby wrote:
> > > 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:
> >
> > Understood.  This is a general problem with the way pg_upgrade displays
> > errors and the databases/objects associated with them.  The attached
> > patch fixes the output text to say "in database", e.g.:
> >
> >   Could not load library "$libdir/pgfincore": ERROR:  could not access file "$libdir/pgfincore": No such file or directory
> >   in database: postgres
> >   in database: too
> >
> > Would intenting help too?  I am inclined to fix this only head, and not
> > to backpatch the change.
>
> Yes, indenting would also help.
>
> I would argue to include in 12.1, since 12 is what most everyone will use for
> upgrades, and patch for .1 will help people upgrading for 11 of the next 12
> months.  (But, your patch is more general than mine).

No, there might be tools that depend on the existing format, and this is
the first report of confusion I have read.

--
  Bruce Momjian  <[hidden email]>        http://momjian.us
  EnterpriseDB                             http://enterprisedb.com

+ As you are, so once was I.  As I am, so you will be. +
+                      Ancient Roman grave inscription +


Reply | Threaded
Open this post in threaded view
|

Re: format of pg_upgrade loadable_libraries warning

Tom Lane-2
Bruce Momjian <[hidden email]> writes:
> On Fri, Oct  4, 2019 at 05:40:08PM -0500, Justin Pryzby wrote:
>> I would argue to include in 12.1, since 12 is what most everyone will use for
>> upgrades, and patch for .1 will help people upgrading for 11 of the next 12
>> months.  (But, your patch is more general than mine).

> No, there might be tools that depend on the existing format, and this is
> the first report of confusion I have read.

Translations will also lag behind any such change.  Speaking of which,
it might be a good idea to include some translator: annotations to
help translators understand what context these fragmentary phrases
are used in.  I'd actually say that my biggest concern with these
messages is whether they can translate into something sane.

                        regards, tom lane


Reply | Threaded
Open this post in threaded view
|

Re: format of pg_upgrade loadable_libraries warning

Bruce Momjian
On Fri, Oct  4, 2019 at 11:55:21PM -0400, Tom Lane wrote:

> Bruce Momjian <[hidden email]> writes:
> > On Fri, Oct  4, 2019 at 05:40:08PM -0500, Justin Pryzby wrote:
> >> I would argue to include in 12.1, since 12 is what most everyone will use for
> >> upgrades, and patch for .1 will help people upgrading for 11 of the next 12
> >> months.  (But, your patch is more general than mine).
>
> > No, there might be tools that depend on the existing format, and this is
> > the first report of confusion I have read.
>
> Translations will also lag behind any such change.  Speaking of which,
> it might be a good idea to include some translator: annotations to
> help translators understand what context these fragmentary phrases
> are used in.  I'd actually say that my biggest concern with these
> messages is whether they can translate into something sane.
Uh, I looked at the pg_ugprade code and the error message is:

        pg_fatal("Your installation contains \"contrib/isn\" functions which rely on the\n"
                 "bigint data type.  Your old and new clusters pass bigint values\n"
                 "differently so this cluster cannot currently be upgraded.  You can\n"
                 "manually upgrade databases that use \"contrib/isn\" facilities and remove\n"
                 "\"contrib/isn\" from the old cluster and restart the upgrade.  A list of\n"
                 "the problem functions is in the file:\n"
                 "    %s\n\n", output_path);

and the "in database" (which I have changed to capitalized "In database"
in the attached patch), looks like:

               fprintf(script, "In database: %s\n", active_db->db_name);

meaning it _isn't_ an output error message, but rather something that
appears in an error file.  I don't think either of these are translated.
Is that wrong?

--
  Bruce Momjian  <[hidden email]>        http://momjian.us
  EnterpriseDB                             http://enterprisedb.com

+ As you are, so once was I.  As I am, so you will be. +
+                      Ancient Roman grave inscription +

error-found.diff (3K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: format of pg_upgrade loadable_libraries warning

Bruce Momjian
On Mon, Oct  7, 2019 at 01:47:57PM -0400, Bruce Momjian wrote:

> On Fri, Oct  4, 2019 at 11:55:21PM -0400, Tom Lane wrote:
> > Bruce Momjian <[hidden email]> writes:
> > > On Fri, Oct  4, 2019 at 05:40:08PM -0500, Justin Pryzby wrote:
> > >> I would argue to include in 12.1, since 12 is what most everyone will use for
> > >> upgrades, and patch for .1 will help people upgrading for 11 of the next 12
> > >> months.  (But, your patch is more general than mine).
> >
> > > No, there might be tools that depend on the existing format, and this is
> > > the first report of confusion I have read.
> >
> > Translations will also lag behind any such change.  Speaking of which,
> > it might be a good idea to include some translator: annotations to
> > help translators understand what context these fragmentary phrases
> > are used in.  I'd actually say that my biggest concern with these
> > messages is whether they can translate into something sane.
>
> Uh, I looked at the pg_ugprade code and the error message is:
>
>         pg_fatal("Your installation contains \"contrib/isn\" functions which rely on the\n"
>                  "bigint data type.  Your old and new clusters pass bigint values\n"
>                  "differently so this cluster cannot currently be upgraded.  You can\n"
>                  "manually upgrade databases that use \"contrib/isn\" facilities and remove\n"
>                  "\"contrib/isn\" from the old cluster and restart the upgrade.  A list of\n"
>                  "the problem functions is in the file:\n"
>                  "    %s\n\n", output_path);
>
> and the "in database" (which I have changed to capitalized "In database"
> in the attached patch), looks like:
>
>                fprintf(script, "In database: %s\n", active_db->db_name);
>
> meaning it _isn't_ an output error message, but rather something that
> appears in an error file.  I don't think either of these are translated.
> Is that wrong?

Patch applied to head.

--
  Bruce Momjian  <[hidden email]>        http://momjian.us
  EnterpriseDB                             http://enterprisedb.com

+ As you are, so once was I.  As I am, so you will be. +
+                      Ancient Roman grave inscription +