Re: [HACKERS] Fix for pg_upgrade and invalid indexes
OK, patch applied and backpatched as far back as pg_upgrade exists in
git.
---
On Fri, Mar 29, 2013 at 11:35:13PM -0400, Bruce Momjian wrote:
> On Fri, Mar 29, 2013 at 07:03:05PM -0400, Tom Lane wrote:
> > Andres Freund writes:
> > > Those columns cannot be NULL, so using IS DISTINCT FROM seems a bit
> > > clumsy.
> >
> > That was what I started to write, too, but actually I think the IS
> > DISTINCT is correct and the RIGHT JOIN should be a LEFT JOIN. Note
> > that the query appears to be intended to collect regular tables as
> > well as indexes. (As patched, that's totally broken, so I infer
> > Bruce hasn't tested it yet.)
>
> Yes, I only ran my simple tests so far --- I wanted to at least get some
> eyes on it. I was wondering if we ever need to use parentheses for
> queries that mix normal and outer joins? I am unclear on that.
>
> Attached is a fixed patch that uses LEFT JOIN. I went back and looked
> at the patch that added this test and I think the patch is now complete.
> I would like to apply it tomorrow/Saturday so it will be ready for
> Monday's packaging, and get some buildfarm time on it.
>
> --
> Bruce Momjian http://momjian.us
> EnterpriseDB http://enterprisedb.com
>
> + It's impossible for everything to be true. +
> diff --git a/contrib/pg_upgrade/check.c b/contrib/pg_upgrade/check.c
> new file mode 100644
> index 65fb548..35783d0
> *** a/contrib/pg_upgrade/check.c
> --- b/contrib/pg_upgrade/check.c
> *** static void check_is_super_user(ClusterI
> *** 20,26
> static void check_for_prepared_transactions(ClusterInfo *cluster);
> static void check_for_isn_and_int8_passing_mismatch(ClusterInfo *cluster);
> static void check_for_reg_data_type_usage(ClusterInfo *cluster);
> - static void check_for_invalid_indexes(ClusterInfo *cluster);
> static void get_bin_version(ClusterInfo *cluster);
> static char *get_canonical_locale_name(int category, const char *locale);
>
> --- 20,25
> *** check_and_dump_old_cluster(bool live_che
> *** 97,103
> check_is_super_user(&old_cluster);
> check_for_prepared_transactions(&old_cluster);
> check_for_reg_data_type_usage(&old_cluster);
> - check_for_invalid_indexes(&old_cluster);
> check_for_isn_and_int8_passing_mismatch(&old_cluster);
>
> /* old = PG 8.3 checks? */
> --- 96,101
> *** check_for_reg_data_type_usage(ClusterInf
> *** 952,1046
> "%s\n\n", output_path);
> }
> else
> - check_ok();
> - }
> -
> -
> - /*
> - * check_for_invalid_indexes()
> - *
> - * CREATE INDEX CONCURRENTLY can create invalid indexes if the index build
> - * fails. These are dumped as valid indexes by pg_dump, but the
> - * underlying files are still invalid indexes. This checks to make sure
> - * no invalid indexes exist, either failed index builds or concurrent
> - * indexes in the process of being created.
> - */
> - static void
> - check_for_invalid_indexes(ClusterInfo *cluster)
> - {
> - int dbnum;
> - FILE *script = NULL;
> - boolfound = false;
> - charoutput_path[MAXPGPATH];
> -
> - prep_status("Checking for invalid indexes from concurrent index
> builds");
> -
> - snprintf(output_path, sizeof(output_path), "invalid_indexes.txt");
> -
> - for (dbnum = 0; dbnum < cluster->dbarr.ndbs; dbnum++)
> - {
> - PGresult *res;
> - booldb_used = false;
> - int ntups;
> - int rowno;
> - int i_nspname,
> - i_relname;
> - DbInfo *active_db = &cluster->dbarr.dbs[dbnum];
> - PGconn *conn = connectToServer(cluster, active_db->db_name);
> -
> - res = executeQueryOrDie(conn,
> - "SELECT
> n.nspname, c.relname "
> - "FROM
> pg_catalog.pg_class c, "
> - "
> pg_catalog.pg_namespace n, "
> - "
> pg_catalog.pg_index i "
> - "WHERE
> (i.indisvalid = false OR "
> - "
> i.indisready = false) AND "
> - "
> i.indexrelid = c.oid AND "
> - "
> c.relnamespace = n.oid AND "
> - /* we do not
> migrate thes
Re: [HACKERS] Fix for pg_upgrade and invalid indexes
On 2013-03-29 19:03:05 -0400, Tom Lane wrote: > Andres Freund writes: > > Those columns cannot be NULL, so using IS DISTINCT FROM seems a bit > > clumsy. > > That was what I started to write, too, but actually I think the IS > DISTINCT is correct and the RIGHT JOIN should be a LEFT JOIN. Note > that the query appears to be intended to collect regular tables as > well as indexes. (As patched, that's totally broken, so I infer > Bruce hasn't tested it yet.) Ah yes. Then I'd actually find it much more readable to formulate it as a NOT EXISTS(), but that might be just me. Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Fix for pg_upgrade and invalid indexes
On Fri, Mar 29, 2013 at 07:03:05PM -0400, Tom Lane wrote:
> Andres Freund writes:
> > Those columns cannot be NULL, so using IS DISTINCT FROM seems a bit
> > clumsy.
>
> That was what I started to write, too, but actually I think the IS
> DISTINCT is correct and the RIGHT JOIN should be a LEFT JOIN. Note
> that the query appears to be intended to collect regular tables as
> well as indexes. (As patched, that's totally broken, so I infer
> Bruce hasn't tested it yet.)
Yes, I only ran my simple tests so far --- I wanted to at least get some
eyes on it. I was wondering if we ever need to use parentheses for
queries that mix normal and outer joins? I am unclear on that.
Attached is a fixed patch that uses LEFT JOIN. I went back and looked
at the patch that added this test and I think the patch is now complete.
I would like to apply it tomorrow/Saturday so it will be ready for
Monday's packaging, and get some buildfarm time on it.
--
Bruce Momjian http://momjian.us
EnterpriseDB http://enterprisedb.com
+ It's impossible for everything to be true. +
diff --git a/contrib/pg_upgrade/check.c b/contrib/pg_upgrade/check.c
new file mode 100644
index 65fb548..35783d0
*** a/contrib/pg_upgrade/check.c
--- b/contrib/pg_upgrade/check.c
*** static void check_is_super_user(ClusterI
*** 20,26
static void check_for_prepared_transactions(ClusterInfo *cluster);
static void check_for_isn_and_int8_passing_mismatch(ClusterInfo *cluster);
static void check_for_reg_data_type_usage(ClusterInfo *cluster);
- static void check_for_invalid_indexes(ClusterInfo *cluster);
static void get_bin_version(ClusterInfo *cluster);
static char *get_canonical_locale_name(int category, const char *locale);
--- 20,25
*** check_and_dump_old_cluster(bool live_che
*** 97,103
check_is_super_user(&old_cluster);
check_for_prepared_transactions(&old_cluster);
check_for_reg_data_type_usage(&old_cluster);
- check_for_invalid_indexes(&old_cluster);
check_for_isn_and_int8_passing_mismatch(&old_cluster);
/* old = PG 8.3 checks? */
--- 96,101
*** check_for_reg_data_type_usage(ClusterInf
*** 952,1046
"%s\n\n", output_path);
}
else
- check_ok();
- }
-
-
- /*
- * check_for_invalid_indexes()
- *
- * CREATE INDEX CONCURRENTLY can create invalid indexes if the index build
- * fails. These are dumped as valid indexes by pg_dump, but the
- * underlying files are still invalid indexes. This checks to make sure
- * no invalid indexes exist, either failed index builds or concurrent
- * indexes in the process of being created.
- */
- static void
- check_for_invalid_indexes(ClusterInfo *cluster)
- {
- int dbnum;
- FILE *script = NULL;
- bool found = false;
- char output_path[MAXPGPATH];
-
- prep_status("Checking for invalid indexes from concurrent index builds");
-
- snprintf(output_path, sizeof(output_path), "invalid_indexes.txt");
-
- for (dbnum = 0; dbnum < cluster->dbarr.ndbs; dbnum++)
- {
- PGresult *res;
- bool db_used = false;
- int ntups;
- int rowno;
- int i_nspname,
- i_relname;
- DbInfo *active_db = &cluster->dbarr.dbs[dbnum];
- PGconn *conn = connectToServer(cluster, active_db->db_name);
-
- res = executeQueryOrDie(conn,
- "SELECT n.nspname, c.relname "
- "FROM pg_catalog.pg_class c, "
- " pg_catalog.pg_namespace n, "
- " pg_catalog.pg_index i "
- "WHERE (i.indisvalid = false OR "
- " i.indisready = false) AND "
- " i.indexrelid = c.oid AND "
- " c.relnamespace = n.oid AND "
- /* we do not migrate these, so skip them */
- " n.nspname != 'pg_catalog' AND "
- " n.nspname != 'information_schema' AND "
- /* indexes do not have toast tables */
- " n.nspname != 'pg_toast'");
-
- ntups = PQntuples(res);
- i_nspname = PQfnumber(res, "nspname");
- i_relname = PQfnumber(res, "relname");
- for (rowno = 0; rowno < ntups; rowno++)
- {
- found = true;
- if (script == NULL && (script = fopen_priv(output_path, "w")) == NULL)
- pg_log(PG_FATAL, "Could not open file \"%s\": %s\n",
- output_path, getErrorText(errno));
- if (!db_used)
- {
- fprintf(script, "Database: %s\n", active_db->db_name);
- db_used = true;
- }
- fprintf(script, " %s.%s\n",
- PQgetvalue(res, rowno, i_nspname),
- PQgetvalue(res, rowno, i_relname));
- }
-
- PQclear(res);
-
- PQfinish(conn);
- }
-
- if (script)
- fclose(script);
-
- if (found)
- {
- pg_log(PG_REPORT, "fatal\n");
- pg_log(PG_FATAL,
- "Your installation contains invalid indexes due to failed or\n"
- "currently running CREATE INDEX CONCURRENTLY operations. You\n"
- "cannot upgrade until these indexes are valid or removed. A\n"
- "list of the problem indexes is in the file:\n"
- "%s\n\n", output_
Re: [HACKERS] Fix for pg_upgrade and invalid indexes
Andres Freund writes: > Those columns cannot be NULL, so using IS DISTINCT FROM seems a bit > clumsy. That was what I started to write, too, but actually I think the IS DISTINCT is correct and the RIGHT JOIN should be a LEFT JOIN. Note that the query appears to be intended to collect regular tables as well as indexes. (As patched, that's totally broken, so I infer Bruce hasn't tested it yet.) regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Fix for pg_upgrade and invalid indexes
On 2013-03-29 16:57:06 -0400, Bruce Momjian wrote:
> On Thu, Mar 28, 2013 at 05:27:28PM -0400, Tom Lane wrote:
> > Bruce Momjian writes:
> > > Should I just patch pg_upgrade to remove the "indisvalid", skip
> > > "indisvalid" indexes, and backpatch it? Users should be using the
> > > version of pg_upgrade to match new pg_dump. Is there any case where
> > > they don't match? Do I still need to check for "indisready"?
> >
> > Yeah, if you can just ignore !indisvalid indexes that should work fine.
> > I see no need to look at indisready if you're doing that.
>
> Attached is a patch that implements the suggested pg_upgrade changes of
> not copying invalid indexes now that pg_dump doesn't dump them. This
> should be backpatched back to 8.4 to match pg_dump. It might require
> release note updates; not sure. Previously pg_upgrade threw an error
> if invalid indexes exist, but only since February, when we released the
> pg_upgrade fix to do this. You can see the majority of this patch is
> removing that check.
> + "RIGHT OUTER JOIN pg_catalog.pg_index i "
> + " ON (c.oid = i.indexrelid) "
>"WHERE relkind IN ('r', 'm', 'i'%s) AND "
> + /* pg_dump only dumps valid indexes; testing
> indisready is
> + * necessary in 9.2, and harmless in earlier/later
> versions. */
> + " i.indisvalid IS DISTINCT FROM false AND "
> + " i.indisready IS DISTINCT FROM false AND "
> /* exclude possible orphaned temp tables */
>" ((n.nspname !~ '^pg_temp_' AND "
>"n.nspname !~ '^pg_toast_temp_' AND "
Those columns cannot be NULL, so using IS DISTINCT FROM seems a bit
clumsy.
Greetings,
Andres Freund
--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Fix for pg_upgrade and invalid indexes
Bruce Momjian writes: > Attached is a patch that implements the suggested pg_upgrade changes of > not copying invalid indexes now that pg_dump doesn't dump them. This > should be backpatched back to 8.4 to match pg_dump. It might require > release note updates; not sure. Previously pg_upgrade threw an error > if invalid indexes exist, but only since February, when we released the > pg_upgrade fix to do this. You can see the majority of this patch is > removing that check. Surely that should be LEFT JOIN not RIGHT JOIN? regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] Fix for pg_upgrade and invalid indexes
On Thu, Mar 28, 2013 at 05:27:28PM -0400, Tom Lane wrote:
> Bruce Momjian writes:
> > Should I just patch pg_upgrade to remove the "indisvalid", skip
> > "indisvalid" indexes, and backpatch it? Users should be using the
> > version of pg_upgrade to match new pg_dump. Is there any case where
> > they don't match? Do I still need to check for "indisready"?
>
> Yeah, if you can just ignore !indisvalid indexes that should work fine.
> I see no need to look at indisready if you're doing that.
Attached is a patch that implements the suggested pg_upgrade changes of
not copying invalid indexes now that pg_dump doesn't dump them. This
should be backpatched back to 8.4 to match pg_dump. It might require
release note updates; not sure. Previously pg_upgrade threw an error
if invalid indexes exist, but only since February, when we released the
pg_upgrade fix to do this. You can see the majority of this patch is
removing that check.
--
Bruce Momjian http://momjian.us
EnterpriseDB http://enterprisedb.com
+ It's impossible for everything to be true. +
diff --git a/contrib/pg_upgrade/check.c b/contrib/pg_upgrade/check.c
new file mode 100644
index 65fb548..35783d0
*** a/contrib/pg_upgrade/check.c
--- b/contrib/pg_upgrade/check.c
*** static void check_is_super_user(ClusterI
*** 20,26
static void check_for_prepared_transactions(ClusterInfo *cluster);
static void check_for_isn_and_int8_passing_mismatch(ClusterInfo *cluster);
static void check_for_reg_data_type_usage(ClusterInfo *cluster);
- static void check_for_invalid_indexes(ClusterInfo *cluster);
static void get_bin_version(ClusterInfo *cluster);
static char *get_canonical_locale_name(int category, const char *locale);
--- 20,25
*** check_and_dump_old_cluster(bool live_che
*** 97,103
check_is_super_user(&old_cluster);
check_for_prepared_transactions(&old_cluster);
check_for_reg_data_type_usage(&old_cluster);
- check_for_invalid_indexes(&old_cluster);
check_for_isn_and_int8_passing_mismatch(&old_cluster);
/* old = PG 8.3 checks? */
--- 96,101
*** check_for_reg_data_type_usage(ClusterInf
*** 952,1046
"%s\n\n", output_path);
}
else
- check_ok();
- }
-
-
- /*
- * check_for_invalid_indexes()
- *
- * CREATE INDEX CONCURRENTLY can create invalid indexes if the index build
- * fails. These are dumped as valid indexes by pg_dump, but the
- * underlying files are still invalid indexes. This checks to make sure
- * no invalid indexes exist, either failed index builds or concurrent
- * indexes in the process of being created.
- */
- static void
- check_for_invalid_indexes(ClusterInfo *cluster)
- {
- int dbnum;
- FILE *script = NULL;
- bool found = false;
- char output_path[MAXPGPATH];
-
- prep_status("Checking for invalid indexes from concurrent index builds");
-
- snprintf(output_path, sizeof(output_path), "invalid_indexes.txt");
-
- for (dbnum = 0; dbnum < cluster->dbarr.ndbs; dbnum++)
- {
- PGresult *res;
- bool db_used = false;
- int ntups;
- int rowno;
- int i_nspname,
- i_relname;
- DbInfo *active_db = &cluster->dbarr.dbs[dbnum];
- PGconn *conn = connectToServer(cluster, active_db->db_name);
-
- res = executeQueryOrDie(conn,
- "SELECT n.nspname, c.relname "
- "FROM pg_catalog.pg_class c, "
- " pg_catalog.pg_namespace n, "
- " pg_catalog.pg_index i "
- "WHERE (i.indisvalid = false OR "
- " i.indisready = false) AND "
- " i.indexrelid = c.oid AND "
- " c.relnamespace = n.oid AND "
- /* we do not migrate these, so skip them */
- " n.nspname != 'pg_catalog' AND "
- " n.nspname != 'information_schema' AND "
- /* indexes do not have toast tables */
- " n.nspname != 'pg_toast'");
-
- ntups = PQntuples(res);
- i_nspname = PQfnumber(res, "nspname");
- i_relname = PQfnumber(res, "relname");
- for (rowno = 0; rowno < ntups; rowno++)
- {
- found = true;
- if (script == NULL && (script = fopen_priv(output_path, "w")) == NULL)
- pg_log(PG_FATAL, "Could not open file \"%s\": %s\n",
- output_path, getErrorText(errno));
- if (!db_used)
- {
- fprintf(script, "Database: %s\n", active_db->db_name);
- db_used = true;
- }
- fprintf(script, " %s.%s\n",
- PQgetvalue(res, rowno, i_nspname),
- PQgetvalue(res, rowno, i_relname));
- }
-
- PQclear(res);
-
- PQfinish(conn);
- }
-
- if (script)
- fclose(script);
-
- if (found)
- {
- pg_log(PG_REPORT, "fatal\n");
- pg_log(PG_FATAL,
- "Your installation contains invalid indexes due to failed or\n"
- "currently running CREATE INDEX CONCURRENTLY operations. You\n"
- "cannot upgrade until these indexes are valid or removed. A\n"
- "list of the problem indexes is in the file:\n"
- "%s\n\n", output_path
Re: [HACKERS] Fix for pg_upgrade status display
On Wed, Dec 5, 2012 at 10:04:53PM -0500, Bruce Momjian wrote: > Pg_upgrade displays file names during copy and database names during > dump/restore. Andrew Dunstan identified three bugs: > > * long file names were being truncated to 60 _leading_ characters, which > often do not change for long file names > > * file names were truncated to 60 characters in log files > > * carriage returns were being output to log files > > The attached patch fixes these --- it prints 60 _trailing_ characters to > the status display, and full path names without carriage returns to log > files. Patch applied. It also suppresses status output to the log file unless verbose mode is used. -- Bruce Momjian http://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. + -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Fix for pg_upgrade status display
On Thu, Dec 6, 2012 at 02:53:44PM -0300, Alvaro Herrera wrote: > Robert Haas escribió: > > On Wed, Dec 5, 2012 at 10:04 PM, Bruce Momjian wrote: > > > Pg_upgrade displays file names during copy and database names during > > > dump/restore. Andrew Dunstan identified three bugs: > > > > > > * long file names were being truncated to 60 _leading_ characters, which > > > often do not change for long file names > > > > > > * file names were truncated to 60 characters in log files > > > > > > * carriage returns were being output to log files > > > > > > The attached patch fixes these --- it prints 60 _trailing_ characters to > > > the status display, and full path names without carriage returns to log > > > files. > > > > This might be a dumb question, but why limit it to 60 characters at > > all instead of, say, MAXPGPATH? > > I think this should be keyed off the terminal width, actually, no? The > whole point of this is to overwrite the same line over and over, right? That seems like overkill for a status message. It is just there so users know pg_upgrade isn't stuck, which was the complaint before the message was used. -- Bruce Momjian http://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. + -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Fix for pg_upgrade status display
On Thu, Dec 6, 2012 at 12:43:53PM -0500, Robert Haas wrote: > On Wed, Dec 5, 2012 at 10:04 PM, Bruce Momjian wrote: > > Pg_upgrade displays file names during copy and database names during > > dump/restore. Andrew Dunstan identified three bugs: > > > > * long file names were being truncated to 60 _leading_ characters, which > > often do not change for long file names > > > > * file names were truncated to 60 characters in log files > > > > * carriage returns were being output to log files > > > > The attached patch fixes these --- it prints 60 _trailing_ characters to > > the status display, and full path names without carriage returns to log > > files. > > This might be a dumb question, but why limit it to 60 characters at > all instead of, say, MAXPGPATH? It is limited to 60 only for screen display, so the user knows what is being processed. If the text wraps across several lines, the \r trick to overwrite the string will not work. -- Bruce Momjian http://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. + -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Fix for pg_upgrade status display
Robert Haas escribió: > On Wed, Dec 5, 2012 at 10:04 PM, Bruce Momjian wrote: > > Pg_upgrade displays file names during copy and database names during > > dump/restore. Andrew Dunstan identified three bugs: > > > > * long file names were being truncated to 60 _leading_ characters, which > > often do not change for long file names > > > > * file names were truncated to 60 characters in log files > > > > * carriage returns were being output to log files > > > > The attached patch fixes these --- it prints 60 _trailing_ characters to > > the status display, and full path names without carriage returns to log > > files. > > This might be a dumb question, but why limit it to 60 characters at > all instead of, say, MAXPGPATH? I think this should be keyed off the terminal width, actually, no? The whole point of this is to overwrite the same line over and over, right? -- Álvaro Herrerahttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Fix for pg_upgrade status display
On Wed, Dec 5, 2012 at 10:04 PM, Bruce Momjian wrote: > Pg_upgrade displays file names during copy and database names during > dump/restore. Andrew Dunstan identified three bugs: > > * long file names were being truncated to 60 _leading_ characters, which > often do not change for long file names > > * file names were truncated to 60 characters in log files > > * carriage returns were being output to log files > > The attached patch fixes these --- it prints 60 _trailing_ characters to > the status display, and full path names without carriage returns to log > files. This might be a dumb question, but why limit it to 60 characters at all instead of, say, MAXPGPATH? -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] Fix for pg_upgrade status display
Pg_upgrade displays file names during copy and database names during
dump/restore. Andrew Dunstan identified three bugs:
* long file names were being truncated to 60 _leading_ characters, which
often do not change for long file names
* file names were truncated to 60 characters in log files
* carriage returns were being output to log files
The attached patch fixes these --- it prints 60 _trailing_ characters to
the status display, and full path names without carriage returns to log
files.
--
Bruce Momjian http://momjian.us
EnterpriseDB http://enterprisedb.com
+ It's impossible for everything to be true. +
diff --git a/contrib/pg_upgrade/dump.c b/contrib/pg_upgrade/dump.c
new file mode 100644
index 2c1b65b..f35852b
*** a/contrib/pg_upgrade/dump.c
--- b/contrib/pg_upgrade/dump.c
*** generate_old_dump(void)
*** 36,42
char file_name[MAXPGPATH];
DbInfo *old_db = &old_cluster.dbarr.dbs[dbnum];
! pg_log(PG_REPORT, OVERWRITE_MESSAGE, old_db->db_name);
snprintf(file_name, sizeof(file_name), DB_DUMP_FILE_MASK, old_db->db_oid);
exec_prog(RESTORE_LOG_FILE, NULL, true,
--- 36,42
char file_name[MAXPGPATH];
DbInfo *old_db = &old_cluster.dbarr.dbs[dbnum];
! pg_log(PG_STATUS, "%s", old_db->db_name);
snprintf(file_name, sizeof(file_name), DB_DUMP_FILE_MASK, old_db->db_oid);
exec_prog(RESTORE_LOG_FILE, NULL, true,
diff --git a/contrib/pg_upgrade/pg_upgrade.c b/contrib/pg_upgrade/pg_upgrade.c
new file mode 100644
index 63df529..2d4b678
*** a/contrib/pg_upgrade/pg_upgrade.c
--- b/contrib/pg_upgrade/pg_upgrade.c
*** create_new_objects(void)
*** 310,316
char file_name[MAXPGPATH];
DbInfo *old_db = &old_cluster.dbarr.dbs[dbnum];
! pg_log(PG_REPORT, OVERWRITE_MESSAGE, old_db->db_name);
snprintf(file_name, sizeof(file_name), DB_DUMP_FILE_MASK, old_db->db_oid);
/*
--- 310,316
char file_name[MAXPGPATH];
DbInfo *old_db = &old_cluster.dbarr.dbs[dbnum];
! pg_log(PG_STATUS, "%s", old_db->db_name);
snprintf(file_name, sizeof(file_name), DB_DUMP_FILE_MASK, old_db->db_oid);
/*
diff --git a/contrib/pg_upgrade/pg_upgrade.h b/contrib/pg_upgrade/pg_upgrade.h
new file mode 100644
index d981035..972e8e9
*** a/contrib/pg_upgrade/pg_upgrade.h
--- b/contrib/pg_upgrade/pg_upgrade.h
***
*** 24,32
#define MIGRATOR_API_VERSION 1
! #define MESSAGE_WIDTH "60"
- #define OVERWRITE_MESSAGE " %-" MESSAGE_WIDTH "." MESSAGE_WIDTH "s\r"
#define GET_MAJOR_VERSION(v) ((v) / 100)
/* contains both global db information and CREATE DATABASE commands */
--- 24,31
#define MIGRATOR_API_VERSION 1
! #define MESSAGE_WIDTH 60
#define GET_MAJOR_VERSION(v) ((v) / 100)
/* contains both global db information and CREATE DATABASE commands */
*** typedef enum
*** 208,213
--- 207,213
typedef enum
{
PG_VERBOSE,
+ PG_STATUS,
PG_REPORT,
PG_WARNING,
PG_FATAL
diff --git a/contrib/pg_upgrade/relfilenode.c b/contrib/pg_upgrade/relfilenode.c
new file mode 100644
index 14e66df..5fec5ad
*** a/contrib/pg_upgrade/relfilenode.c
--- b/contrib/pg_upgrade/relfilenode.c
*** transfer_relfile(pageCnvCtx *pageConvert
*** 213,219
unlink(new_file);
/* Copying files might take some time, so give feedback. */
! pg_log(PG_REPORT, OVERWRITE_MESSAGE, old_file);
if ((user_opts.transfer_mode == TRANSFER_MODE_LINK) && (pageConverter != NULL))
pg_log(PG_FATAL, "This upgrade requires page-by-page conversion, "
--- 213,219
unlink(new_file);
/* Copying files might take some time, so give feedback. */
! pg_log(PG_STATUS, "%s", old_file);
if ((user_opts.transfer_mode == TRANSFER_MODE_LINK) && (pageConverter != NULL))
pg_log(PG_FATAL, "This upgrade requires page-by-page conversion, "
diff --git a/contrib/pg_upgrade/util.c b/contrib/pg_upgrade/util.c
new file mode 100644
index 0c1eccc..e3fdf3a
*** a/contrib/pg_upgrade/util.c
--- b/contrib/pg_upgrade/util.c
*** prep_status(const char *fmt,...)
*** 75,81
if (strlen(message) > 0 && message[strlen(message) - 1] == '\n')
pg_log(PG_REPORT, "%s", message);
else
! pg_log(PG_REPORT, "%-" MESSAGE_WIDTH "s", message);
}
--- 75,82
if (strlen(message) > 0 && message[strlen(message) - 1] == '\n')
pg_log(PG_REPORT, "%s", message);
else
! /* trim strings that don't end in a newline */
! pg_log(PG_REPORT, "%-*s", MESSAGE_WIDTH, message);
}
*** pg_log(eLogType type, char *fmt,...)
*** 93,110
/* fopen() on log_opts.internal might have failed, so check it */
if ((type != PG_VERBOSE || log_opts.verbose) && log_opts.internal != NULL)
{
! /*
! * There's nothing much we can do about it if fwrite fails, but some
! * platforms declare fwrite with warn_unused_result. Do a little
! * dance with casting to void to sh
[HACKERS] Fix for pg_upgrade with non-writeable cwd
An EntpriseDB testing report indicated that pg_upgrade crashes if it
can't write into the current directory. This only happens in 9.2 and
head, so I have applied the attached patch to fix it.
--
Bruce Momjian http://momjian.us
EnterpriseDB http://enterprisedb.com
+ It's impossible for everything to be true. +
diff --git a/contrib/pg_upgrade/util.c b/contrib/pg_upgrade/util.c
new file mode 100644
index 1c71204..2c0dfd6
*** a/contrib/pg_upgrade/util.c
--- b/contrib/pg_upgrade/util.c
*** pg_log(eLogType type, char *fmt,...)
*** 78,84
va_end(args);
/* PG_VERBOSE is only output in verbose mode */
! if (type != PG_VERBOSE || log_opts.verbose)
{
fwrite(message, strlen(message), 1, log_opts.internal);
/* if we are using OVERWRITE_MESSAGE, add newline to log file */
--- 78,85
va_end(args);
/* PG_VERBOSE is only output in verbose mode */
! /* fopen() on log_opts.internal might have failed, so check it */
! if ((type != PG_VERBOSE || log_opts.verbose) && log_opts.internal != NULL)
{
fwrite(message, strlen(message), 1, log_opts.internal);
/* if we are using OVERWRITE_MESSAGE, add newline to log file */
-
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] Fix for pg_upgrade tablespace function usage
I have applied the attached patch to git head to fix the new SQL tablespace location function usage in pg_upgrade to properly check cluster version numbers, and a fix missing table alias. I found this problem during testing. -- Bruce Momjian http://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. + diff --git a/contrib/pg_upgrade/info.c b/contrib/pg_upgrade/info.c new file mode 100644 index e8361ce..692cdc2 *** a/contrib/pg_upgrade/info.c --- b/contrib/pg_upgrade/info.c *** get_db_infos(ClusterInfo *cluster) *** 204,210 /* we don't preserve pg_database.oid so we sort by name */ "ORDER BY 2", /* 9.2 removed the spclocation column */ ! (GET_MAJOR_VERSION(old_cluster.major_version) <= 901) ? "t.spclocation" : "pg_catalog.pg_tablespace_location(t.oid) AS spclocation"); res = executeQueryOrDie(conn, "%s", query); --- 204,210 /* we don't preserve pg_database.oid so we sort by name */ "ORDER BY 2", /* 9.2 removed the spclocation column */ ! (GET_MAJOR_VERSION(cluster->major_version) <= 901) ? "t.spclocation" : "pg_catalog.pg_tablespace_location(t.oid) AS spclocation"); res = executeQueryOrDie(conn, "%s", query); *** get_rel_infos(ClusterInfo *cluster, DbIn *** 287,293 /* we preserve pg_class.oid so we sort by it to match old/new */ "ORDER BY 1;", /* 9.2 removed the spclocation column */ !(GET_MAJOR_VERSION(old_cluster.major_version) <= 901) ? "t.spclocation" : "pg_catalog.pg_tablespace_location(t.oid) AS spclocation", /* see the comment at the top of old_8_3_create_sequence_script() */ (GET_MAJOR_VERSION(old_cluster.major_version) <= 803) ? --- 287,293 /* we preserve pg_class.oid so we sort by it to match old/new */ "ORDER BY 1;", /* 9.2 removed the spclocation column */ !(GET_MAJOR_VERSION(cluster->major_version) <= 901) ? "t.spclocation" : "pg_catalog.pg_tablespace_location(t.oid) AS spclocation", /* see the comment at the top of old_8_3_create_sequence_script() */ (GET_MAJOR_VERSION(old_cluster.major_version) <= 803) ? diff --git a/contrib/pg_upgrade/tablespace.c b/contrib/pg_upgrade/tablespace.c new file mode 100644 index 11fd9d0..6b61f4b *** a/contrib/pg_upgrade/tablespace.c --- b/contrib/pg_upgrade/tablespace.c *** get_tablespace_paths(void) *** 53,59 " spcname != 'pg_global'", /* 9.2 removed the spclocation column */ (GET_MAJOR_VERSION(old_cluster.major_version) <= 901) ? ! "t.spclocation" : "pg_catalog.pg_tablespace_location(oid) AS spclocation"); res = executeQueryOrDie(conn, "%s", query); --- 53,59 " spcname != 'pg_global'", /* 9.2 removed the spclocation column */ (GET_MAJOR_VERSION(old_cluster.major_version) <= 901) ? ! "spclocation" : "pg_catalog.pg_tablespace_location(oid) AS spclocation"); res = executeQueryOrDie(conn, "%s", query); -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] fix for pg_upgrade
OK, works now with the recent update. Thanks -- View this message in context: http://postgresql.1045698.n5.nabble.com/fix-for-pg-upgrade-tp3411128p5059777.html Sent from the PostgreSQL - hackers mailing list archive at Nabble.com. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] fix for pg_upgrade
Great, thanks! -- View this message in context: http://postgresql.1045698.n5.nabble.com/fix-for-pg-upgrade-tp3411128p4856336.html Sent from the PostgreSQL - hackers mailing list archive at Nabble.com. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] fix for pg_upgrade
> Alvaro Herrera wrote: > > > > Excerpts from Bruce Momjian's message of mi sep 28 13:48:28 -0300 > > 2011: > > > Bruce Momjian wrote: > > > > OK, so it fails for all tables and you are using the newest version. > > > > Thanks for all your work. I am now guessing that pg_upgrade 9.1.X is > > > > just broken on Windows. > > > > > > > > Perhaps the variables set by pg_upgrade_support.so are not being passed > > > > into the server variables? I know pg_upgrade 9.0.X worked on Windows > > > > because EnterpriseDB did extensive testing recently on this. Has > > > > anyone used pg_upgrade 9.1.X on Windows? > > > > > > OK, I have a new theory. postmaster.c processes the -b > > > (binary-upgrade) flag by setting a C variable: > > > > > > case 'b': > > > /* Undocumented flag used for binary upgrades */ > > > IsBinaryUpgrade = true; > > > break; > > > > > > I am now wondering if this variable is not being passed down to the > > > sessions during Win32's EXEC_BACKEND. Looking at the other postmaster > > > settings, these set GUC variables, which I assume are passed down. Can > > > someone confirm this? > > > > Well, you could compile it with -DEXEC_BACKEND to test it for yourself. > > > > > How should this be fixed? > > > > Maybe it should be part of struct BackendParameters. > > Thanks. That's what I did, and tested the failure with -DEXEC_BACKEND, > and the fix with the patch, which is attached. I am confident this will > fix Windows as well. Applied, and backpatched to 9.1.X. Thanks for the report. The fix will be in 9.1.2. -- Bruce Momjian http://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. + -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] fix for pg_upgrade
Alvaro Herrera wrote: > > Excerpts from Bruce Momjian's message of mi?? sep 28 13:48:28 -0300 2011: > > Bruce Momjian wrote: > > > OK, so it fails for all tables and you are using the newest version. > > > Thanks for all your work. I am now guessing that pg_upgrade 9.1.X is > > > just broken on Windows. > > > > > > Perhaps the variables set by pg_upgrade_support.so are not being passed > > > into the server variables? I know pg_upgrade 9.0.X worked on Windows > > > because EnterpriseDB did extensive testing recently on this. Has > > > anyone used pg_upgrade 9.1.X on Windows? > > > > OK, I have a new theory. postmaster.c processes the -b > > (binary-upgrade) flag by setting a C variable: > > > > case 'b': > > /* Undocumented flag used for binary upgrades */ > > IsBinaryUpgrade = true; > > break; > > > > I am now wondering if this variable is not being passed down to the > > sessions during Win32's EXEC_BACKEND. Looking at the other postmaster > > settings, these set GUC variables, which I assume are passed down. Can > > someone confirm this? > > Well, you could compile it with -DEXEC_BACKEND to test it for yourself. > > > How should this be fixed? > > Maybe it should be part of struct BackendParameters. Thanks. That's what I did, and tested the failure with -DEXEC_BACKEND, and the fix with the patch, which is attached. I am confident this will fix Windows as well. -- Bruce Momjian http://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. + diff --git a/src/backend/postmaster/postmaster.c b/src/backend/postmaster/postmaster.c new file mode 100644 index 94b57fa..0a84d97 *** a/src/backend/postmaster/postmaster.c --- b/src/backend/postmaster/postmaster.c *** typedef struct *** 433,438 --- 433,439 TimestampTz PgStartTime; TimestampTz PgReloadTime; bool redirection_done; + bool IsBinaryUpgrade; #ifdef WIN32 HANDLE PostmasterHandle; HANDLE initial_signal_pipe; *** save_backend_variables(BackendParameters *** 4653,4658 --- 4654,4660 param->PgReloadTime = PgReloadTime; param->redirection_done = redirection_done; + param->IsBinaryUpgrade = IsBinaryUpgrade; #ifdef WIN32 param->PostmasterHandle = PostmasterHandle; *** restore_backend_variables(BackendParamet *** 4874,4879 --- 4876,4882 PgReloadTime = param->PgReloadTime; redirection_done = param->redirection_done; + IsBinaryUpgrade = param->IsBinaryUpgrade; #ifdef WIN32 PostmasterHandle = param->PostmasterHandle; -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] fix for pg_upgrade
Excerpts from Bruce Momjian's message of mié sep 28 13:48:28 -0300 2011: > Bruce Momjian wrote: > > OK, so it fails for all tables and you are using the newest version. > > Thanks for all your work. I am now guessing that pg_upgrade 9.1.X is > > just broken on Windows. > > > > Perhaps the variables set by pg_upgrade_support.so are not being passed > > into the server variables? I know pg_upgrade 9.0.X worked on Windows > > because EnterpriseDB did extensive testing recently on this. Has > > anyone used pg_upgrade 9.1.X on Windows? > > OK, I have a new theory. postmaster.c processes the -b > (binary-upgrade) flag by setting a C variable: > > case 'b': > /* Undocumented flag used for binary upgrades */ > IsBinaryUpgrade = true; > break; > > I am now wondering if this variable is not being passed down to the > sessions during Win32's EXEC_BACKEND. Looking at the other postmaster > settings, these set GUC variables, which I assume are passed down. Can > someone confirm this? Well, you could compile it with -DEXEC_BACKEND to test it for yourself. > How should this be fixed? Maybe it should be part of struct BackendParameters. -- Álvaro Herrera The PostgreSQL Company - Command Prompt, Inc. PostgreSQL Replication, Consulting, Custom Development, 24x7 support -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] fix for pg_upgrade
panam wrote: > Hi Bruce, > > here is the file you asked for: > http://postgresql.1045698.n5.nabble.com/file/n4850735/pg_upgrade_logfile.txt > pg_upgrade_logfile.txt > OK, I see it using -b to pg_ctl: ""C:\Program Files\PostgreSQL\9.1\bin/pg_ctl" -w -l "nul" -D "D:\applications\postgres\9.1" -o "-p 5432 -b" start >> "nul" 2>&1" What I have to find out is whether this is passed to the individual session processes. I guess is no. > I guess you are not addressing me here, right? > > The server will need to > > be started with -b and this will disable autovacuum. Can someone on > > Windows try this? No, not really. I think it is a software bug and I need guidance about a solution. -- Bruce Momjian http://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. + -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] fix for pg_upgrade
Hi Bruce, here is the file you asked for: http://postgresql.1045698.n5.nabble.com/file/n4850735/pg_upgrade_logfile.txt pg_upgrade_logfile.txt I guess you are not addressing me here, right? > The server will need to > be started with -b and this will disable autovacuum. Can someone on > Windows try this? Thanks panam -- View this message in context: http://postgresql.1045698.n5.nabble.com/fix-for-pg-upgrade-tp3411128p4850735.html Sent from the PostgreSQL - hackers mailing list archive at Nabble.com. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] fix for pg_upgrade
Bruce Momjian wrote: > OK, so it fails for all tables and you are using the newest version. > Thanks for all your work. I am now guessing that pg_upgrade 9.1.X is > just broken on Windows. > > Perhaps the variables set by pg_upgrade_support.so are not being passed > into the server variables? I know pg_upgrade 9.0.X worked on Windows > because EnterpriseDB did extensive testing recently on this. Has > anyone used pg_upgrade 9.1.X on Windows? OK, I have a new theory. postmaster.c processes the -b (binary-upgrade) flag by setting a C variable: case 'b': /* Undocumented flag used for binary upgrades */ IsBinaryUpgrade = true; break; I am now wondering if this variable is not being passed down to the sessions during Win32's EXEC_BACKEND. Looking at the other postmaster settings, these set GUC variables, which I assume are passed down. Can someone confirm this? How should this be fixed? FYI, the binary-upgrade set() functions will not operate unless the -b option is enabled, which explains the failure the reporter is seeing. -- Bruce Momjian http://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. + -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] fix for pg_upgrade
panam wrote:
> Here are all generated log files.
>
> I just removed all other DBs except gnucash (which includes the accounts
> table), but the issue also emerges with other DBs.
> Upgraded the 9.1 instance to the new build (9.1.1.) as well but this
> apparently did not change anything.
> PG versions are (including generated logs):
> PostgreSQL 9.0.4, compiled by Visual C++ build 1500, 64-bit
> PostgreSQL 9.1.0, compiled by Visual C++ build 1500, 64-bit:
> http://postgresql.1045698.n5.nabble.com/file/n4848829/pg_upgrade_9.1.0.zip
> pg_upgrade_9.1.0.zip
> PostgreSQL 9.1.1, compiled by Visual C++ build 1500, 64-bit:
> http://postgresql.1045698.n5.nabble.com/file/n4848829/pg_upgrade_9.1.1.zip
> pg_upgrade_9.1.1.zip
> I hope that is what you meant with "pg_upgrade log file".
OK, so it fails for all tables and you are using the newest version.
Thanks for all your work. I am now guessing that pg_upgrade 9.1.X is
just broken on Windows.
Perhaps the variables set by pg_upgrade_support.so are not being passed
into the server variables? I know pg_upgrade 9.0.X worked on Windows
because EnterpriseDB did extensive testing recently on this. Has
anyone used pg_upgrade 9.1.X on Windows?
As far as a log file, you need you to use '-l log' and email me that
file.
As far as testing, I wonder if we need to load in pg_upgrade_support on
Windows, and rerun some of the pg_dumpall SQL create table statements to
see why the pg_class.oid and others are not getting set. For example,
this:
-- For binary upgrade, must preserve pg_class oids
SELECT
binary_upgrade.set_next_heap_pg_class_oid('465783'::pg_catalog.oid);
SELECT
binary_upgrade.set_next_toast_pg_class_oid('465786'::pg_catalog.oid);
SELECT
binary_upgrade.set_next_index_pg_class_oid('465788'::pg_catalog.oid);
CREATE TABLE accounts (
guid character varying(32) NOT NULL,
name character varying(2048) NOT NULL,
account_type character varying(2048) NOT NULL,
commodity_guid character varying(32),
commodity_scu integer NOT NULL,
non_std_scu integer NOT NULL,
parent_guid character varying(32),
code character varying(2048),
description character varying(2048),
hidden integer,
placeholder integer
);
should set the accounts pg_class.oid as 465783. The server will need to
be started with -b and this will disable autovacuum. Can someone on
Windows try this?
--
Bruce Momjian http://momjian.us
EnterpriseDB http://enterprisedb.com
+ It's impossible for everything to be true. +
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] fix for pg_upgrade
Here are all generated log files. I just removed all other DBs except gnucash (which includes the accounts table), but the issue also emerges with other DBs. Upgraded the 9.1 instance to the new build (9.1.1.) as well but this apparently did not change anything. PG versions are (including generated logs): PostgreSQL 9.0.4, compiled by Visual C++ build 1500, 64-bit PostgreSQL 9.1.0, compiled by Visual C++ build 1500, 64-bit: http://postgresql.1045698.n5.nabble.com/file/n4848829/pg_upgrade_9.1.0.zip pg_upgrade_9.1.0.zip PostgreSQL 9.1.1, compiled by Visual C++ build 1500, 64-bit: http://postgresql.1045698.n5.nabble.com/file/n4848829/pg_upgrade_9.1.1.zip pg_upgrade_9.1.1.zip I hope that is what you meant with "pg_upgrade log file". Regards, panam -- View this message in context: http://postgresql.1045698.n5.nabble.com/fix-for-pg-upgrade-tp3411128p4848829.html Sent from the PostgreSQL - hackers mailing list archive at Nabble.com. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] fix for pg_upgrade
panam wrote:
> Hi Bruce,
>
> here is the whole dump (old DB):
> http://postgresql.1045698.n5.nabble.com/file/n4844725/dump.txt dump.txt
Wow, that is interesting. I see this in the dump output:
-- For binary upgrade, must preserve relfilenodes
SELECT
binary_upgrade.set_next_heap_relfilenode('465783'::pg_catalog.oid);
SELECT
binary_upgrade.set_next_toast_relfilenode('465786'::pg_catalog.oid);
SELECT
binary_upgrade.set_next_index_relfilenode('465788'::pg_catalog.oid);
CREATE TABLE accounts (
guid character varying(32) NOT NULL,
name character varying(2048) NOT NULL,
account_type character varying(2048) NOT NULL,
commodity_guid character varying(32),
commodity_scu integer NOT NULL,
non_std_scu integer NOT NULL,
parent_guid character varying(32),
code character varying(2048),
description character varying(2048),
hidden integer,
placeholder integer
);
and it is clearly saying the oid/relfilenode should be 465783, but your
9.1 query shows:
C:\Program Files\PostgreSQL\9.1\bin>psql -c "select * from pg_class
where oid = 465783 or oid = 16505;" -p 5433 -U postgres
relname | relnamespace | reltype | reloftype | relowner | relam |
relfilenode | reltablespace | relpages | reltuples | reltoastrelid |
reltoastidxid | relhasindex | relisshared | relpersistence | relkind | relnatts
| relchecks | relhasoids | relhaspkey | relhasrules | relhastriggers |
relhassubclass | relfrozenxid | relacl | reloptions
--+--+-+---+--+---+-+---+--+---+---+---+-+-++-+--+---+++-+++--++
accounts | 2200 | 16507 | 0 |16417 | 0 |
16505 | 0 |0 | 0 | 16508 | 0
| t | f | p | r | 11 | 0 |
f | t | f | f | f |
3934366 ||
(1 row)
and 9.0 says correctly 465783:
C:\Program Files\PostgreSQL\9.0\bin>psql -c "select * from pg_class
where oid = 465783 or oid = 16505;" -p 5432 -U postgres
relname | relnamespace | reltype | reloftype | relowner | relam |
relfilenode | reltablespace | relpages | reltuples | reltoastrelid |
reltoastidxid | relhasindex | relisshared | relistemp | relkind | relnatts |
relchecks | relhasoids | relhaspkey | relhasexclusion | relhasrules |
relhastriggers | relhassubclass | relfrozenxid | relacl | reloptions
--+--+-+---+--+---+-+---+--+---+---+---+-+-+---+-+--+---+++-+-+++--++
accounts | 465781 | 465785 | 0 | 456619 | 0 |
465783 | 0 |3 | 122 |465786 | 0
| t | f | f | r | 11 | 0 | f
| t | f | f | f | f
| 3934366 ||
(1 row)
It is as though the system ignoring the set_next_heap_relfilenode()
call, but I don't see how that could happen. I don't see any other
'accounts' table in that dump.
My only guess at this point is that somehow the -b/IsBinaryUpgrade flag
is not being processed or regognized, and hence the binary_upgrade 'set'
routines are not working.
Is this 9.1 final or later? Can you turn on debug mode and send me the
pg_upgrade log file that is generated? I am going go look for the
pg_ctl -o '-b' flag. Are all databases/objects failing or just this
one?
--
Bruce Momjian http://momjian.us
EnterpriseDB http://enterprisedb.com
+ It's impossible for everything to be true. +
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] fix for pg_upgrade
Hi Bruce, here is the whole dump (old DB): http://postgresql.1045698.n5.nabble.com/file/n4844725/dump.txt dump.txt Regards, panam -- View this message in context: http://postgresql.1045698.n5.nabble.com/fix-for-pg-upgrade-tp3411128p4844725.html Sent from the PostgreSQL - hackers mailing list archive at Nabble.com. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] fix for pg_upgrade
Hi Bruce, on the old DB I've got 465783 as oid whereas on the new one it is 16505. is not in the dump file (old db), even 16385 (i guess this is a typo here) or 16505 are not. The only line in which 465783 could be found is Is that enough information or should I send the whole dump? That's a bit of work as I have to expunge some sensitive schema data, or is there a meaningful way to just do the dump for a single db? Thanks & regards, panam -- View this message in context: http://postgresql.1045698.n5.nabble.com/fix-for-pg-upgrade-tp3411128p4843289.html Sent from the PostgreSQL - hackers mailing list archive at Nabble.com. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] fix for pg_upgrade
panam wrote: > Hi Bruce, > > on the old DB I've got 465783 as oid whereas on the new one it is 16505. > > is not in the dump file (old db), even 16385 (i guess this is a typo here) > or 16505 are not. > The only line in which 465783 could be found is I need to see the lines after this. > Is that enough information or should I send the whole dump? That's a bit of > work as I have to expunge some sensitive schema data, or is there a > meaningful way to just do the dump for a single db? You can do: pg_dump --binary-upgrade --schema-only dbname -- Bruce Momjian http://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. + -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] fix for pg_upgrade
panam wrote:
> OK, i started once again:
>
>
> I hope the following is the correct way of querying the table corresponding
> to a relid:
>
>
>
>
>
>
>
>
>
> --
> View this message in context:
> http://postgresql.1045698.n5.nabble.com/fix-for-pg-upgrade-tp3411128p4838427.html
Yes, that is very close to what I needed. Ideally you would have
included the oid from pg_class:
select oid, * from pg_class where oid = 465783 or oid = 16505
Can you supply that?
Also can you email me privately the following output from the old
database? It should only be the schema and not your data:
pg_dumpall --schema-only --binary-upgrade
I am looking for something like this in the file:
-- For binary upgrade, must preserve pg_class oids
SELECT
binary_upgrade.set_next_heap_pg_class_oid('16385'::pg_catalog.oid);
CREATE TABLE test (
x integer
);
but for your case it would be the 'accounts' file. You can email just
those lines if you want, and that you can probably email to hackers.
Thanks.
--
Bruce Momjian http://momjian.us
EnterpriseDB http://enterprisedb.com
+ It's impossible for everything to be true. +
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] fix for pg_upgrade
OK, i started once again: I hope the following is the correct way of querying the table corresponding to a relid: -- View this message in context: http://postgresql.1045698.n5.nabble.com/fix-for-pg-upgrade-tp3411128p4838427.html Sent from the PostgreSQL - hackers mailing list archive at Nabble.com. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] fix for pg_upgrade
\panam wrote: > Hi, just tried to upgrade from 9.0 to 9.1 and got this error during > pg_upgrade : > Mismatch of relation id: database "xyz", old relid 465783, new relid 16494 > It seems, I get this error on every table as I got it on another table > (which I did not need and deleted) before as well. Schmemas seem to be > migrated but the content is missing. > > I am using Windows 7 64bit (both PG servers are 64 bit as well), everthing > on the same machine. Sorry for the delay in replying. It is odd you got a mismatch of relids because pg_upgrade is supposed to preserve all of those. Can you do a query to find out what table is relid of 465783 on the old cluster? -- Bruce Momjian http://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. + -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] fix for pg_upgrade
Hi, just tried to upgrade from 9.0 to 9.1 and got this error during pg_upgrade : Mismatch of relation id: database "xyz", old relid 465783, new relid 16494 It seems, I get this error on every table as I got it on another table (which I did not need and deleted) before as well. Schmemas seem to be migrated but the content is missing. I am using Windows 7 64bit (both PG servers are 64 bit as well), everthing on the same machine. Any ideas? Thanks & regards panam -- View this message in context: http://postgresql.1045698.n5.nabble.com/fix-for-pg-upgrade-tp3411128p4798957.html Sent from the PostgreSQL - hackers mailing list archive at Nabble.com. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Fix for pg_upgrade user flag
On 05/07/2011 06:48 PM, Bruce Momjian wrote: "postgres" is not compiled in. It's whatever user you run initdb under. In particular, in the regression tests, it is probably not "postgres". Thanks. I get confused because the 'postgres' database is hardcoded in, but not the username. Not sure why I am so easily confused. There is a requirement for a known database name, but no requirement for a known superuser name. cheers andrew -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Fix for pg_upgrade user flag
Peter Eisentraut wrote: > On l?r, 2011-05-07 at 13:50 -0400, Bruce Momjian wrote: > > I was really wondering if I should be using that hard-coded name, > > rather than allowing the user to supply it. They have to compile in a > > different name, and I assume that name is accessible somewhere. > > "postgres" is not compiled in. It's whatever user you run initdb under. > In particular, in the regression tests, it is probably not "postgres". Thanks. I get confused because the 'postgres' database is hardcoded in, but not the username. Not sure why I am so easily confused. -- Bruce Momjian http://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. + -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Fix for pg_upgrade user flag
Kevin Grittner wrote:
> > Bruce Momjian wrote:
> > Tom Lane wrote:
> >> Bruce Momjian writes:
> >>> One question I have is why we even bother to allow the database
> >>> username to be specified? Shouldn't we just hard-code that to
> >>> 'postgres'?
> >>
> >> Only if you want to render pg_upgrade unusable by a significant
> >> fraction of people. "postgres" is not the hard wired name of the
> >> bootstrap superuser.
> >
> > I was really wondering if I should be using that hard-coded name,
> > rather than allowing the user to supply it. They have to compile in
> > a different name, and I assume that name is accessible somewhere.
>
> At home, on my ubuntu machine, I built and ran initdb without
> specifying a superuser. It used my OS login of "kevin". Then,
>
> test=# \du
> List of roles
> Role name | Attributes | Member
> of
> ---++
> ---
> kevin | Superuser, Create role, Create DB, Replication | {}
>
> test=# create user xxx superuser;
> CREATE ROLE
> test=# \c test xxx
> You are now connected to database "test" as user "xxx".
> test=# alter user kevin rename to yyy;
> ALTER ROLE
> test=# \du
> List of roles
> Role name | Attributes | Member
> of
> ---++
> ---
> xxx | Superuser, Replication | {}
> yyy | Superuser, Create role, Create DB, Replication | {}
>
> If I run pg_upgrade now, what will it pick? How?
Good point --- you would need the user flag in pg_upgrade. Thanks.
--
Bruce Momjian http://momjian.us
EnterpriseDB http://enterprisedb.com
+ It's impossible for everything to be true. +
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Fix for pg_upgrade user flag
On lör, 2011-05-07 at 13:50 -0400, Bruce Momjian wrote: > I was really wondering if I should be using that hard-coded name, > rather than allowing the user to supply it. They have to compile in a > different name, and I assume that name is accessible somewhere. "postgres" is not compiled in. It's whatever user you run initdb under. In particular, in the regression tests, it is probably not "postgres". -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Fix for pg_upgrade user flag
> Bruce Momjian wrote:
> Tom Lane wrote:
>> Bruce Momjian writes:
>>> One question I have is why we even bother to allow the database
>>> username to be specified? Shouldn't we just hard-code that to
>>> 'postgres'?
>>
>> Only if you want to render pg_upgrade unusable by a significant
>> fraction of people. "postgres" is not the hard wired name of the
>> bootstrap superuser.
>
> I was really wondering if I should be using that hard-coded name,
> rather than allowing the user to supply it. They have to compile in
> a different name, and I assume that name is accessible somewhere.
At home, on my ubuntu machine, I built and ran initdb without
specifying a superuser. It used my OS login of "kevin". Then,
test=# \du
List of roles
Role name | Attributes | Member
of
---++
---
kevin | Superuser, Create role, Create DB, Replication | {}
test=# create user xxx superuser;
CREATE ROLE
test=# \c test xxx
You are now connected to database "test" as user "xxx".
test=# alter user kevin rename to yyy;
ALTER ROLE
test=# \du
List of roles
Role name | Attributes | Member
of
---++
---
xxx | Superuser, Replication | {}
yyy | Superuser, Create role, Create DB, Replication | {}
If I run pg_upgrade now, what will it pick? How?
-Kevin
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Fix for pg_upgrade user flag
Tom Lane wrote: > Bruce Momjian writes: > > One question I have is why we even bother to allow the database username > > to be specified? Shouldn't we just hard-code that to 'postgres'? > > Only if you want to render pg_upgrade unusable by a significant fraction > of people. "postgres" is not the hard wired name of the bootstrap > superuser. I was really wondering if I should be using that hard-coded name, rather than allowing the user to supply it. They have to compile in a different name, and I assume that name is accessible somewhere. -- Bruce Momjian http://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. + -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Fix for pg_upgrade user flag
Bruce Momjian writes: > One question I have is why we even bother to allow the database username > to be specified? Shouldn't we just hard-code that to 'postgres'? Only if you want to render pg_upgrade unusable by a significant fraction of people. "postgres" is not the hard wired name of the bootstrap superuser. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Fix for pg_upgrade user flag
Robert Haas wrote: > On Sat, May 7, 2011 at 9:50 AM, Robert Haas wrote: > > On Sat, May 7, 2011 at 8:56 AM, Bruce Momjian wrote: > >> The attached, applied patch checks that the pg_upgrade user specified is > >> a super-user. ?It also reports the error message when the post-pg_ctl > >> connection fails. > >> > >> This was prompted by a private bug report from EnterpriseDB. > > > > It strikes me that it's fairly crazy to think you're going to be able > > to catch all the possible errors the server might throw this way. > > Don't we need some way of letting the actual server errors leak out? > > Or, hmm. Maybe you just did that. If so, never mind. :-) What I did was to report the errors of our first database probe after we started the server --- for some reason, that code was not reporting the libpq error message, while all other failed connections did. The second change was to only run pg_upgrade as a database super-user, and hopefully that will avoid odd pg_dump error messages. One question I have is why we even bother to allow the database username to be specified? Shouldn't we just hard-code that to 'postgres'? Is there any reason to allow another username to be used? You can't drop the postgres user but you can remove super-user permissions from it so maybe we have to continue allowing it: postgres=> drop user postgres; ERROR: cannot drop role postgres because it is required by the database system postgres=> alter user postgres nosuperuser; ALTER ROLE -- Bruce Momjian http://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. + -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Fix for pg_upgrade user flag
On Sat, May 7, 2011 at 9:50 AM, Robert Haas wrote: > On Sat, May 7, 2011 at 8:56 AM, Bruce Momjian wrote: >> The attached, applied patch checks that the pg_upgrade user specified is >> a super-user. It also reports the error message when the post-pg_ctl >> connection fails. >> >> This was prompted by a private bug report from EnterpriseDB. > > It strikes me that it's fairly crazy to think you're going to be able > to catch all the possible errors the server might throw this way. > Don't we need some way of letting the actual server errors leak out? Or, hmm. Maybe you just did that. If so, never mind. :-) -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Fix for pg_upgrade user flag
On Sat, May 7, 2011 at 8:56 AM, Bruce Momjian wrote: > The attached, applied patch checks that the pg_upgrade user specified is > a super-user. It also reports the error message when the post-pg_ctl > connection fails. > > This was prompted by a private bug report from EnterpriseDB. It strikes me that it's fairly crazy to think you're going to be able to catch all the possible errors the server might throw this way. Don't we need some way of letting the actual server errors leak out? -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] Fix for pg_upgrade user flag
The attached, applied patch checks that the pg_upgrade user specified is
a super-user. It also reports the error message when the post-pg_ctl
connection fails.
This was prompted by a private bug report from EnterpriseDB.
--
Bruce Momjian http://momjian.us
EnterpriseDB http://enterprisedb.com
+ It's impossible for everything to be true. +
diff --git a/contrib/pg_upgrade/check.c b/contrib/pg_upgrade/check.c
new file mode 100644
index 35b178e..26dec39
*** a/contrib/pg_upgrade/check.c
--- b/contrib/pg_upgrade/check.c
*** static void check_new_cluster_is_empty(v
*** 15,20
--- 15,21
static void check_old_cluster_has_new_cluster_dbs(void);
static void check_locale_and_encoding(ControlData *oldctrl,
ControlData *newctrl);
+ static void check_is_super_user(ClusterInfo *cluster);
static void check_for_isn_and_int8_passing_mismatch(ClusterInfo *cluster);
static void check_for_reg_data_type_usage(ClusterInfo *cluster);
*** check_old_cluster(bool live_check,
*** 63,69
/*
* Check for various failure cases
*/
!
check_for_reg_data_type_usage(&old_cluster);
check_for_isn_and_int8_passing_mismatch(&old_cluster);
--- 64,70
/*
* Check for various failure cases
*/
! check_is_super_user(&old_cluster);
check_for_reg_data_type_usage(&old_cluster);
check_for_isn_and_int8_passing_mismatch(&old_cluster);
*** create_script_for_old_cluster_deletion(
*** 473,478
--- 474,505
/*
+ * check_is_super_user()
+ *
+ * Make sure we are the super-user.
+ */
+ static void
+ check_is_super_user(ClusterInfo *cluster)
+ {
+ PGresult *res;
+ PGconn *conn = connectToServer(cluster, "template1");
+
+ /* Can't use pg_authid because only superusers can view it. */
+ res = executeQueryOrDie(conn,
+ "SELECT rolsuper "
+ "FROM pg_catalog.pg_roles "
+ "WHERE rolname = current_user");
+
+ if (PQntuples(res) != 1 || strcmp(PQgetvalue(res, 0, 0), "t") != 0)
+ pg_log(PG_FATAL, "the database user is not a superuser\n");
+
+ PQclear(res);
+
+ PQfinish(conn);
+ }
+
+
+ /*
* check_for_isn_and_int8_passing_mismatch()
*
* /contrib/isn relies on data type int8, and in 8.4 int8 can now be passed
diff --git a/contrib/pg_upgrade/server.c b/contrib/pg_upgrade/server.c
new file mode 100644
index 9a55075..d6efe9a
*** a/contrib/pg_upgrade/server.c
--- b/contrib/pg_upgrade/server.c
*** connectToServer(ClusterInfo *cluster, co
*** 27,33
if (conn == NULL || PQstatus(conn) != CONNECTION_OK)
{
! pg_log(PG_REPORT, "Connection to database failed: %s\n",
PQerrorMessage(conn));
if (conn)
--- 27,33
if (conn == NULL || PQstatus(conn) != CONNECTION_OK)
{
! pg_log(PG_REPORT, "connection to database failed: %s\n",
PQerrorMessage(conn));
if (conn)
*** start_postmaster(ClusterInfo *cluster)
*** 189,195
if ((conn = get_db_conn(cluster, "template1")) == NULL ||
PQstatus(conn) != CONNECTION_OK)
{
! if (conn)
PQfinish(conn);
pg_log(PG_FATAL, "unable to connect to %s postmaster started with the command: %s\n"
"Perhaps pg_hba.conf was not set to \"trust\".\n",
--- 189,197
if ((conn = get_db_conn(cluster, "template1")) == NULL ||
PQstatus(conn) != CONNECTION_OK)
{
! pg_log(PG_REPORT, "\nconnection to database failed: %s\n",
! PQerrorMessage(conn));
! if (conn)
PQfinish(conn);
pg_log(PG_FATAL, "unable to connect to %s postmaster started with the command: %s\n"
"Perhaps pg_hba.conf was not set to \"trust\".\n",
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] Fix for pg_upgrade with extra new cluster databases
The attached, applied patch adds a check to throw a pg_upgrade error
during the check phase, rather than during the post-check upgrade phase.
Specifically, if someone removes the 'postgres' database from the old
cluster and the new cluster has a 'postgres' database, the number of
databases will not match. We actually could upgrade such a setup, but
it would violate the 1-to-1 mapping of database counts, so we throw an
error instead.
Previously they got an error during the upgrade, and not at the check
stage; PG 9.0.4 does the same.
--
Bruce Momjian http://momjian.us
EnterpriseDB http://enterprisedb.com
+ It's impossible for everything to be true. +
diff --git a/contrib/pg_upgrade/check.c b/contrib/pg_upgrade/check.c
new file mode 100644
index 7472440..469bbdb
*** a/contrib/pg_upgrade/check.c
--- b/contrib/pg_upgrade/check.c
***
*** 11,17
static void set_locale_and_encoding(ClusterInfo *cluster);
! static void check_new_db_is_empty(void);
static void check_locale_and_encoding(ControlData *oldctrl,
ControlData *newctrl);
static void check_for_isn_and_int8_passing_mismatch(ClusterInfo *cluster);
--- 11,18
static void set_locale_and_encoding(ClusterInfo *cluster);
! static void check_new_cluster_is_empty(void);
! static void check_old_cluster_has_new_cluster_dbs(void);
static void check_locale_and_encoding(ControlData *oldctrl,
ControlData *newctrl);
static void check_for_isn_and_int8_passing_mismatch(ClusterInfo *cluster);
*** check_new_cluster(void)
*** 112,118
{
set_locale_and_encoding(&new_cluster);
! check_new_db_is_empty();
check_loadable_libraries();
--- 113,122
{
set_locale_and_encoding(&new_cluster);
! get_db_and_rel_infos(&new_cluster);
!
! check_new_cluster_is_empty();
! check_old_cluster_has_new_cluster_dbs();
check_loadable_libraries();
*** check_locale_and_encoding(ControlData *o
*** 341,352
static void
! check_new_db_is_empty(void)
{
int dbnum;
- bool found = false;
-
- get_db_and_rel_infos(&new_cluster);
for (dbnum = 0; dbnum < new_cluster.dbarr.ndbs; dbnum++)
{
--- 345,353
static void
! check_new_cluster_is_empty(void)
{
int dbnum;
for (dbnum = 0; dbnum < new_cluster.dbarr.ndbs; dbnum++)
{
*** check_new_db_is_empty(void)
*** 358,372
{
/* pg_largeobject and its index should be skipped */
if (strcmp(rel_arr->rels[relnum].nspname, "pg_catalog") != 0)
! {
! found = true;
! break;
! }
}
}
! if (found)
! pg_log(PG_FATAL, "New cluster is not empty; exiting\n");
}
--- 359,394
{
/* pg_largeobject and its index should be skipped */
if (strcmp(rel_arr->rels[relnum].nspname, "pg_catalog") != 0)
! pg_log(PG_FATAL, "New cluster database \"%s\" is not empty\n",
! new_cluster.dbarr.dbs[dbnum].db_name);
}
}
! }
!
!
! /*
! * If someone removes the 'postgres' database from the old cluster and
! * the new cluster has a 'postgres' database, the number of databases
! * will not match. We actually could upgrade such a setup, but it would
! * violate the 1-to-1 mapping of database counts, so we throw an error
! * instead.
! */
! static void
! check_old_cluster_has_new_cluster_dbs(void)
! {
! int old_dbnum, new_dbnum;
!
! for (new_dbnum = 0; new_dbnum < new_cluster.dbarr.ndbs; new_dbnum++)
! {
! for (old_dbnum = 0; old_dbnum < old_cluster.dbarr.ndbs; old_dbnum++)
! if (strcmp(old_cluster.dbarr.dbs[old_dbnum].db_name,
! new_cluster.dbarr.dbs[new_dbnum].db_name) == 0)
! break;
! if (old_dbnum == old_cluster.dbarr.ndbs)
! pg_log(PG_FATAL, "New cluster database \"%s\" does not exist in the old cluster\n",
! new_cluster.dbarr.dbs[new_dbnum].db_name);
! }
}
diff --git a/contrib/pg_upgrade/relfilenode.c b/contrib/pg_upgrade/relfilenode.c
new file mode 100644
index 6fb336c..9a0a3ac
*** a/contrib/pg_upgrade/relfilenode.c
--- b/contrib/pg_upgrade/relfilenode.c
*** transfer_all_new_dbs(DbInfoArr *old_db_a
*** 37,48
prep_status("Restoring user relation files\n");
- /*
- * If the user removed the 'postgres' database from the old cluster,
- * this will cause the database counts to not match and throw an error.
- * We could allow this to work because the new database is empty (we
- * checked), but we don't.
- */
if (old_db_arr->ndbs != new_db_arr->ndbs)
pg_log(PG_FATAL, "old and new clusters have a different number of databases\n");
--- 37,42
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] Fix for pg_upgrade
The attached patches fixes a pg_upgrade crash in 9.0 caused by a new cluster database that doesn't exist in the old cluster; instead throw an error. This was reported to me by EnterpriseDB testing staff. This bug does not exist in git head. -- Bruce Momjian http://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. + diff --git a/contrib/pg_upgrade/relfilenode.c b/contrib/pg_upgrade/relfilenode.c new file mode 100644 index b20ca20..fc36968 *** a/contrib/pg_upgrade/relfilenode.c --- b/contrib/pg_upgrade/relfilenode.c *** transfer_all_new_dbs(migratorContext *ct *** 46,51 --- 46,55 int n_maps; pageCnvCtx *pageConverter = NULL; + if (!old_db) + pg_log(ctx, PG_FATAL, + "the new cluster database %s was not found in the old cluster\n", new_db->db_name); + n_maps = 0; mappings = gen_db_file_maps(ctx, old_db, new_db, &n_maps, old_pgdata, new_pgdata); -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Fix for pg_upgrade discriptor leaks
Tom Lane wrote: > Bruce Momjian writes: > > I have applied the attached patch to fix pg_upgrade file descriptor > > leaks in error paths. > > It seems rather pointless to spend code closing descriptors immediately > before a fatal exit. Well, it is not before a fatal but rather before it returns -1, which might fatal or might not. I also had code in to close file descriptors that was a little too tricky about using a single variable to indicate two things so I cleaned it up. I got a private email report about these so obviously they were confusing. -- Bruce Momjian http://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. + -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Fix for pg_upgrade discriptor leaks
Bruce Momjian writes: > I have applied the attached patch to fix pg_upgrade file descriptor > leaks in error paths. It seems rather pointless to spend code closing descriptors immediately before a fatal exit. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] Fix for pg_upgrade discriptor leaks
I have applied the attached patch to fix pg_upgrade file descriptor
leaks in error paths.
--
Bruce Momjian http://momjian.us
EnterpriseDB http://enterprisedb.com
+ It's impossible for everything to be true. +
diff --git a/contrib/pg_upgrade/check.c b/contrib/pg_upgrade/check.c
index 9ef63ec..1c4847a 100644
*** a/contrib/pg_upgrade/check.c
--- b/contrib/pg_upgrade/check.c
*** check_for_isn_and_int8_passing_mismatch(
*** 515,523
PQfinish(conn);
}
if (found)
{
- fclose(script);
pg_log(PG_REPORT, "fatal\n");
pg_log(PG_FATAL,
"| Your installation contains \"/contrib/isn\" functions\n"
--- 515,525
PQfinish(conn);
}
+ if (script)
+ fclose(script);
+
if (found)
{
pg_log(PG_REPORT, "fatal\n");
pg_log(PG_FATAL,
"| Your installation contains \"/contrib/isn\" functions\n"
*** check_for_reg_data_type_usage(ClusterInf
*** 616,624
PQfinish(conn);
}
if (found)
{
- fclose(script);
pg_log(PG_REPORT, "fatal\n");
pg_log(PG_FATAL,
"| Your installation contains one of the reg* data types in\n"
--- 618,628
PQfinish(conn);
}
+ if (script)
+ fclose(script);
+
if (found)
{
pg_log(PG_REPORT, "fatal\n");
pg_log(PG_FATAL,
"| Your installation contains one of the reg* data types in\n"
diff --git a/contrib/pg_upgrade/file.c b/contrib/pg_upgrade/file.c
index deaca46..803e0a0 100644
*** a/contrib/pg_upgrade/file.c
--- b/contrib/pg_upgrade/file.c
*** pg_scandir_internal(const char *dirname,
*** 302,308
--- 302,311
(size_t) ((name_num + 1) * sizeof(struct dirent *)));
if (*namelist == NULL)
+ {
+ closedir(dirdesc);
return -1;
+ }
entrysize = sizeof(struct dirent) - sizeof(direntry->d_name) +
strlen(direntry->d_name) + 1;
*** pg_scandir_internal(const char *dirname,
*** 310,316
--- 313,322
(*namelist)[name_num] = (struct dirent *) malloc(entrysize);
if ((*namelist)[name_num] == NULL)
+ {
+ closedir(dirdesc);
return -1;
+ }
memcpy((*namelist)[name_num], direntry, entrysize);
diff --git a/contrib/pg_upgrade/server.c b/contrib/pg_upgrade/server.c
index c7684f8..84bd03e 100644
*** a/contrib/pg_upgrade/server.c
--- b/contrib/pg_upgrade/server.c
*** get_major_server_version(ClusterInfo *cl
*** 144,154
if (fscanf(version_fd, "%63s", cluster->major_version_str) == 0 ||
sscanf(cluster->major_version_str, "%d.%d", &integer_version,
&fractional_version) != 2)
- {
pg_log(PG_FATAL, "could not get version from %s\n", datadir);
! fclose(version_fd);
! return 0;
! }
return (100 * integer_version + fractional_version) * 100;
}
--- 144,152
if (fscanf(version_fd, "%63s", cluster->major_version_str) == 0 ||
sscanf(cluster->major_version_str, "%d.%d", &integer_version,
&fractional_version) != 2)
pg_log(PG_FATAL, "could not get version from %s\n", datadir);
!
! fclose(version_fd);
return (100 * integer_version + fractional_version) * 100;
}
diff --git a/contrib/pg_upgrade/version.c b/contrib/pg_upgrade/version.c
index e321538..8ba7e98 100644
*** a/contrib/pg_upgrade/version.c
--- b/contrib/pg_upgrade/version.c
*** new_9_0_populate_pg_largeobject_metadata
*** 62,71
PQfinish(conn);
}
if (found)
{
- if (!check_mode)
- fclose(script);
report_status(PG_WARNING, "warning");
if (check_mode)
pg_log(PG_WARNING, "\n"
--- 62,72
PQfinish(conn);
}
+ if (script)
+ fclose(script);
+
if (found)
{
report_status(PG_WARNING, "warning");
if (check_mode)
pg_log(PG_WARNING, "\n"
diff --git a/contrib/pg_upgrade/version_old_8_3.c b/contrib/pg_upgrade/version_old_8_3.c
index 5b226b2..3ec4b59 100644
*** a/contrib/pg_upgrade/version_old_8_3.c
--- b/contrib/pg_upgrade/version_old_8_3.c
*** old_8_3_check_for_name_data_type_usage(C
*** 87,95
PQfinish(conn);
}
if (found)
{
- fclose(script);
pg_log(PG_REPORT, "fatal\n");
pg_log(PG_FATAL,
"| Your installation contains the \"name\" data type in\n"
--- 87,97
PQfinish(conn);
}
+ if (script)
+ fclose(script);
+
if (found)
{
pg_log(PG_REPORT, "fatal\n");
pg_log(PG_FATAL,
"| Your installation contains the \"name\" data type in\n"
*** old_8_3_check_for_tsquery_usage(ClusterI
*** 175,183
PQfinish(conn);
}
if (found)
{
- fclose(script);
pg_log(PG_REPORT, "fatal\n");
pg_log(PG_FATAL,
"| Your installation contains the \"tsquery\" data type.\n"
--- 177,187
PQfinish(conn);
}
+ if (script)
+ fclose(script);
+
if (found)
{
pg_log(PG_REPORT, "fatal\n");
pg_log(PG_FATAL,
"| Your installation contains the \"tsquery\" data type.\n"
*** old_8_3_rebu
[HACKERS] fix for pg_upgrade
I got a report from someone using pg_upgrade coming from PG 8.3 --- turns out we didn't rename toast tables to match the new relfilenode in pre-8.4, so the attached applied patch avoids the check for these cases. This check is new in pg_upgrade for 9.1. -- Bruce Momjian http://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. + commit 9e5bed2df1693a46dfaed862d7462ba2379f8f79 Author: Bruce Momjian Date: Sat Mar 5 21:12:21 2011 -0500 Restructure pg_upgrade checks because pre-8.4 Postgres did not rename toast file names to match the new relfilenode. diff --git a/contrib/pg_upgrade/info.c b/contrib/pg_upgrade/info.c index a502507..ad7edc4 100644 *** a/contrib/pg_upgrade/info.c --- b/contrib/pg_upgrade/info.c *** gen_db_file_maps(DbInfo *old_db, DbInfo *** 51,59 RelInfo*new_rel = &new_db->rel_arr.rels[relnum]; if (old_rel->reloid != new_rel->reloid) ! pg_log(PG_FATAL, "mismatch of relation id: database \"%s\", old relid %d, new relid %d\n", old_db->db_name, old_rel->reloid, new_rel->reloid); ! create_rel_filename_map(old_pgdata, new_pgdata, old_db, new_db, old_rel, new_rel, maps + num_maps); num_maps++; --- 51,68 RelInfo*new_rel = &new_db->rel_arr.rels[relnum]; if (old_rel->reloid != new_rel->reloid) ! pg_log(PG_FATAL, "Mismatch of relation id: database \"%s\", old relid %d, new relid %d\n", old_db->db_name, old_rel->reloid, new_rel->reloid); ! ! /* toast names were not renamed to match their relfilenodes in pre-8.4 */ ! if (GET_MAJOR_VERSION(old_cluster.major_version) >= 804 && ! (strcmp(old_rel->nspname, new_rel->nspname) != 0 || ! strcmp(old_rel->relname, new_rel->relname) != 0)) ! pg_log(PG_FATAL, "Mismatch of relation names: database \"%s\", " ! "old rel %s.%s, new rel %s.%s\n", ! old_db->db_name, old_rel->nspname, old_rel->relname, ! new_rel->nspname, new_rel->relname); ! create_rel_filename_map(old_pgdata, new_pgdata, old_db, new_db, old_rel, new_rel, maps + num_maps); num_maps++; *** create_rel_filename_map(const char *old_ *** 104,115 /* new_relfilenode will match old and new pg_class.oid */ map->new_relfilenode = new_rel->relfilenode; - if (strcmp(old_rel->nspname, new_rel->nspname) != 0 || - strcmp(old_rel->relname, new_rel->relname) != 0) - pg_log(PG_FATAL, "mismatch of relation id: database \"%s\", old rel %s.%s, new rel %s.%s\n", - old_db->db_name, old_rel->nspname, old_rel->relname, - new_rel->nspname, new_rel->relname); - /* used only for logging and error reporing, old/new are identical */ snprintf(map->nspname, sizeof(map->nspname), "%s", old_rel->nspname); snprintf(map->relname, sizeof(map->relname), "%s", old_rel->relname); --- 113,118 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Fix for pg_upgrade migrating pg_largeobject_metadata
Patch applied.
I did not backpatch to 9.0 because you can't migrate from 9.0 to 9.0
with the same catversion (because of tablespace conflict), and a pre-9.0
migration to 9.0 has not large object permissions to migrate. In
summary, it didn't seem worth the risk, and was hard to test.
---
Bruce Momjian wrote:
> Bruce Momjian wrote:
> > Tom Lane wrote:
> > > Bruce Momjian writes:
> > > > Tom Lane wrote:
> > > >> That isn't going to work. At least not unless you start trying to
> > > >> force
> > > >> roles to have the same OIDs in the new installation.
> > >
> > > > If so I can use the CREATE ROLE ... SYSID clause when doing a binary
> > > > upgrade.
> > >
> > > Oh, I had forgotten we still had that wart in the grammar.
> > > It doesn't actually work:
> > >
> > > else if (strcmp(defel->defname, "sysid") == 0)
> > > {
> > > ereport(NOTICE,
> > > (errmsg("SYSID can no longer be
> > > specified")));
> > > }
> > >
> > > Not sure if it's better to try to make that work again than to add
> > > another hack in pg_upgrade_support. On the whole that's a keyword
> > > I'd rather see us drop someday soon.
> >
> > OK, let me work on adding it to pg_upgrade_support. Glad you saw this.
>
> I have fixed the bug by using pg_upgrade_support. It was a little
> complicated because you need to install the pg_upgrade_support functions
> in the super-user database so it is available when you create the users
> in the first step of restoring the pg_dumpall file.
>
> I am afraid we have to batckpatch this to fix to 9.0 for 9.0 to 9.0
> upgrades. It does not apply when coming from pre-9.0 because there was
> no pg_largeobject_metadata.
>
> For testing I did this:
>
> CREATE DATABASE lo;
> \c lo
> SELECT lo_import('/etc/motd');
> \set loid `psql -qt -c 'select loid from pg_largeobject' lo`
> CREATE ROLE user1;
> CREATE ROLE user2;
> -- force user2 to have a different user id on restore
> DROP ROLE user1;
> GRANT ALL ON LARGE OBJECT :loid TO user2;
>
> The fixed version shows:
>
> lo=> select * from pg_largeobject_metadata;
>lomowner | lomacl
> --+--
> 10 | {postgres=rw/postgres,user2=rw/postgres}
> (1 row)
>
> In the broken version, 'user2' was a raw oid, obviously wrong.
>
> Fortunately this was found during my testing and not reported as a bug
> by a pg_upgrade user.
--
Bruce Momjian http://momjian.us
EnterpriseDB http://enterprisedb.com
+ It's impossible for everything to be true. +
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] Fix for pg_upgrade migrating pg_largeobject_metadata
Bruce Momjian wrote:
> Tom Lane wrote:
> > Bruce Momjian writes:
> > > Tom Lane wrote:
> > >> That isn't going to work. At least not unless you start trying to force
> > >> roles to have the same OIDs in the new installation.
> >
> > > If so I can use the CREATE ROLE ... SYSID clause when doing a binary
> > > upgrade.
> >
> > Oh, I had forgotten we still had that wart in the grammar.
> > It doesn't actually work:
> >
> > else if (strcmp(defel->defname, "sysid") == 0)
> > {
> > ereport(NOTICE,
> > (errmsg("SYSID can no longer be
> > specified")));
> > }
> >
> > Not sure if it's better to try to make that work again than to add
> > another hack in pg_upgrade_support. On the whole that's a keyword
> > I'd rather see us drop someday soon.
>
> OK, let me work on adding it to pg_upgrade_support. Glad you saw this.
I have fixed the bug by using pg_upgrade_support. It was a little
complicated because you need to install the pg_upgrade_support functions
in the super-user database so it is available when you create the users
in the first step of restoring the pg_dumpall file.
I am afraid we have to batckpatch this to fix to 9.0 for 9.0 to 9.0
upgrades. It does not apply when coming from pre-9.0 because there was
no pg_largeobject_metadata.
For testing I did this:
CREATE DATABASE lo;
\c lo
SELECT lo_import('/etc/motd');
\set loid `psql -qt -c 'select loid from pg_largeobject' lo`
CREATE ROLE user1;
CREATE ROLE user2;
-- force user2 to have a different user id on restore
DROP ROLE user1;
GRANT ALL ON LARGE OBJECT :loid TO user2;
The fixed version shows:
lo=> select * from pg_largeobject_metadata;
lomowner | lomacl
--+--
10 | {postgres=rw/postgres,user2=rw/postgres}
(1 row)
In the broken version, 'user2' was a raw oid, obviously wrong.
Fortunately this was found during my testing and not reported as a bug
by a pg_upgrade user.
--
Bruce Momjian http://momjian.us
EnterpriseDB http://enterprisedb.com
+ It's impossible for everything to be true. +
diff --git a/contrib/pg_upgrade/dump.c b/contrib/pg_upgrade/dump.c
index 52ab481..aba95f4 100644
*** /tmp/pgdiff.20347/YPWaqb_dump.c Wed Jan 5 20:52:07 2011
--- contrib/pg_upgrade/dump.c Wed Jan 5 20:43:29 2011
*** generate_old_dump(void)
*** 33,39
*
*This function splits pg_dumpall output into global values and
*database creation, and per-db schemas. This allows us to create
! *the toast place holders between restoring these two parts of the
*dump. We split on the first "\connect " after a CREATE ROLE
*username match; this is where the per-db restore starts.
*
--- 33,39
*
*This function splits pg_dumpall output into global values and
*database creation, and per-db schemas. This allows us to create
! *the support functions between restoring these two parts of the
*dump. We split on the first "\connect " after a CREATE ROLE
*username match; this is where the per-db restore starts.
*
diff --git a/contrib/pg_upgrade/function.c b/contrib/pg_upgrade/function.c
index 2ab8e4f..5675551 100644
*** /tmp/pgdiff.20347/QpVBHa_function.c Wed Jan 5 20:52:07 2011
--- contrib/pg_upgrade/function.c Wed Jan 5 20:26:33 2011
***
*** 13,35
/*
! * install_support_functions()
*
* pg_upgrade requires some support functions that enable it to modify
* backend behavior.
*/
void
! install_support_functions(void)
{
! int dbnum;
!
! prep_status("Adding support functions to new cluster");
!
! for (dbnum = 0; dbnum < new_cluster.dbarr.ndbs; dbnum++)
! {
! DbInfo *new_db = &new_cluster.dbarr.dbs[dbnum];
! PGconn *conn = connectToServer(&new_cluster,
new_db->db_name);
!
/* suppress NOTICE of dropped objects */
PQclear(executeQueryOrDie(conn,
"SET
client_min_messages = warning;"));
--- 13,28
/*
! * install_db_support_functions()
*
* pg_upgrade requires some support functions that enable it to modify
* backend behavior.
*/
void
! install_db_support_functions(const char *db_name)
{
! PGconn *conn = connectToServer(&new_cluster, db_name);
!
/* suppress NOTICE of dropped objects */
PQclear(executeQueryOrDie(conn,
"SET
client_min_messages = warning;"));
*** install_support_functions(void)
*** 83,91
"RETURNS
