[PATCH] improve the pg_upgrade error message

Started by Jeevan Ladheover 4 years ago11 messages

Jeevan Ladhe

jeevan.ladhe@enterprisedb.com

over 4 years ago

1 attachment(s)

While looking into one of the pg_upgrade issue, I found it

challenging to find out the database that has the datallowconn set to

'false' that was throwing following error:

*"All non-template0 databases must allow connections, i.e. their
pg_database.datallowconn must be true"*

edb=# create database mydb;

CREATE DATABASE

edb=# update pg_database set datallowconn='false' where datname like 'mydb';

UPDATE 1

Now, when I try to upgrade the server, without the patch we get above

error, which leaves no clue behind about which database has datallowconn

set to 'false'. It can be argued that we can query the pg_database

catalog and find that out easily, but at times it is challenging to get

that from the customer environment. But, anyways I feel we have scope to

improve the error message here per the attached patch.

With attached patch, now I get following error:

*"All non-template0 databases must allow connections, i.e. their
pg_database.datallowconn must be true; database "mydb" has datallowconn set
to false."*

Regards,

Jeevan Ladhe

Attachments:

0001-Improve-the-pg_upgrade-error-message.patchapplication/octet-stream; name=0001-Improve-the-pg_upgrade-error-message.patchDownload

From e675f8f528a6b3ff6e5681398f46d9830b01adff Mon Sep 17 00:00:00 2001
From: Jeevan Ladhe <jeevan.ladhe@enterprisedb.com>
Date: Mon, 12 Jul 2021 16:54:13 +0530
Subject: [PATCH] Improve the pg_upgrade error message.

---
 src/bin/pg_upgrade/check.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/src/bin/pg_upgrade/check.c b/src/bin/pg_upgrade/check.c
index 0c47a6b8cc..68361d8d84 100644
--- a/src/bin/pg_upgrade/check.c
+++ b/src/bin/pg_upgrade/check.c
@@ -734,7 +734,8 @@ check_proper_datallowconn(ClusterInfo *cluster)
 			 */
 			if (strcmp(datallowconn, "f") == 0)
 				pg_fatal("All non-template0 databases must allow connections, "
-						 "i.e. their pg_database.datallowconn must be true\n");
+						 "i.e. their pg_database.datallowconn must be true; "
+						 "database \"%s\" has datallowconn set to false.\n", datname);
 		}
 	}
 
-- 
2.25.1

Laurenz Albe

laurenz.albe@cybertec.at

over 4 years ago

In reply to: Jeevan Ladhe (#1)

Re: [PATCH] improve the pg_upgrade error message

On Mon, 2021-07-12 at 16:58 +0530, Jeevan Ladhe wrote:

While looking into one of the pg_upgrade issue, I found it
challenging to find out the database that has the datallowconn set to
'false' that was throwing following error:

"All non-template0 databases must allow connections, i.e. their pg_database.datallowconn must be true"

It can be argued that we can query the pg_database
catalog and find that out easily, but at times it is challenging to get
that from the customer environment.

With attached patch, now I get following error:
"All non-template0 databases must allow connections, i.e. their pg_database.datallowconn must be true; database "mydb" has datallowconn set to false."

I am in favor of that in principle, but I think that additional information
should be separate line.

Yours,
Laurenz Albe

Suraj Kharage

suraj.kharage@enterprisedb.com

over 4 years ago

In reply to: Jeevan Ladhe (#1)

Re: [PATCH] improve the pg_upgrade error message

+1 for the change. Patch looks good to me.

On Mon, Jul 12, 2021 at 4:59 PM Jeevan Ladhe <jeevan.ladhe@enterprisedb.com>
wrote:

While looking into one of the pg_upgrade issue, I found it

challenging to find out the database that has the datallowconn set to

'false' that was throwing following error:

*"All non-template0 databases must allow connections, i.e. their
pg_database.datallowconn must be true"*

edb=# create database mydb;

CREATE DATABASE

edb=# update pg_database set datallowconn='false' where datname like
'mydb';

UPDATE 1

Now, when I try to upgrade the server, without the patch we get above

error, which leaves no clue behind about which database has datallowconn

set to 'false'. It can be argued that we can query the pg_database

catalog and find that out easily, but at times it is challenging to get

that from the customer environment. But, anyways I feel we have scope to

improve the error message here per the attached patch.

With attached patch, now I get following error:

*"All non-template0 databases must allow connections, i.e. their
pg_database.datallowconn must be true; database "mydb" has datallowconn set
to false."*

Regards,

Jeevan Ladhe

--
--

Thanks & Regards,
Suraj kharage,

edbpostgres.com

Justin Pryzby

pryzby@telsasoft.com

over 4 years ago

In reply to: Laurenz Albe (#2)

Re: [PATCH] improve the pg_upgrade error message

On Mon, Jul 12, 2021 at 02:06:31PM +0200, Laurenz Albe wrote:

On Mon, 2021-07-12 at 16:58 +0530, Jeevan Ladhe wrote:

While looking into one of the pg_upgrade issue, I found it
challenging to find out the database that has the datallowconn set to
'false' that was throwing following error:

"All non-template0 databases must allow connections, i.e. their pg_database.datallowconn must be true"

It can be argued that we can query the pg_database
catalog and find that out easily, but at times it is challenging to get
that from the customer environment.

With attached patch, now I get following error:
"All non-template0 databases must allow connections, i.e. their pg_database.datallowconn must be true; database "mydb" has datallowconn set to false."

I am in favor of that in principle, but I think that additional information
should be separate line.

I think the style for this kind of error is established by commit 1634d3615.
This shows "In database: ..."

More importantly, if you're going to show the name of the problematic DB, you
should show the name of *all* the problem DBs. Otherwise it gives the
impression the upgrade will progress if you fix just that one.

The admin might fix DB123, restart their upgrade procedure, spend 5 or 15
minutes with that, only to have it then fail on DB1234.

--
Justin

Jeevan Ladhe

jeevan.ladhe@enterprisedb.com

over 4 years ago

In reply to: Justin Pryzby (#4)

1 attachment(s)

Re: [PATCH] improve the pg_upgrade error message

The admin might fix DB123, restart their upgrade procedure, spend 5 or 15
minutes with that, only to have it then fail on DB1234.

Agree with this observation.

Here is a patch that writes the list of all the databases other than
template0
that are having their pg_database.datallowconn to false in a file. Similar
approach is seen in other functions like check_for_data_types_usage(),
check_for_data_types_usage() etc. Thanks Suraj Kharage for the offline
suggestion.

PFA patch.

For experiment, here is how it turns out after the fix.

postgres=# update pg_database set datallowconn='false' where datname in
('mydb', 'mydb1', 'mydb2');
UPDATE 3

$ pg_upgrade -d /tmp/v96/data -D /tmp/v13/data -b $HOME/v96/install/bin -B
$HOME/v13/install/bin
Performing Consistency Checks
-----------------------------
Checking cluster versions ok
Checking database user is the install user ok
Checking database connection settings fatal

All non-template0 databases must allow connections, i.e. their
pg_database.datallowconn must be true. Your installation contains
non-template0 databases with their pg_database.datallowconn set to
false. Consider allowing connection for all non-template0 databases
using:
UPDATE pg_catalog.pg_database SET datallowconn='true' WHERE datname NOT
LIKE 'template0';
A list of databases with the problem is given in the file:
databases_with_datallowconn_false.txt

Failure, exiting

$ cat databases_with_datallowconn_false.txt
mydb
mydb1
mydb2

Regards,
Jeevan Ladhe

Attachments:

v2-0001-Improve-the-pg_upgrade-error-message.patchapplication/octet-stream; name=v2-0001-Improve-the-pg_upgrade-error-message.patchDownload

From ce03fb3489f6683da8c0ace7c33b5a95b2f399ae Mon Sep 17 00:00:00 2001
From: Jeevan Ladhe <jeevan.ladhe@enterprisedb.com>
Date: Tue, 13 Jul 2021 18:52:38 +0530
Subject: [PATCH] Improve the pg_upgrade error message.

---
 src/bin/pg_upgrade/check.c | 30 +++++++++++++++++++++++++++---
 1 file changed, 27 insertions(+), 3 deletions(-)

diff --git a/src/bin/pg_upgrade/check.c b/src/bin/pg_upgrade/check.c
index 0c47a6b8cc..e6b52aa3e8 100644
--- a/src/bin/pg_upgrade/check.c
+++ b/src/bin/pg_upgrade/check.c
@@ -700,9 +700,13 @@ check_proper_datallowconn(ClusterInfo *cluster)
 	int			ntups;
 	int			i_datname;
 	int			i_datallowconn;
+	FILE	   *script = NULL;
+	char		output_path[MAXPGPATH];
 
 	prep_status("Checking database connection settings");
 
+	snprintf(output_path, sizeof(output_path), "databases_with_datallowconn_false.txt");
+
 	conn_template1 = connectToServer(cluster, "template1");
 
 	/* get database names */
@@ -733,8 +737,13 @@ check_proper_datallowconn(ClusterInfo *cluster)
 			 * restore
 			 */
 			if (strcmp(datallowconn, "f") == 0)
-				pg_fatal("All non-template0 databases must allow connections, "
-						 "i.e. their pg_database.datallowconn must be true\n");
+			{
+				if (script == NULL && (script = fopen_priv(output_path, "w")) == NULL)
+					pg_fatal("could not open file \"%s\": %s\n",
+							 output_path, strerror(errno));
+
+				fprintf(script, "%s\n", datname);
+			}
 		}
 	}
 
@@ -742,7 +751,22 @@ check_proper_datallowconn(ClusterInfo *cluster)
 
 	PQfinish(conn_template1);
 
-	check_ok();
+	if (script)
+	{
+		fclose(script);
+
+		pg_log(PG_REPORT, "fatal\n");
+		pg_fatal("All non-template0 databases must allow connections, i.e. their\n"
+				 "pg_database.datallowconn must be true. Your installation contains\n"
+				 "non-template0 databases with their pg_database.datallowconn set to\n"
+				 "false. Consider allowing connection for all non-template0 databases\n"
+				 "using:\n"
+				 "    UPDATE pg_catalog.pg_database SET datallowconn='true' WHERE datname NOT LIKE 'template0';\n"
+				 "A list of databases with the problem is given in the file:\n"
+				 "    %s\n\n", output_path);
+	}
+	else
+		check_ok();
 }
 
 
-- 
2.25.1

Suraj Kharage

suraj.kharage@enterprisedb.com

over 4 years ago

In reply to: Jeevan Ladhe (#5)

Re: [PATCH] improve the pg_upgrade error message

Thanks Jeevan for working on this.
Overall patch looks good to me.

+ pg_fatal("All non-template0 databases must allow connections, i.e.
their\n"
+ "pg_database.datallowconn must be true. Your installation contains\n"
+ "non-template0 databases with their pg_database.datallowconn set to\n"
+ "false. Consider allowing connection for all non-template0 databases\n"
+ "using:\n"
+ "    UPDATE pg_catalog.pg_database SET datallowconn='true' WHERE datname
NOT LIKE 'template0';\n"
+ "A list of databases with the problem is given in the file:\n"
+ "    %s\n\n", output_path);

Instead of giving suggestion about updating the pg_database catalog, can we
give "ALTER DATABASE <datname> ALLOW_CONNECTIONS true;" command?
Also, it would be good if we give 2 spaces after full stop in an error
message.

On Tue, Jul 13, 2021 at 6:57 PM Jeevan Ladhe <jeevan.ladhe@enterprisedb.com>
wrote:

The admin might fix DB123, restart their upgrade procedure, spend 5 or 15

minutes with that, only to have it then fail on DB1234.

Agree with this observation.

Here is a patch that writes the list of all the databases other than
template0
that are having their pg_database.datallowconn to false in a file. Similar
approach is seen in other functions like check_for_data_types_usage(),
check_for_data_types_usage() etc. Thanks Suraj Kharage for the offline
suggestion.

PFA patch.

For experiment, here is how it turns out after the fix.

postgres=# update pg_database set datallowconn='false' where datname in
('mydb', 'mydb1', 'mydb2');
UPDATE 3

$ pg_upgrade -d /tmp/v96/data -D /tmp/v13/data -b $HOME/v96/install/bin -B
$HOME/v13/install/bin
Performing Consistency Checks
-----------------------------
Checking cluster versions ok
Checking database user is the install user ok
Checking database connection settings fatal

All non-template0 databases must allow connections, i.e. their
pg_database.datallowconn must be true. Your installation contains
non-template0 databases with their pg_database.datallowconn set to
false. Consider allowing connection for all non-template0 databases
using:
UPDATE pg_catalog.pg_database SET datallowconn='true' WHERE datname
NOT LIKE 'template0';
A list of databases with the problem is given in the file:
databases_with_datallowconn_false.txt

Failure, exiting

$ cat databases_with_datallowconn_false.txt
mydb
mydb1
mydb2

Regards,
Jeevan Ladhe

--
--

Thanks & Regards,
Suraj kharage,

edbpostgres.com

Daniel Gustafsson

daniel@yesql.se

about 4 years ago

In reply to: Suraj Kharage (#6)

1 attachment(s)

Re: [PATCH] improve the pg_upgrade error message

On 14 Jul 2021, at 07:27, Suraj Kharage <suraj.kharage@enterprisedb.com> wrote:

Overall patch looks good to me.

Agreed, I think this is a good change and in line with how the check functions
work in general.

Instead of giving suggestion about updating the pg_database catalog, can we give "ALTER DATABASE <datname> ALLOW_CONNECTIONS true;" command?

I would actually prefer to not give any suggestions at all, we typically don't
in these error messages. Since there are many ways to do it (dropping the
database being one) I think leaving that to the user is per application style.

Also, it would be good if we give 2 spaces after full stop in an error message.

Correct, fixed in the attached which also tweaks the language slightly to match
other errors.

I propose to commit the attached, which also adds a function comment while
there, unless there are objections.

--
Daniel Gustafsson https://vmware.com/

Attachments:

0001-List-offending-databases-in-pg_upgrade-datallowconn-.patchapplication/octet-stream; name=0001-List-offending-databases-in-pg_upgrade-datallowconn-.patch; x-unix-mode=0644Download

From 1b2b555c9d885979f38209b3b6bcf125596d426c Mon Sep 17 00:00:00 2001
From: Daniel Gustafsson <dgustafsson@postgresql.org>
Date: Thu, 21 Oct 2021 12:13:34 +0200
Subject: [PATCH] List offending databases in pg_upgrade datallowconn check

The check for datallowconn being properly set on all databases in the
old cluster errored out on the first instance, rather than report the
set of problematic databases.  This adds reporting to a textfile like
how many other checks are performed.  While there, also add a comment
to the function as per how other checks are commented.

Author: Jeevan Ladhe <jeevan.ladhe@enterprisedb.com>
Reviewed-by: Suraj Kharage <suraj.kharage@enterprisedb.com>
Reviewed-by: Justin Pryzby <pryzby@telsasoft.com>
Discussion: https://postgr.es/m/CAOgcT0McABqF_iFFQuuRf9is+gmYMsmUu_SWNikyr=2VGyP9Jw@mail.gmail.com
---
 src/bin/pg_upgrade/check.c | 37 ++++++++++++++++++++++++++++++++++---
 1 file changed, 34 insertions(+), 3 deletions(-)

diff --git a/src/bin/pg_upgrade/check.c b/src/bin/pg_upgrade/check.c
index ad5f391995..5487641c18 100644
--- a/src/bin/pg_upgrade/check.c
+++ b/src/bin/pg_upgrade/check.c
@@ -693,6 +693,13 @@ check_is_install_user(ClusterInfo *cluster)
 }
 
 
+/*
+ *	check_proper_datallowconn
+ *
+ *	Ensure that all non-template0 databases allow connections since they
+ *	otherwise won't be restored; and that template0 explicitly doesn't allow
+ *	connections since it would make pg_dumpall --globals restore fail.
+ */
 static void
 check_proper_datallowconn(ClusterInfo *cluster)
 {
@@ -702,9 +709,14 @@ check_proper_datallowconn(ClusterInfo *cluster)
 	int			ntups;
 	int			i_datname;
 	int			i_datallowconn;
+	FILE	   *script = NULL;
+	char		output_path[MAXPGPATH];
 
 	prep_status("Checking database connection settings");
 
+	snprintf(output_path, sizeof(output_path),
+			 "databases_with_datallowconn_false.txt");
+
 	conn_template1 = connectToServer(cluster, "template1");
 
 	/* get database names */
@@ -735,8 +747,13 @@ check_proper_datallowconn(ClusterInfo *cluster)
 			 * restore
 			 */
 			if (strcmp(datallowconn, "f") == 0)
-				pg_fatal("All non-template0 databases must allow connections, "
-						 "i.e. their pg_database.datallowconn must be true\n");
+			{
+				if (script == NULL && (script = fopen_priv(output_path, "w")) == NULL)
+					pg_fatal("could not open file \"%s\": %s\n",
+							 output_path, strerror(errno));
+
+				fprintf(script, "%s\n", datname);
+			}
 		}
 	}
 
@@ -744,7 +761,21 @@ check_proper_datallowconn(ClusterInfo *cluster)
 
 	PQfinish(conn_template1);
 
-	check_ok();
+	if (script)
+	{
+		fclose(script);
+
+		pg_log(PG_REPORT, "fatal\n");
+		pg_fatal("All non-template0 databases must allow connections, i.e. their\n"
+				 "pg_database.datallowconn must be true.  Your installation contains\n"
+				 "non-template0 databases with their pg_database.datallowconn set to\n"
+				 "false.  Consider allowing connection for all non-template0 databases\n"
+				 "or drop the databases which do not allow connections.  A list of\n"
+				 "databases with the problem is in the file:\n"
+				 "    %s\n\n", output_path);
+	}
+	else
+		check_ok();
 }
 
 
-- 
2.24.3 (Apple Git-128)

Jeevan Ladhe

jeevan.ladhe@enterprisedb.com

about 4 years ago

In reply to: Daniel Gustafsson (#7)

Re: [PATCH] improve the pg_upgrade error message

Hi Daniel,

Was wondering if we had any barriers to getting this committed.
I believe it will be good to have this change and also it will be more in
line
with other check functions also.

Regards,
Jeevan

On Thu, Oct 21, 2021 at 3:51 PM Daniel Gustafsson <daniel@yesql.se> wrote:

Show quoted text

On 14 Jul 2021, at 07:27, Suraj Kharage <suraj.kharage@enterprisedb.com>

wrote:

Overall patch looks good to me.

Agreed, I think this is a good change and in line with how the check
functions
work in general.

Instead of giving suggestion about updating the pg_database catalog, can

we give "ALTER DATABASE <datname> ALLOW_CONNECTIONS true;" command?

I would actually prefer to not give any suggestions at all, we typically
don't
in these error messages. Since there are many ways to do it (dropping the
database being one) I think leaving that to the user is per application
style.

Also, it would be good if we give 2 spaces after full stop in an error

message.

Correct, fixed in the attached which also tweaks the language slightly to
match
other errors.

I propose to commit the attached, which also adds a function comment while
there, unless there are objections.

--
Daniel Gustafsson https://vmware.com/

Daniel Gustafsson

daniel@yesql.se

about 4 years ago

In reply to: Jeevan Ladhe (#8)

Re: [PATCH] improve the pg_upgrade error message

On 1 Dec 2021, at 10:59, Jeevan Ladhe <jeevan.ladhe@enterprisedb.com> wrote:

Was wondering if we had any barriers to getting this committed.

No barrier other than available time to, I will try to get to it shortly.

--
Daniel Gustafsson https://vmware.com/

#10

Jeevan Ladhe

jeevan.ladhe@enterprisedb.com

about 4 years ago

In reply to: Daniel Gustafsson (#9)

Re: [PATCH] improve the pg_upgrade error message

On Wed, Dec 1, 2021 at 3:45 PM Daniel Gustafsson <daniel@yesql.se> wrote:

On 1 Dec 2021, at 10:59, Jeevan Ladhe <jeevan.ladhe@enterprisedb.com>

wrote:

Was wondering if we had any barriers to getting this committed.

No barrier other than available time to, I will try to get to it shortly.

Great! Thank you.

Show quoted text

--
Daniel Gustafsson https://vmware.com/

#11

Daniel Gustafsson

daniel@yesql.se

almost 4 years ago

In reply to: Daniel Gustafsson (#9)

Re: [PATCH] improve the pg_upgrade error message

On 1 Dec 2021, at 11:15, Daniel Gustafsson <daniel@yesql.se> wrote:

On 1 Dec 2021, at 10:59, Jeevan Ladhe <jeevan.ladhe@enterprisedb.com> wrote:

Was wondering if we had any barriers to getting this committed.

No barrier other than available time to, I will try to get to it shortly.

The "shortly" aspect wasn't really fulfilled, but I got around to taking
another look at this today and pushed it now with a few small changes to
reflect how pg_upgrade has changed.

--
Daniel Gustafsson https://vmware.com/