pg.ddx.io pgsql-hackers@postgresql.org mailing list archive
help / color / mirror / Atom feedformat of pg_upgrade loadable_libraries warning
16+ messages / 5 participants
[nested] [flat]
* format of pg_upgrade loadable_libraries warning
@ 2019-10-02 17:23 Justin Pryzby <pryzby@telsasoft.com>
0 siblings, 1 reply; 16+ messages in thread
From: Justin Pryzby @ 2019-10-02 17:23 UTC (permalink / raw)
To: pgsql-hackers@lists.postgresql.org; +Cc: Bruce Momjian <bruce@momjian.us>; Daniel Gustafsson <daniel@yesql.se>
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=60e3bd1d7f92430b24b710ecf0559656e...
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
Attachments:
[text/x-diff] v1-0001-Indent-and-repeat-name-of-library-failing-to-load.patch (1018B, ../../20191002172337.GC9680@telsasoft.com/2-v1-0001-Indent-and-repeat-name-of-library-failing-to-load.patch)
download | inline diff:
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
^ permalink raw reply [nested|flat] 16+ messages in thread
* Re: format of pg_upgrade loadable_libraries warning
@ 2019-10-04 21:37 Bruce Momjian <bruce@momjian.us>
parent: Justin Pryzby <pryzby@telsasoft.com>
0 siblings, 1 reply; 16+ messages in thread
From: Bruce Momjian @ 2019-10-04 21:37 UTC (permalink / raw)
To: Justin Pryzby <pryzby@telsasoft.com>; +Cc: pgsql-hackers@lists.postgresql.org, Daniel Gustafsson <daniel@yesql.se>
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=60e3bd1d7f92430b24b710ecf0559656e...
>
> 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 <bruce@momjian.us> 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 +
Attachments:
[text/x-diff] error-found.diff (3.7K, ../../20191004213746.GD29227@momjian.us/2-error-found.diff)
download | inline diff:
diff --git a/src/bin/pg_upgrade/check.c b/src/bin/pg_upgrade/check.c
new file mode 100644
index 617270f..1bbb57c
*** a/src/bin/pg_upgrade/check.c
--- b/src/bin/pg_upgrade/check.c
*************** check_for_isn_and_int8_passing_mismatch(
*** 858,864 ****
output_path, strerror(errno));
if (!db_used)
{
! fprintf(script, "Database: %s\n", active_db->db_name);
db_used = true;
}
fprintf(script, " %s.%s\n",
--- 858,864 ----
output_path, strerror(errno));
if (!db_used)
{
! fprintf(script, "in database: %s\n", active_db->db_name);
db_used = true;
}
fprintf(script, " %s.%s\n",
*************** check_for_tables_with_oids(ClusterInfo *
*** 937,943 ****
output_path, strerror(errno));
if (!db_used)
{
! fprintf(script, "Database: %s\n", active_db->db_name);
db_used = true;
}
fprintf(script, " %s.%s\n",
--- 937,943 ----
output_path, strerror(errno));
if (!db_used)
{
! fprintf(script, "in database: %s\n", active_db->db_name);
db_used = true;
}
fprintf(script, " %s.%s\n",
*************** check_for_reg_data_type_usage(ClusterInf
*** 1046,1052 ****
output_path, strerror(errno));
if (!db_used)
{
! fprintf(script, "Database: %s\n", active_db->db_name);
db_used = true;
}
fprintf(script, " %s.%s.%s\n",
--- 1046,1052 ----
output_path, strerror(errno));
if (!db_used)
{
! fprintf(script, "in database: %s\n", active_db->db_name);
db_used = true;
}
fprintf(script, " %s.%s.%s\n",
*************** check_for_jsonb_9_4_usage(ClusterInfo *c
*** 1137,1143 ****
output_path, strerror(errno));
if (!db_used)
{
! fprintf(script, "Database: %s\n", active_db->db_name);
db_used = true;
}
fprintf(script, " %s.%s.%s\n",
--- 1137,1143 ----
output_path, strerror(errno));
if (!db_used)
{
! fprintf(script, "in database: %s\n", active_db->db_name);
db_used = true;
}
fprintf(script, " %s.%s.%s\n",
diff --git a/src/bin/pg_upgrade/function.c b/src/bin/pg_upgrade/function.c
new file mode 100644
index 0c66d1c..506fd34
*** a/src/bin/pg_upgrade/function.c
--- b/src/bin/pg_upgrade/function.c
*************** check_loadable_libraries(void)
*** 256,262 ****
}
if (was_load_failure)
! fprintf(script, _("Database: %s\n"),
old_cluster.dbarr.dbs[os_info.libraries[libnum].dbnum].db_name);
}
--- 256,262 ----
}
if (was_load_failure)
! fprintf(script, _("in database: %s\n"),
old_cluster.dbarr.dbs[os_info.libraries[libnum].dbnum].db_name);
}
diff --git a/src/bin/pg_upgrade/version.c b/src/bin/pg_upgrade/version.c
new file mode 100644
index 10cb362..fba7c6c
*** a/src/bin/pg_upgrade/version.c
--- b/src/bin/pg_upgrade/version.c
*************** old_9_3_check_for_line_data_type_usage(C
*** 157,163 ****
strerror(errno));
if (!db_used)
{
! fprintf(script, "Database: %s\n", active_db->db_name);
db_used = true;
}
fprintf(script, " %s.%s.%s\n",
--- 157,163 ----
strerror(errno));
if (!db_used)
{
! fprintf(script, "in database: %s\n", active_db->db_name);
db_used = true;
}
fprintf(script, " %s.%s.%s\n",
*************** old_9_6_check_for_unknown_data_type_usag
*** 258,264 ****
strerror(errno));
if (!db_used)
{
! fprintf(script, "Database: %s\n", active_db->db_name);
db_used = true;
}
fprintf(script, " %s.%s.%s\n",
--- 258,264 ----
strerror(errno));
if (!db_used)
{
! fprintf(script, "in database: %s\n", active_db->db_name);
db_used = true;
}
fprintf(script, " %s.%s.%s\n",
^ permalink raw reply [nested|flat] 16+ messages in thread
* Re: format of pg_upgrade loadable_libraries warning
@ 2019-10-04 22:40 Justin Pryzby <pryzby@telsasoft.com>
parent: Bruce Momjian <bruce@momjian.us>
0 siblings, 1 reply; 16+ messages in thread
From: Justin Pryzby @ 2019-10-04 22:40 UTC (permalink / raw)
To: Bruce Momjian <bruce@momjian.us>; +Cc: pgsql-hackers@lists.postgresql.org, Daniel Gustafsson <daniel@yesql.se>
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=60e3bd1d7f92430b24b710ecf0559656e...
> >
> > 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
^ permalink raw reply [nested|flat] 16+ messages in thread
* Re: format of pg_upgrade loadable_libraries warning
@ 2019-10-05 00:15 Bruce Momjian <bruce@momjian.us>
parent: Justin Pryzby <pryzby@telsasoft.com>
0 siblings, 1 reply; 16+ messages in thread
From: Bruce Momjian @ 2019-10-05 00:15 UTC (permalink / raw)
To: Justin Pryzby <pryzby@telsasoft.com>; +Cc: pgsql-hackers@lists.postgresql.org, Daniel Gustafsson <daniel@yesql.se>
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=60e3bd1d7f92430b24b710ecf0559656e...
> > >
> > > 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 <bruce@momjian.us> 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 +
^ permalink raw reply [nested|flat] 16+ messages in thread
* Re: format of pg_upgrade loadable_libraries warning
@ 2019-10-05 03:55 Tom Lane <tgl@sss.pgh.pa.us>
parent: Bruce Momjian <bruce@momjian.us>
0 siblings, 1 reply; 16+ messages in thread
From: Tom Lane @ 2019-10-05 03:55 UTC (permalink / raw)
To: Bruce Momjian <bruce@momjian.us>; +Cc: Justin Pryzby <pryzby@telsasoft.com>; pgsql-hackers@lists.postgresql.org, Daniel Gustafsson <daniel@yesql.se>
Bruce Momjian <bruce@momjian.us> 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
^ permalink raw reply [nested|flat] 16+ messages in thread
* Re: format of pg_upgrade loadable_libraries warning
@ 2019-10-07 17:47 Bruce Momjian <bruce@momjian.us>
parent: Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 2 replies; 16+ messages in thread
From: Bruce Momjian @ 2019-10-07 17:47 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: Justin Pryzby <pryzby@telsasoft.com>; pgsql-hackers@lists.postgresql.org, Daniel Gustafsson <daniel@yesql.se>
On Fri, Oct 4, 2019 at 11:55:21PM -0400, Tom Lane wrote:
> Bruce Momjian <bruce@momjian.us> 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 <bruce@momjian.us> 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 +
Attachments:
[text/x-diff] error-found.diff (3.7K, ../../20191007174757.GE4732@momjian.us/2-error-found.diff)
download | inline diff:
diff --git a/src/bin/pg_upgrade/check.c b/src/bin/pg_upgrade/check.c
new file mode 100644
index 617270f..e7bf48a
*** a/src/bin/pg_upgrade/check.c
--- b/src/bin/pg_upgrade/check.c
*************** check_for_isn_and_int8_passing_mismatch(
*** 858,864 ****
output_path, strerror(errno));
if (!db_used)
{
! fprintf(script, "Database: %s\n", active_db->db_name);
db_used = true;
}
fprintf(script, " %s.%s\n",
--- 858,864 ----
output_path, strerror(errno));
if (!db_used)
{
! fprintf(script, "In database: %s\n", active_db->db_name);
db_used = true;
}
fprintf(script, " %s.%s\n",
*************** check_for_tables_with_oids(ClusterInfo *
*** 937,943 ****
output_path, strerror(errno));
if (!db_used)
{
! fprintf(script, "Database: %s\n", active_db->db_name);
db_used = true;
}
fprintf(script, " %s.%s\n",
--- 937,943 ----
output_path, strerror(errno));
if (!db_used)
{
! fprintf(script, "In database: %s\n", active_db->db_name);
db_used = true;
}
fprintf(script, " %s.%s\n",
*************** check_for_reg_data_type_usage(ClusterInf
*** 1046,1052 ****
output_path, strerror(errno));
if (!db_used)
{
! fprintf(script, "Database: %s\n", active_db->db_name);
db_used = true;
}
fprintf(script, " %s.%s.%s\n",
--- 1046,1052 ----
output_path, strerror(errno));
if (!db_used)
{
! fprintf(script, "In database: %s\n", active_db->db_name);
db_used = true;
}
fprintf(script, " %s.%s.%s\n",
*************** check_for_jsonb_9_4_usage(ClusterInfo *c
*** 1137,1143 ****
output_path, strerror(errno));
if (!db_used)
{
! fprintf(script, "Database: %s\n", active_db->db_name);
db_used = true;
}
fprintf(script, " %s.%s.%s\n",
--- 1137,1143 ----
output_path, strerror(errno));
if (!db_used)
{
! fprintf(script, "In database: %s\n", active_db->db_name);
db_used = true;
}
fprintf(script, " %s.%s.%s\n",
diff --git a/src/bin/pg_upgrade/function.c b/src/bin/pg_upgrade/function.c
new file mode 100644
index 0c66d1c..3cbaab6
*** a/src/bin/pg_upgrade/function.c
--- b/src/bin/pg_upgrade/function.c
*************** check_loadable_libraries(void)
*** 256,262 ****
}
if (was_load_failure)
! fprintf(script, _("Database: %s\n"),
old_cluster.dbarr.dbs[os_info.libraries[libnum].dbnum].db_name);
}
--- 256,262 ----
}
if (was_load_failure)
! fprintf(script, _("In database: %s\n"),
old_cluster.dbarr.dbs[os_info.libraries[libnum].dbnum].db_name);
}
diff --git a/src/bin/pg_upgrade/version.c b/src/bin/pg_upgrade/version.c
new file mode 100644
index 10cb362..42f1ce7
*** a/src/bin/pg_upgrade/version.c
--- b/src/bin/pg_upgrade/version.c
*************** old_9_3_check_for_line_data_type_usage(C
*** 157,163 ****
strerror(errno));
if (!db_used)
{
! fprintf(script, "Database: %s\n", active_db->db_name);
db_used = true;
}
fprintf(script, " %s.%s.%s\n",
--- 157,163 ----
strerror(errno));
if (!db_used)
{
! fprintf(script, "In database: %s\n", active_db->db_name);
db_used = true;
}
fprintf(script, " %s.%s.%s\n",
*************** old_9_6_check_for_unknown_data_type_usag
*** 258,264 ****
strerror(errno));
if (!db_used)
{
! fprintf(script, "Database: %s\n", active_db->db_name);
db_used = true;
}
fprintf(script, " %s.%s.%s\n",
--- 258,264 ----
strerror(errno));
if (!db_used)
{
! fprintf(script, "In database: %s\n", active_db->db_name);
db_used = true;
}
fprintf(script, " %s.%s.%s\n",
^ permalink raw reply [nested|flat] 16+ messages in thread
* Re: format of pg_upgrade loadable_libraries warning
@ 2019-10-09 02:17 Bruce Momjian <bruce@momjian.us>
parent: Bruce Momjian <bruce@momjian.us>
1 sibling, 0 replies; 16+ messages in thread
From: Bruce Momjian @ 2019-10-09 02:17 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: Justin Pryzby <pryzby@telsasoft.com>; pgsql-hackers@lists.postgresql.org, Daniel Gustafsson <daniel@yesql.se>
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 <bruce@momjian.us> 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 <bruce@momjian.us> 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 +
^ permalink raw reply [nested|flat] 16+ messages in thread
* Re: format of pg_upgrade loadable_libraries warning
@ 2019-11-14 19:06 Alvaro Herrera <alvherre@2ndquadrant.com>
parent: Bruce Momjian <bruce@momjian.us>
1 sibling, 2 replies; 16+ messages in thread
From: Alvaro Herrera @ 2019-11-14 19:06 UTC (permalink / raw)
To: Bruce Momjian <bruce@momjian.us>; +Cc: Tom Lane <tgl@sss.pgh.pa.us>; Justin Pryzby <pryzby@telsasoft.com>; pgsql-hackers@lists.postgresql.org, Daniel Gustafsson <daniel@yesql.se>
On 2019-Oct-07, Bruce Momjian wrote:
> 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?
pg_fatal is a "gettext trigger" (see nls.mk), so that part of the
message is definitely translated. And the fprintf format string should
be decorated with _() in order to make translatable too; otherwise the
message is only half-translated when it appears in the pg_upgrade log,
which is not nice. This should look like:
if (!db_used)
{
/* translator: This is an error message indicator */
fprintf(script, _("In database: %s\n"), active_db->db_name);
db_used = true;
}
fprintf(script, " %s.%s\n",
BTW, how is one supposed to "manually upgrade databases that use
contrib/isb"? This part is not very clear.
--
Álvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
^ permalink raw reply [nested|flat] 16+ messages in thread
* Re: format of pg_upgrade loadable_libraries warning
@ 2019-11-14 19:46 Tom Lane <tgl@sss.pgh.pa.us>
parent: Alvaro Herrera <alvherre@2ndquadrant.com>
1 sibling, 1 reply; 16+ messages in thread
From: Tom Lane @ 2019-11-14 19:46 UTC (permalink / raw)
To: Alvaro Herrera <alvherre@2ndquadrant.com>; +Cc: Bruce Momjian <bruce@momjian.us>; Justin Pryzby <pryzby@telsasoft.com>; pgsql-hackers@lists.postgresql.org, Daniel Gustafsson <daniel@yesql.se>
Alvaro Herrera <alvherre@2ndquadrant.com> writes:
> On 2019-Oct-07, Bruce Momjian wrote:
>> 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?
> pg_fatal is a "gettext trigger" (see nls.mk), so that part of the
> message is definitely translated.
Right, but Bruce's point is that what goes into the separate output
file listing problem cases is not translated, and never has been.
Maybe we should start doing so, but that would be a distinct issue.
I'm not really sure that we should translate it, anyway --- could
there be anyone out there who is using tools to process these files?
> BTW, how is one supposed to "manually upgrade databases that use
> contrib/isb"? This part is not very clear.
Agreed, the pg_fatal message is claiming that you can do something
without really providing any concrete instructions for it. I'm not
sure that that's helpful.
regards, tom lane
^ permalink raw reply [nested|flat] 16+ messages in thread
* Re: format of pg_upgrade loadable_libraries warning
@ 2019-11-14 22:16 Bruce Momjian <bruce@momjian.us>
parent: Alvaro Herrera <alvherre@2ndquadrant.com>
1 sibling, 2 replies; 16+ messages in thread
From: Bruce Momjian @ 2019-11-14 22:16 UTC (permalink / raw)
To: Alvaro Herrera <alvherre@2ndquadrant.com>; +Cc: Tom Lane <tgl@sss.pgh.pa.us>; Justin Pryzby <pryzby@telsasoft.com>; pgsql-hackers@lists.postgresql.org, Daniel Gustafsson <daniel@yesql.se>
On Thu, Nov 14, 2019 at 04:06:52PM -0300, Alvaro Herrera wrote:
> BTW, how is one supposed to "manually upgrade databases that use
> contrib/isb"? This part is not very clear.
I thought you would dump out databases that use isn, drop those
databases, use pg_upgrade for the remaining databases, then load the
dumped database. Attached is a patch that improves the wording.
--
Bruce Momjian <bruce@momjian.us> 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 +
Attachments:
[text/x-diff] isn.diff (966B, ../../20191114221641.GD6160@momjian.us/2-isn.diff)
download | inline diff:
diff --git a/src/bin/pg_upgrade/check.c b/src/bin/pg_upgrade/check.c
index ff7057db73..e5f1c64799 100644
--- a/src/bin/pg_upgrade/check.c
+++ b/src/bin/pg_upgrade/check.c
@@ -887,9 +887,9 @@ check_for_isn_and_int8_passing_mismatch(ClusterInfo *cluster)
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"
+ "manually dump databases that use \"contrib/isn\" facilities in the old\n"
+ "cluster, drop them, restart the upgrade, and restore them laster. A\n"
+ "list of the problem functions is in the file:\n"
" %s\n\n", output_path);
}
else
^ permalink raw reply [nested|flat] 16+ messages in thread
* Re: format of pg_upgrade loadable_libraries warning
@ 2019-11-14 22:17 Bruce Momjian <bruce@momjian.us>
parent: Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 0 replies; 16+ messages in thread
From: Bruce Momjian @ 2019-11-14 22:17 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: Alvaro Herrera <alvherre@2ndquadrant.com>; Justin Pryzby <pryzby@telsasoft.com>; pgsql-hackers@lists.postgresql.org, Daniel Gustafsson <daniel@yesql.se>
On Thu, Nov 14, 2019 at 02:46:29PM -0500, Tom Lane wrote:
> Alvaro Herrera <alvherre@2ndquadrant.com> writes:
> > On 2019-Oct-07, Bruce Momjian wrote:
> >> 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?
>
> > pg_fatal is a "gettext trigger" (see nls.mk), so that part of the
> > message is definitely translated.
>
> Right, but Bruce's point is that what goes into the separate output
> file listing problem cases is not translated, and never has been.
> Maybe we should start doing so, but that would be a distinct issue.
> I'm not really sure that we should translate it, anyway --- could
> there be anyone out there who is using tools to process these files?
Yes, we are lacking in all these output files so if we do one, we should
do them all.
--
Bruce Momjian <bruce@momjian.us> 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 +
^ permalink raw reply [nested|flat] 16+ messages in thread
* Re: format of pg_upgrade loadable_libraries warning
@ 2019-11-14 22:49 Tom Lane <tgl@sss.pgh.pa.us>
parent: Bruce Momjian <bruce@momjian.us>
1 sibling, 1 reply; 16+ messages in thread
From: Tom Lane @ 2019-11-14 22:49 UTC (permalink / raw)
To: Bruce Momjian <bruce@momjian.us>; +Cc: Alvaro Herrera <alvherre@2ndquadrant.com>; Justin Pryzby <pryzby@telsasoft.com>; pgsql-hackers@lists.postgresql.org, Daniel Gustafsson <daniel@yesql.se>
Bruce Momjian <bruce@momjian.us> writes:
> On Thu, Nov 14, 2019 at 04:06:52PM -0300, Alvaro Herrera wrote:
>> BTW, how is one supposed to "manually upgrade databases that use
>> contrib/isb"? This part is not very clear.
> I thought you would dump out databases that use isn, drop those
> databases, use pg_upgrade for the remaining databases, then load the
> dumped database. Attached is a patch that improves the wording.
That's better wording, but do we need similar for any of the other
not-supported checks?
regards, tom lane
^ permalink raw reply [nested|flat] 16+ messages in thread
* Re: format of pg_upgrade loadable_libraries warning
@ 2019-11-14 23:00 Bruce Momjian <bruce@momjian.us>
parent: Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 0 replies; 16+ messages in thread
From: Bruce Momjian @ 2019-11-14 23:00 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: Alvaro Herrera <alvherre@2ndquadrant.com>; Justin Pryzby <pryzby@telsasoft.com>; pgsql-hackers@lists.postgresql.org, Daniel Gustafsson <daniel@yesql.se>
On Thu, Nov 14, 2019 at 05:49:12PM -0500, Tom Lane wrote:
> Bruce Momjian <bruce@momjian.us> writes:
> > On Thu, Nov 14, 2019 at 04:06:52PM -0300, Alvaro Herrera wrote:
> >> BTW, how is one supposed to "manually upgrade databases that use
> >> contrib/isb"? This part is not very clear.
>
> > I thought you would dump out databases that use isn, drop those
> > databases, use pg_upgrade for the remaining databases, then load the
> > dumped database. Attached is a patch that improves the wording.
>
> That's better wording, but do we need similar for any of the other
> not-supported checks?
I don't think so. The other checks are checking for the _use_ of
certain things in user objects, and the user objects can be dropped,
while this check checks for the existence of _functions_ from an
extension that must be uninstalled. I assume the extension can be
uninstalled in the database, but I assume something uses it and that
telling people to find all the users of the extension then dropping it
is too complex to describe.
--
Bruce Momjian <bruce@momjian.us> 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 +
^ permalink raw reply [nested|flat] 16+ messages in thread
* Re: format of pg_upgrade loadable_libraries warning
@ 2019-11-14 23:32 Daniel Gustafsson <daniel@yesql.se>
parent: Bruce Momjian <bruce@momjian.us>
1 sibling, 1 reply; 16+ messages in thread
From: Daniel Gustafsson @ 2019-11-14 23:32 UTC (permalink / raw)
To: Bruce Momjian <bruce@momjian.us>; +Cc: Alvaro Herrera <alvherre@2ndquadrant.com>; Tom Lane <tgl@sss.pgh.pa.us>; Justin Pryzby <pryzby@telsasoft.com>; pgsql-hackers@lists.postgresql.org
> On 14 Nov 2019, at 23:16, Bruce Momjian <bruce@momjian.us> wrote:
>
> On Thu, Nov 14, 2019 at 04:06:52PM -0300, Alvaro Herrera wrote:
>> BTW, how is one supposed to "manually upgrade databases that use
>> contrib/isb"? This part is not very clear.
>
> I thought you would dump out databases that use isn, drop those
> databases, use pg_upgrade for the remaining databases, then load the
> dumped database. Attached is a patch that improves the wording.
I agree with this patch, that's much a more informative message.
There is one tiny typo in the patch: s/laster/later/
+ "cluster, drop them, restart the upgrade, and restore them laster. A\n"
cheers ./daniel
^ permalink raw reply [nested|flat] 16+ messages in thread
* Re: format of pg_upgrade loadable_libraries warning
@ 2019-11-28 01:26 Bruce Momjian <bruce@momjian.us>
parent: Daniel Gustafsson <daniel@yesql.se>
0 siblings, 1 reply; 16+ messages in thread
From: Bruce Momjian @ 2019-11-28 01:26 UTC (permalink / raw)
To: Daniel Gustafsson <daniel@yesql.se>; +Cc: Alvaro Herrera <alvherre@2ndquadrant.com>; Tom Lane <tgl@sss.pgh.pa.us>; Justin Pryzby <pryzby@telsasoft.com>; pgsql-hackers@lists.postgresql.org
On Fri, Nov 15, 2019 at 12:32:55AM +0100, Daniel Gustafsson wrote:
> > On 14 Nov 2019, at 23:16, Bruce Momjian <bruce@momjian.us> wrote:
> >
> > On Thu, Nov 14, 2019 at 04:06:52PM -0300, Alvaro Herrera wrote:
> >> BTW, how is one supposed to "manually upgrade databases that use
> >> contrib/isb"? This part is not very clear.
> >
> > I thought you would dump out databases that use isn, drop those
> > databases, use pg_upgrade for the remaining databases, then load the
> > dumped database. Attached is a patch that improves the wording.
>
> I agree with this patch, that's much a more informative message.
>
> There is one tiny typo in the patch: s/laster/later/
>
> + "cluster, drop them, restart the upgrade, and restore them laster. A\n"
I have applied the patch, with improved wording. I only applied this to
PG 13 since I was worried old tools might be checking for the old error
text. Should this be backpatched more?
--
Bruce Momjian <bruce@momjian.us> 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 +
^ permalink raw reply [nested|flat] 16+ messages in thread
* Re: format of pg_upgrade loadable_libraries warning
@ 2019-11-28 08:58 Daniel Gustafsson <daniel@yesql.se>
parent: Bruce Momjian <bruce@momjian.us>
0 siblings, 0 replies; 16+ messages in thread
From: Daniel Gustafsson @ 2019-11-28 08:58 UTC (permalink / raw)
To: Bruce Momjian <bruce@momjian.us>; +Cc: Alvaro Herrera <alvherre@2ndquadrant.com>; Tom Lane <tgl@sss.pgh.pa.us>; Justin Pryzby <pryzby@telsasoft.com>; pgsql-hackers@lists.postgresql.org
> On 28 Nov 2019, at 02:26, Bruce Momjian <bruce@momjian.us> wrote:
> I have applied the patch, with improved wording. I only applied this to
> PG 13 since I was worried old tools might be checking for the old error
> text. Should this be backpatched more?
I don't think it's unreasonable to assume that there are lots of inhouse
tooling for pg_upgrade orchestration which grep for specific messages.
Stopping at 13 seems perfectly reasonable for this change.
cheers ./daniel
^ permalink raw reply [nested|flat] 16+ messages in thread
end of thread, other threads:[~2019-11-28 08:58 UTC | newest]
Thread overview: 16+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2019-10-02 17:23 format of pg_upgrade loadable_libraries warning Justin Pryzby <pryzby@telsasoft.com>
2019-10-04 21:37 ` Bruce Momjian <bruce@momjian.us>
2019-10-04 22:40 ` Justin Pryzby <pryzby@telsasoft.com>
2019-10-05 00:15 ` Bruce Momjian <bruce@momjian.us>
2019-10-05 03:55 ` Tom Lane <tgl@sss.pgh.pa.us>
2019-10-07 17:47 ` Bruce Momjian <bruce@momjian.us>
2019-10-09 02:17 ` Bruce Momjian <bruce@momjian.us>
2019-11-14 19:06 ` Alvaro Herrera <alvherre@2ndquadrant.com>
2019-11-14 19:46 ` Tom Lane <tgl@sss.pgh.pa.us>
2019-11-14 22:17 ` Bruce Momjian <bruce@momjian.us>
2019-11-14 22:16 ` Bruce Momjian <bruce@momjian.us>
2019-11-14 22:49 ` Tom Lane <tgl@sss.pgh.pa.us>
2019-11-14 23:00 ` Bruce Momjian <bruce@momjian.us>
2019-11-14 23:32 ` Daniel Gustafsson <daniel@yesql.se>
2019-11-28 01:26 ` Bruce Momjian <bruce@momjian.us>
2019-11-28 08:58 ` Daniel Gustafsson <daniel@yesql.se>
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox