pg_upgrade generated files in subdir follow-up
Commit 38bfae36526 moved the .txt files pg_upgrade generates to a separate
subdir, but there are a few left which are written to cwd. The thread
resulting in that patch doesn't discuss these files specifically so it seems
they are just an oversight. Unless I'm missing something.
Should something the attached be applied to ensure all generated files are
placed in the subdirectory?
--
Daniel Gustafsson https://vmware.com/
Attachments:
pg_upgrade_generated_files.diffapplication/octet-stream; name=pg_upgrade_generated_files.diff; x-unix-mode=0644Download
diff --git a/src/bin/pg_upgrade/check.c b/src/bin/pg_upgrade/check.c
index f4969bcdad..f1bc1e6886 100644
--- a/src/bin/pg_upgrade/check.c
+++ b/src/bin/pg_upgrade/check.c
@@ -1218,7 +1218,9 @@ check_for_composite_data_type_usage(ClusterInfo *cluster)
prep_status("Checking for system-defined composite types in user tables");
- snprintf(output_path, sizeof(output_path), "tables_using_composite.txt");
+ snprintf(output_path, sizeof(output_path), "%s/%s",
+ log_opts.basedir,
+ "tables_using_composite.txt");
/*
* Look for composite types that were made during initdb *or* belong to
@@ -1275,7 +1277,9 @@ check_for_reg_data_type_usage(ClusterInfo *cluster)
prep_status("Checking for reg* data types in user tables");
- snprintf(output_path, sizeof(output_path), "tables_using_reg.txt");
+ snprintf(output_path, sizeof(output_path), "%s/%s",
+ log_opts.basedir,
+ "tables_using_reg.txt");
/*
* Note: older servers will not have all of these reg* types, so we have
@@ -1328,7 +1332,9 @@ check_for_jsonb_9_4_usage(ClusterInfo *cluster)
prep_status("Checking for incompatible \"jsonb\" data type");
- snprintf(output_path, sizeof(output_path), "tables_using_jsonb.txt");
+ snprintf(output_path, sizeof(output_path), "%s/%s",
+ log_opts.basedir,
+ "tables_using_jsonb.txt");
if (check_for_data_type_usage(cluster, "pg_catalog.jsonb", output_path))
{
diff --git a/src/bin/pg_upgrade/version.c b/src/bin/pg_upgrade/version.c
index 064d23797c..dc19fc6ec8 100644
--- a/src/bin/pg_upgrade/version.c
+++ b/src/bin/pg_upgrade/version.c
@@ -183,7 +183,9 @@ old_9_3_check_for_line_data_type_usage(ClusterInfo *cluster)
prep_status("Checking for incompatible \"line\" data type");
- snprintf(output_path, sizeof(output_path), "tables_using_line.txt");
+ snprintf(output_path, sizeof(output_path), "%s/%s",
+ log_opts.basedir,
+ "tables_using_line.txt");
if (check_for_data_type_usage(cluster, "pg_catalog.line", output_path))
{
@@ -221,7 +223,9 @@ old_9_6_check_for_unknown_data_type_usage(ClusterInfo *cluster)
prep_status("Checking for invalid \"unknown\" user columns");
- snprintf(output_path, sizeof(output_path), "tables_using_unknown.txt");
+ snprintf(output_path, sizeof(output_path), "%s/%s",
+ log_opts.basedir,
+ "tables_using_unknown.txt");
if (check_for_data_type_usage(cluster, "pg_catalog.unknown", output_path))
{
@@ -364,7 +368,9 @@ old_11_check_for_sql_identifier_data_type_usage(ClusterInfo *cluster)
prep_status("Checking for invalid \"sql_identifier\" user columns");
- snprintf(output_path, sizeof(output_path), "tables_using_sql_identifier.txt");
+ snprintf(output_path, sizeof(output_path), "%s/%s",
+ log_opts.basedir,
+ "tables_using_sql_identifier.txt");
if (check_for_data_type_usage(cluster, "information_schema.sql_identifier",
output_path))
Daniel Gustafsson <daniel@yesql.se> writes:
Commit 38bfae36526 moved the .txt files pg_upgrade generates to a separate
subdir, but there are a few left which are written to cwd. The thread
resulting in that patch doesn't discuss these files specifically so it seems
they are just an oversight. Unless I'm missing something.
Should something the attached be applied to ensure all generated files are
placed in the subdirectory?
It certainly looks inconsistent ATM. I wondered if maybe the plan was to
put routine output into the log directory but problem-reporting files
into cwd --- but that isn't what's happening now.
As long as we report the path to where the file is, I don't see a reason
not to put problem-reporting files in the subdir too.
regards, tom lane
On 31 Aug 2022, at 15:59, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Daniel Gustafsson <daniel@yesql.se> writes:
Commit 38bfae36526 moved the .txt files pg_upgrade generates to a separate
subdir, but there are a few left which are written to cwd. The thread
resulting in that patch doesn't discuss these files specifically so it seems
they are just an oversight. Unless I'm missing something.Should something the attached be applied to ensure all generated files are
placed in the subdirectory?It certainly looks inconsistent ATM. I wondered if maybe the plan was to
put routine output into the log directory but problem-reporting files
into cwd --- but that isn't what's happening now.
Right, check_proper_datallowconn and check_for_isn_and_int8_passing_mismatch
and a few other check functions already place error reporting in the subdir.
As long as we report the path to where the file is, I don't see a reason
not to put problem-reporting files in the subdir too.
Agreed. The documentation states:
"pg_upgrade creates various working files, such as schema dumps, stored
within pg_upgrade_output.d in the directory of the new cluster. Each
run creates a new subdirectory named with a timestamp formatted as per
ISO 8601 (%Y%m%dT%H%M%S), where all the generated files are stored."
The delete_old_cluster and reindex_hash scripts are still placed in CWD, which
isn't changed by this patch, as that seems correct (and might break scripts if
we move them). Maybe we should amend the docs to mention that scripts aren't
generated in the subdir?
--
Daniel Gustafsson https://vmware.com/
On Wed, Aug 31, 2022 at 04:41:24PM +0200, Daniel Gustafsson wrote:
On 31 Aug 2022, at 15:59, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Daniel Gustafsson <daniel@yesql.se> writes:
Commit 38bfae36526 moved the .txt files pg_upgrade generates to a separate
subdir, but there are a few left which are written to cwd. The thread
resulting in that patch doesn't discuss these files specifically so it seems
they are just an oversight. Unless I'm missing something.Should something the attached be applied to ensure all generated files are
placed in the subdirectory?It certainly looks inconsistent ATM. I wondered if maybe the plan was to
put routine output into the log directory but problem-reporting files
into cwd --- but that isn't what's happening now.
The script files are intended to stay where they are, and the error
files are intended to move under the subdir, to allow for their easy
removal, per Tom's request.
Right, check_proper_datallowconn and check_for_isn_and_int8_passing_mismatch
and a few other check functions already place error reporting in the subdir.
It looks like I may have grepped for fprintf or similar, and missed
checking output_path.
I updated your patach to put the logic inside
check_for_data_types_usage(), which is shorter, and seems to simplify
doing what's intended into the future.
--
Justin
Attachments:
0001-pg_upgrade-generated-files-in-subdir-follow-up.patchtext/x-diff; charset=us-asciiDownload
From 36aaf5470d5b0bb0ad2a322880e526b8c5f87066 Mon Sep 17 00:00:00 2001
From: Daniel Gustafsson <daniel@yesql.se>
Date: Wed, 31 Aug 2022 14:09:06 +0200
Subject: [PATCH 1/2] pg_upgrade generated files in subdir follow-up
Commit 38bfae36526 moved the .txt files pg_upgrade generates to a separate
subdir, but there are a few left which are written to cwd. The thread
resulting in that patch doesn't discuss these files specifically so it seems
they are just an oversight. Unless I'm missing something.
Should something the attached be applied to ensure all generated files are
placed in the subdirectory?
--
Daniel Gustafsson https://vmware.com/
---
src/bin/pg_upgrade/check.c | 12 +++++++++---
src/bin/pg_upgrade/version.c | 12 +++++++++---
2 files changed, 18 insertions(+), 6 deletions(-)
diff --git a/src/bin/pg_upgrade/check.c b/src/bin/pg_upgrade/check.c
index f4969bcdad7..f1bc1e68868 100644
--- a/src/bin/pg_upgrade/check.c
+++ b/src/bin/pg_upgrade/check.c
@@ -1218,7 +1218,9 @@ check_for_composite_data_type_usage(ClusterInfo *cluster)
prep_status("Checking for system-defined composite types in user tables");
- snprintf(output_path, sizeof(output_path), "tables_using_composite.txt");
+ snprintf(output_path, sizeof(output_path), "%s/%s",
+ log_opts.basedir,
+ "tables_using_composite.txt");
/*
* Look for composite types that were made during initdb *or* belong to
@@ -1275,7 +1277,9 @@ check_for_reg_data_type_usage(ClusterInfo *cluster)
prep_status("Checking for reg* data types in user tables");
- snprintf(output_path, sizeof(output_path), "tables_using_reg.txt");
+ snprintf(output_path, sizeof(output_path), "%s/%s",
+ log_opts.basedir,
+ "tables_using_reg.txt");
/*
* Note: older servers will not have all of these reg* types, so we have
@@ -1328,7 +1332,9 @@ check_for_jsonb_9_4_usage(ClusterInfo *cluster)
prep_status("Checking for incompatible \"jsonb\" data type");
- snprintf(output_path, sizeof(output_path), "tables_using_jsonb.txt");
+ snprintf(output_path, sizeof(output_path), "%s/%s",
+ log_opts.basedir,
+ "tables_using_jsonb.txt");
if (check_for_data_type_usage(cluster, "pg_catalog.jsonb", output_path))
{
diff --git a/src/bin/pg_upgrade/version.c b/src/bin/pg_upgrade/version.c
index 064d23797c5..dc19fc6ec84 100644
--- a/src/bin/pg_upgrade/version.c
+++ b/src/bin/pg_upgrade/version.c
@@ -183,7 +183,9 @@ old_9_3_check_for_line_data_type_usage(ClusterInfo *cluster)
prep_status("Checking for incompatible \"line\" data type");
- snprintf(output_path, sizeof(output_path), "tables_using_line.txt");
+ snprintf(output_path, sizeof(output_path), "%s/%s",
+ log_opts.basedir,
+ "tables_using_line.txt");
if (check_for_data_type_usage(cluster, "pg_catalog.line", output_path))
{
@@ -221,7 +223,9 @@ old_9_6_check_for_unknown_data_type_usage(ClusterInfo *cluster)
prep_status("Checking for invalid \"unknown\" user columns");
- snprintf(output_path, sizeof(output_path), "tables_using_unknown.txt");
+ snprintf(output_path, sizeof(output_path), "%s/%s",
+ log_opts.basedir,
+ "tables_using_unknown.txt");
if (check_for_data_type_usage(cluster, "pg_catalog.unknown", output_path))
{
@@ -364,7 +368,9 @@ old_11_check_for_sql_identifier_data_type_usage(ClusterInfo *cluster)
prep_status("Checking for invalid \"sql_identifier\" user columns");
- snprintf(output_path, sizeof(output_path), "tables_using_sql_identifier.txt");
+ snprintf(output_path, sizeof(output_path), "%s/%s",
+ log_opts.basedir,
+ "tables_using_sql_identifier.txt");
if (check_for_data_type_usage(cluster, "information_schema.sql_identifier",
output_path))
--
2.17.1
0002-f.patchtext/x-diff; charset=us-asciiDownload
From 22a18af9fa4bed32db50a600d1c2770c1e929fbd Mon Sep 17 00:00:00 2001
From: Justin Pryzby <pryzbyj@telsasoft.com>
Date: Fri, 9 Sep 2022 07:52:02 -0500
Subject: [PATCH 2/2] f
---
src/bin/pg_upgrade/check.c | 11 ++++-----
src/bin/pg_upgrade/pg_upgrade.h | 2 +-
src/bin/pg_upgrade/version.c | 41 ++++++++++++++-------------------
3 files changed, 22 insertions(+), 32 deletions(-)
diff --git a/src/bin/pg_upgrade/check.c b/src/bin/pg_upgrade/check.c
index f1bc1e68868..dc722da910c 100644
--- a/src/bin/pg_upgrade/check.c
+++ b/src/bin/pg_upgrade/check.c
@@ -1328,15 +1328,12 @@ check_for_reg_data_type_usage(ClusterInfo *cluster)
static void
check_for_jsonb_9_4_usage(ClusterInfo *cluster)
{
- char output_path[MAXPGPATH];
+ const char output_basename[] = "tables_using_jsonb.txt";
prep_status("Checking for incompatible \"jsonb\" data type");
- snprintf(output_path, sizeof(output_path), "%s/%s",
- log_opts.basedir,
- "tables_using_jsonb.txt");
-
- if (check_for_data_type_usage(cluster, "pg_catalog.jsonb", output_path))
+ if (check_for_data_type_usage(cluster, "pg_catalog.jsonb",
+ output_basename))
{
pg_log(PG_REPORT, "fatal");
pg_fatal("Your installation contains the \"jsonb\" data type in user tables.\n"
@@ -1344,7 +1341,7 @@ check_for_jsonb_9_4_usage(ClusterInfo *cluster)
"cluster cannot currently be upgraded. You can\n"
"drop the problem columns and restart the upgrade.\n"
"A list of the problem columns is in the file:\n"
- " %s", output_path);
+ " %s/%s", log_opts.basedir, output_basename);
}
else
check_ok();
diff --git a/src/bin/pg_upgrade/pg_upgrade.h b/src/bin/pg_upgrade/pg_upgrade.h
index 60c3c8dd68e..0a1bdfca5b2 100644
--- a/src/bin/pg_upgrade/pg_upgrade.h
+++ b/src/bin/pg_upgrade/pg_upgrade.h
@@ -443,7 +443,7 @@ unsigned int str2uint(const char *str);
bool check_for_data_types_usage(ClusterInfo *cluster,
const char *base_query,
- const char *output_path);
+ const char *output_basename);
bool check_for_data_type_usage(ClusterInfo *cluster,
const char *type_name,
const char *output_path);
diff --git a/src/bin/pg_upgrade/version.c b/src/bin/pg_upgrade/version.c
index dc19fc6ec84..059bd9b1e1c 100644
--- a/src/bin/pg_upgrade/version.c
+++ b/src/bin/pg_upgrade/version.c
@@ -30,12 +30,16 @@
bool
check_for_data_types_usage(ClusterInfo *cluster,
const char *base_query,
- const char *output_path)
+ const char *output_basename)
{
bool found = false;
FILE *script = NULL;
int dbnum;
+ char output_path[MAXPGPATH];
+ snprintf(output_path, sizeof(output_path), "%s/%s", log_opts.basedir,
+ output_basename);
+
for (dbnum = 0; dbnum < cluster->dbarr.ndbs; dbnum++)
{
DbInfo *active_db = &cluster->dbarr.dbs[dbnum];
@@ -152,7 +156,7 @@ check_for_data_types_usage(ClusterInfo *cluster,
bool
check_for_data_type_usage(ClusterInfo *cluster,
const char *type_name,
- const char *output_path)
+ const char *output_basename)
{
bool found;
char *base_query;
@@ -160,7 +164,7 @@ check_for_data_type_usage(ClusterInfo *cluster,
base_query = psprintf("SELECT '%s'::pg_catalog.regtype AS oid",
type_name);
- found = check_for_data_types_usage(cluster, base_query, output_path);
+ found = check_for_data_types_usage(cluster, base_query, output_basename);
free(base_query);
@@ -179,15 +183,11 @@ check_for_data_type_usage(ClusterInfo *cluster,
void
old_9_3_check_for_line_data_type_usage(ClusterInfo *cluster)
{
- char output_path[MAXPGPATH];
+ const char output_basename[] = "tables_using_line.txt";
prep_status("Checking for incompatible \"line\" data type");
- snprintf(output_path, sizeof(output_path), "%s/%s",
- log_opts.basedir,
- "tables_using_line.txt");
-
- if (check_for_data_type_usage(cluster, "pg_catalog.line", output_path))
+ if (check_for_data_type_usage(cluster, "pg_catalog.line", output_basename))
{
pg_log(PG_REPORT, "fatal");
pg_fatal("Your installation contains the \"line\" data type in user tables.\n"
@@ -196,7 +196,7 @@ old_9_3_check_for_line_data_type_usage(ClusterInfo *cluster)
"cluster cannot currently be upgraded. You can\n"
"drop the problem columns and restart the upgrade.\n"
"A list of the problem columns is in the file:\n"
- " %s", output_path);
+ " %s/%s", log_opts.basedir, output_basename);
}
else
check_ok();
@@ -219,15 +219,12 @@ old_9_3_check_for_line_data_type_usage(ClusterInfo *cluster)
void
old_9_6_check_for_unknown_data_type_usage(ClusterInfo *cluster)
{
- char output_path[MAXPGPATH];
+ const char output_basename[] = "tables_using_unknown.txt";
prep_status("Checking for invalid \"unknown\" user columns");
- snprintf(output_path, sizeof(output_path), "%s/%s",
- log_opts.basedir,
- "tables_using_unknown.txt");
-
- if (check_for_data_type_usage(cluster, "pg_catalog.unknown", output_path))
+ if (check_for_data_type_usage(cluster, "pg_catalog.unknown",
+ output_basename))
{
pg_log(PG_REPORT, "fatal");
pg_fatal("Your installation contains the \"unknown\" data type in user tables.\n"
@@ -235,7 +232,7 @@ old_9_6_check_for_unknown_data_type_usage(ClusterInfo *cluster)
"cluster cannot currently be upgraded. You can\n"
"drop the problem columns and restart the upgrade.\n"
"A list of the problem columns is in the file:\n"
- " %s", output_path);
+ " %s/%s", log_opts.basedir, output_basename);
}
else
check_ok();
@@ -364,16 +361,12 @@ old_9_6_invalidate_hash_indexes(ClusterInfo *cluster, bool check_mode)
void
old_11_check_for_sql_identifier_data_type_usage(ClusterInfo *cluster)
{
- char output_path[MAXPGPATH];
+ const char output_basename[] = "tables_using_sql_identifier.txt";
prep_status("Checking for invalid \"sql_identifier\" user columns");
- snprintf(output_path, sizeof(output_path), "%s/%s",
- log_opts.basedir,
- "tables_using_sql_identifier.txt");
-
if (check_for_data_type_usage(cluster, "information_schema.sql_identifier",
- output_path))
+ output_basename))
{
pg_log(PG_REPORT, "fatal");
pg_fatal("Your installation contains the \"sql_identifier\" data type in user tables.\n"
@@ -381,7 +374,7 @@ old_11_check_for_sql_identifier_data_type_usage(ClusterInfo *cluster)
"cluster cannot currently be upgraded. You can\n"
"drop the problem columns and restart the upgrade.\n"
"A list of the problem columns is in the file:\n"
- " %s", output_path);
+ " %s/%s", log_opts.basedir, output_basename);
}
else
check_ok();
--
2.17.1
On Fri, Sep 09, 2022 at 11:07:41AM -0500, Justin Pryzby wrote:
On Wed, Aug 31, 2022 at 04:41:24PM +0200, Daniel Gustafsson wrote:
On 31 Aug 2022, at 15:59, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Daniel Gustafsson <daniel@yesql.se> writes:
Commit 38bfae36526 moved the .txt files pg_upgrade generates to a separate
subdir, but there are a few left which are written to cwd. The thread
resulting in that patch doesn't discuss these files specifically so it seems
they are just an oversight. Unless I'm missing something.Should something the attached be applied to ensure all generated files are
placed in the subdirectory?It certainly looks inconsistent ATM. I wondered if maybe the plan was to
put routine output into the log directory but problem-reporting files
into cwd --- but that isn't what's happening now.The script files are intended to stay where they are, and the error
files are intended to move under the subdir, to allow for their easy
removal, per Tom's request.
Right. The .txt files reporting that something went wrong should be
in the basedir, like loadable_libraries.txt, as these are not really
internal logs but provide information about a failure state. I have
double-checked the whole code of pg_upgrade, and I am not seeing
another area to fix, so 0001 looks fine to me. This one is on me, so
I guess that I'd like to take care of it myself.
It looks like I may have grepped for fprintf or similar, and missed
checking output_path.I updated your patach to put the logic inside
check_for_data_types_usage(), which is shorter, and seems to simplify
doing what's intended into the future.
0002 makes the code more complicated IMO, as we still need to report
the location of the file in the logs. So I would leave things to
what's proposed in 0001.
--
Michael
On 12 Sep 2022, at 09:33, Michael Paquier <michael@paquier.xyz> wrote:
I have double-checked the whole code of pg_upgrade, and I am not seeing another
area to fix, so 0001 looks fine to me. This one is on me, so I guess that I'd
like to take care of it myself.
Sure, go for it.
--
Daniel Gustafsson https://vmware.com/