Slight refactoring of state check in pg_upgrade check_ function
I noticed that the pg_upgrade check_ functions were determining failures found
in a few different ways. Some keep a boolen flag variable, and some (like
check_for_incompatible_polymorphics) check the state of the script filehandle
which is guaranteed to be set (with the error message referring to the path of
said file). Others like check_loadable_libraries only check the flag variable
and fclose the handle assuming it was opened.
The attached diff changes the functions to do it consistently in one way, by
checking the state of the filehandle. Since we are referring to the file by
path in the printed error message it seemed the cleanest approach, and it saves
a few lines of code without IMO reducing readability.
There is no change in functionality, just code consistency.
--
Daniel Gustafsson https://vmware.com/
Attachments:
v1-0001-Refactor-check_-functions-to-use-filehandle-for-s.patchapplication/octet-stream; name=v1-0001-Refactor-check_-functions-to-use-filehandle-for-s.patch; x-unix-mode=0644Download
From 1bff21c0f1faa80c7cd3aba2425ed660d6b0331f Mon Sep 17 00:00:00 2001
From: Daniel Gustafsson <dgustafsson@postgresql.org>
Date: Sun, 28 Aug 2022 22:13:37 +0200
Subject: [PATCH v1] Refactor check_ functions to use filehandle for status
When reporting failure in check_ functions there is (typically) a text-
file mentioned in the error report which contains further details. Some
check_ functions kept a separate flag variable to indicate failure, and
some just checked the state of the filehandle as it's guaranteed to be
open when the check failed. This refactors the functions to consistently
do the same check on error reporting. As the error report contains the
filepath, it makes more sense to check the filehandle state and skip the
flag variable.
Discussion: https://postgr.es/m/xxx
---
src/bin/pg_upgrade/check.c | 30 +++++-------------------------
src/bin/pg_upgrade/function.c | 4 +---
src/bin/pg_upgrade/version.c | 7 +------
3 files changed, 7 insertions(+), 34 deletions(-)
diff --git a/src/bin/pg_upgrade/check.c b/src/bin/pg_upgrade/check.c
index 0ed0590d82..f4969bcdad 100644
--- a/src/bin/pg_upgrade/check.c
+++ b/src/bin/pg_upgrade/check.c
@@ -711,7 +711,6 @@ check_proper_datallowconn(ClusterInfo *cluster)
int i_datallowconn;
FILE *script = NULL;
char output_path[MAXPGPATH];
- bool found = false;
prep_status("Checking database connection settings");
@@ -750,7 +749,6 @@ check_proper_datallowconn(ClusterInfo *cluster)
*/
if (strcmp(datallowconn, "f") == 0)
{
- found = true;
if (script == NULL && (script = fopen_priv(output_path, "w")) == NULL)
pg_fatal("could not open file \"%s\": %s",
output_path, strerror(errno));
@@ -765,10 +763,8 @@ check_proper_datallowconn(ClusterInfo *cluster)
PQfinish(conn_template1);
if (script)
- fclose(script);
-
- if (found)
{
+ fclose(script);
pg_log(PG_REPORT, "fatal");
pg_fatal("All non-template0 databases must allow connections, i.e. their\n"
"pg_database.datallowconn must be true. Your installation contains\n"
@@ -829,7 +825,6 @@ check_for_isn_and_int8_passing_mismatch(ClusterInfo *cluster)
{
int dbnum;
FILE *script = NULL;
- bool found = false;
char output_path[MAXPGPATH];
prep_status("Checking for contrib/isn with bigint-passing mismatch");
@@ -870,7 +865,6 @@ check_for_isn_and_int8_passing_mismatch(ClusterInfo *cluster)
i_proname = PQfnumber(res, "proname");
for (rowno = 0; rowno < ntups; rowno++)
{
- found = true;
if (script == NULL && (script = fopen_priv(output_path, "w")) == NULL)
pg_fatal("could not open file \"%s\": %s",
output_path, strerror(errno));
@@ -890,10 +884,8 @@ check_for_isn_and_int8_passing_mismatch(ClusterInfo *cluster)
}
if (script)
- fclose(script);
-
- if (found)
{
+ fclose(script);
pg_log(PG_REPORT, "fatal");
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"
@@ -915,7 +907,6 @@ check_for_user_defined_postfix_ops(ClusterInfo *cluster)
{
int dbnum;
FILE *script = NULL;
- bool found = false;
char output_path[MAXPGPATH];
prep_status("Checking for user-defined postfix operators");
@@ -968,7 +959,6 @@ check_for_user_defined_postfix_ops(ClusterInfo *cluster)
i_typname = PQfnumber(res, "typname");
for (rowno = 0; rowno < ntups; rowno++)
{
- found = true;
if (script == NULL &&
(script = fopen_priv(output_path, "w")) == NULL)
pg_fatal("could not open file \"%s\": %s",
@@ -992,10 +982,8 @@ check_for_user_defined_postfix_ops(ClusterInfo *cluster)
}
if (script)
- fclose(script);
-
- if (found)
{
+ fclose(script);
pg_log(PG_REPORT, "fatal");
pg_fatal("Your installation contains user-defined postfix operators, which are not\n"
"supported anymore. Consider dropping the postfix operators and replacing\n"
@@ -1145,7 +1133,6 @@ check_for_tables_with_oids(ClusterInfo *cluster)
{
int dbnum;
FILE *script = NULL;
- bool found = false;
char output_path[MAXPGPATH];
prep_status("Checking for tables WITH OIDS");
@@ -1179,7 +1166,6 @@ check_for_tables_with_oids(ClusterInfo *cluster)
i_relname = PQfnumber(res, "relname");
for (rowno = 0; rowno < ntups; rowno++)
{
- found = true;
if (script == NULL && (script = fopen_priv(output_path, "w")) == NULL)
pg_fatal("could not open file \"%s\": %s",
output_path, strerror(errno));
@@ -1199,10 +1185,8 @@ check_for_tables_with_oids(ClusterInfo *cluster)
}
if (script)
- fclose(script);
-
- if (found)
{
+ fclose(script);
pg_log(PG_REPORT, "fatal");
pg_fatal("Your installation contains tables declared WITH OIDS, which is not\n"
"supported anymore. Consider removing the oid column using\n"
@@ -1401,7 +1385,6 @@ check_for_user_defined_encoding_conversions(ClusterInfo *cluster)
{
int dbnum;
FILE *script = NULL;
- bool found = false;
char output_path[MAXPGPATH];
prep_status("Checking for user-defined encoding conversions");
@@ -1441,7 +1424,6 @@ check_for_user_defined_encoding_conversions(ClusterInfo *cluster)
i_nspname = PQfnumber(res, "nspname");
for (rowno = 0; rowno < ntups; rowno++)
{
- found = true;
if (script == NULL &&
(script = fopen_priv(output_path, "w")) == NULL)
pg_fatal("could not open file \"%s\": %s",
@@ -1463,10 +1445,8 @@ check_for_user_defined_encoding_conversions(ClusterInfo *cluster)
}
if (script)
- fclose(script);
-
- if (found)
{
+ fclose(script);
pg_log(PG_REPORT, "fatal");
pg_fatal("Your installation contains user-defined encoding conversions.\n"
"The conversion function parameters changed in PostgreSQL version 14\n"
diff --git a/src/bin/pg_upgrade/function.c b/src/bin/pg_upgrade/function.c
index 70b492cc3a..93d975864b 100644
--- a/src/bin/pg_upgrade/function.c
+++ b/src/bin/pg_upgrade/function.c
@@ -123,7 +123,6 @@ check_loadable_libraries(void)
int libnum;
int was_load_failure = false;
FILE *script = NULL;
- bool found = false;
char output_path[MAXPGPATH];
prep_status("Checking for presence of required libraries");
@@ -158,7 +157,6 @@ check_loadable_libraries(void)
if (PQresultStatus(res) != PGRES_COMMAND_OK)
{
- found = true;
was_load_failure = true;
if (script == NULL && (script = fopen_priv(output_path, "w")) == NULL)
@@ -181,7 +179,7 @@ check_loadable_libraries(void)
PQfinish(conn);
- if (found)
+ if (script)
{
fclose(script);
pg_log(PG_REPORT, "fatal");
diff --git a/src/bin/pg_upgrade/version.c b/src/bin/pg_upgrade/version.c
index d2636ba857..064d23797c 100644
--- a/src/bin/pg_upgrade/version.c
+++ b/src/bin/pg_upgrade/version.c
@@ -391,7 +391,6 @@ report_extension_updates(ClusterInfo *cluster)
{
int dbnum;
FILE *script = NULL;
- bool found = false;
char *output_path = "update_extensions.sql";
prep_status("Checking for extension updates");
@@ -417,8 +416,6 @@ report_extension_updates(ClusterInfo *cluster)
i_name = PQfnumber(res, "name");
for (rowno = 0; rowno < ntups; rowno++)
{
- found = true;
-
if (script == NULL && (script = fopen_priv(output_path, "w")) == NULL)
pg_fatal("could not open file \"%s\": %s", output_path,
strerror(errno));
@@ -442,10 +439,8 @@ report_extension_updates(ClusterInfo *cluster)
}
if (script)
- fclose(script);
-
- if (found)
{
+ fclose(script);
report_status(PG_REPORT, "notice");
pg_log(PG_REPORT, "\n"
"Your installation contains extensions that should be updated\n"
--
2.32.1 (Apple Git-133)
On Sun, Aug 28, 2022 at 10:42:24PM +0200, Daniel Gustafsson wrote:
I noticed that the pg_upgrade check_ functions were determining failures found
in a few different ways. Some keep a boolen flag variable, and some (like
check_for_incompatible_polymorphics) check the state of the script filehandle
which is guaranteed to be set (with the error message referring to the path of
said file). Others like check_loadable_libraries only check the flag variable
and fclose the handle assuming it was opened.The attached diff changes the functions to do it consistently in one way, by
checking the state of the filehandle. Since we are referring to the file by
path in the printed error message it seemed the cleanest approach, and it saves
a few lines of code without IMO reducing readability.There is no change in functionality, just code consistency.
The patch looks reasonable to me.
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
On Sun, Aug 28, 2022 at 03:06:09PM -0700, Nathan Bossart wrote:
On Sun, Aug 28, 2022 at 10:42:24PM +0200, Daniel Gustafsson wrote:
I noticed that the pg_upgrade check_ functions were determining failures found
in a few different ways. Some keep a boolen flag variable, and some (like
check_for_incompatible_polymorphics) check the state of the script filehandle
which is guaranteed to be set (with the error message referring to the path of
said file). Others like check_loadable_libraries only check the flag variable
and fclose the handle assuming it was opened.The attached diff changes the functions to do it consistently in one way, by
checking the state of the filehandle. Since we are referring to the file by
path in the printed error message it seemed the cleanest approach, and it saves
a few lines of code without IMO reducing readability.There is no change in functionality, just code consistency.
The patch looks reasonable to me.
+1. Those checks have accumulated over time with different authors,
hence the stylistic differences.
--
Bruce Momjian <bruce@momjian.us> https://momjian.us
EDB https://enterprisedb.com
Indecision is a decision. Inaction is an action. Mark Batterson
On 30 Aug 2022, at 23:08, Bruce Momjian <bruce@momjian.us> wrote:
On Sun, Aug 28, 2022 at 03:06:09PM -0700, Nathan Bossart wrote:
The patch looks reasonable to me.
+1. Those checks have accumulated over time with different authors,
hence the stylistic differences.
Pushed, thanks for review!
--
Daniel Gustafsson https://vmware.com/