On Sat, Jun 02, 2012 at 05:10:03PM -0400, Tom Lane wrote:
> Bruce Momjian <br...@momjian.us> writes:
> > On Fri, Jun 01, 2012 at 09:52:59AM -0400, Tom Lane wrote:
> >> It seems that pg_upgrade needs a check to make sure that the bootstrap
> >> superuser is named the same in old and new clusters.
>
> > The attached patch adds checks to verify the the old/new servers have
> > the same install-user oid.
>
> That may or may not be a useful check to make, but it's got
> approximately nothing to do with what I was complaining about.
>
> In particular, supposing that the user has given you a username that
> isn't the bootstrap superuser in the new cluster, this patch is not
> going to stop the update script from failing. Because the script is
> then going to try to replace the bootstrap superuser, and that is
> certainly going to give an error.
>
> I see the point of worrying about the install user as well as the
> bootstrap superuser, but wouldn't it be best to insist they be the same?
> Particularly in the new cluster, where if they aren't the same it means
> the user has manually created at least one role in the new cluster,
> which is likely to lead to OID conflicts or worse.
>
> Furthermore, if the bootstrap superusers aren't named the same, your
> patch fails to handle the original complaint. In the case the
> OP mentioned, the old cluster had
> OID 10: "ubuntu"
> some user-defined OID: "postgres"
> and the new cluster had
> OID 10: "postgres"
> If the user tells pg_upgrade to use username postgres, your check will
> not fail AFAICS, but nonetheless things are going to be messed up after
> the upgrade, because some objects and privileges that used to belong to
> the bootstrap superuser will now belong to a non-default superuser,
> whereas what used to belong to the non-default superuser will now belong
> to the bootstrap superuser. That cannot be thought desirable. For one
> reason, in the old installation the postgres role could have been
> dropped (possibly after dropping a few non-builtin objects) whereas the
> "ubuntu" role was pinned. In the new installation, "postgres" is pinned
> and "ubuntu" won't be.
>
> I think the checks that are actually needed here are (1) bootstrap
> superusers are named the same, and (2) there are no roles other than the
> bootstrap superuser in the new cluster.
You are right that it is more complex than I stated, but given the
limited feedback I got on the pg_upgrade/plplython, I figured people
didn't want to hear the details. Here they are:
There are three failure modes for pg_upgrade:
1. check failure
2. schema restore failure
3. silent failure/corruption
Of course, the later items are worse than the earlier ones. The
reporter got a "schema restore failure" while still following the
pg_upgrade instructions. My initial patch changed that #2 error to a #1
error. Tom is right that creating users in the new cluster (against
instructions), can still generate a #2 error if a new/old pg_authid.oid
match, and they are not the install user, but seeing that is something
that is against the instructions, I was going to leave that as a #2.
However, since Tom feels we should check that and make it a #1 failure,
I have added that test to the attached patch.
--
Bruce Momjian <br...@momjian.us> 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 d226f00..9f3dcda
*** a/contrib/pg_upgrade/check.c
--- b/contrib/pg_upgrade/check.c
*************** check_new_cluster(void)
*** 138,144 ****
* We don't restore our own user, so both clusters must match have
* matching install-user oids.
*/
! if (old_cluster.install_user_oid != new_cluster.install_user_oid)
pg_log(PG_FATAL,
"Old and new cluster install users have different values for pg_authid.oid.\n");
--- 138,144 ----
* We don't restore our own user, so both clusters must match have
* matching install-user oids.
*/
! if (old_cluster.install_role_oid != new_cluster.install_role_oid)
pg_log(PG_FATAL,
"Old and new cluster install users have different values for pg_authid.oid.\n");
*************** check_new_cluster(void)
*** 147,153 ****
* defined users might match users defined in the old cluster and
* generate an error during pg_dump restore.
*/
! if (new_cluster.user_count != 1)
pg_log(PG_FATAL, "Only the install user can be defined in the new cluster.\n");
check_for_prepared_transactions(&new_cluster);
--- 147,153 ----
* defined users might match users defined in the old cluster and
* generate an error during pg_dump restore.
*/
! if (new_cluster.role_count != 1)
pg_log(PG_FATAL, "Only the install user can be defined in the new cluster.\n");
check_for_prepared_transactions(&new_cluster);
*************** check_is_super_user(ClusterInfo *cluster
*** 618,624 ****
pg_log(PG_FATAL, "database user \"%s\" is not a superuser\n",
os_info.user);
! cluster->install_user_oid = atooid(PQgetvalue(res, 0, 1));
PQclear(res);
--- 618,624 ----
pg_log(PG_FATAL, "database user \"%s\" is not a superuser\n",
os_info.user);
! cluster->install_role_oid = atooid(PQgetvalue(res, 0, 1));
PQclear(res);
*************** check_is_super_user(ClusterInfo *cluster
*** 629,635 ****
if (PQntuples(res) != 1)
pg_log(PG_FATAL, "could not determine the number of users\n");
! cluster->user_count = atoi(PQgetvalue(res, 0, 0));
PQclear(res);
--- 629,635 ----
if (PQntuples(res) != 1)
pg_log(PG_FATAL, "could not determine the number of users\n");
! cluster->role_count = atoi(PQgetvalue(res, 0, 0));
PQclear(res);
diff --git a/contrib/pg_upgrade/pg_upgrade.h b/contrib/pg_upgrade/pg_upgrade.h
new file mode 100644
index 528eea7..1d7f56c
*** a/contrib/pg_upgrade/pg_upgrade.h
--- b/contrib/pg_upgrade/pg_upgrade.h
*************** typedef struct
*** 230,237 ****
char major_version_str[64]; /* string PG_VERSION of cluster */
uint32 bin_version; /* version returned from pg_ctl */
Oid pg_database_oid; /* OID of pg_database relation */
! Oid install_user_oid; /* OID of connected user */
! Oid user_count; /* number of users defined in the cluster */
char *tablespace_suffix; /* directory specification */
} ClusterInfo;
--- 230,237 ----
char major_version_str[64]; /* string PG_VERSION of cluster */
uint32 bin_version; /* version returned from pg_ctl */
Oid pg_database_oid; /* OID of pg_database relation */
! Oid install_role_oid; /* OID of connected role */
! Oid role_count; /* number of roles defined in the cluster */
char *tablespace_suffix; /* directory specification */
} ClusterInfo;
diff --git a/contrib/pg_upgrade/check.c b/contrib/pg_upgrade/check.c
new file mode 100644
index 2669c09..d226f00
*** a/contrib/pg_upgrade/check.c
--- b/contrib/pg_upgrade/check.c
*************** check_new_cluster(void)
*** 121,137 ****
{
set_locale_and_encoding(&new_cluster);
get_db_and_rel_infos(&new_cluster);
check_new_cluster_is_empty();
- check_for_prepared_transactions(&new_cluster);
check_loadable_libraries();
- check_locale_and_encoding(&old_cluster.controldata, &new_cluster.controldata);
-
if (user_opts.transfer_mode == TRANSFER_MODE_LINK)
check_hard_link();
}
--- 121,156 ----
{
set_locale_and_encoding(&new_cluster);
+ check_locale_and_encoding(&old_cluster.controldata, &new_cluster.controldata);
+
get_db_and_rel_infos(&new_cluster);
check_new_cluster_is_empty();
check_loadable_libraries();
if (user_opts.transfer_mode == TRANSFER_MODE_LINK)
check_hard_link();
+
+ check_is_super_user(&new_cluster);
+
+ /*
+ * We don't restore our own user, so both clusters must match have
+ * matching install-user oids.
+ */
+ if (old_cluster.install_user_oid != new_cluster.install_user_oid)
+ pg_log(PG_FATAL,
+ "Old and new cluster install users have different values for pg_authid.oid.\n");
+
+ /*
+ * We only allow the install user in the new cluster because other
+ * defined users might match users defined in the old cluster and
+ * generate an error during pg_dump restore.
+ */
+ if (new_cluster.user_count != 1)
+ pg_log(PG_FATAL, "Only the install user can be defined in the new cluster.\n");
+
+ check_for_prepared_transactions(&new_cluster);
}
*************** create_script_for_old_cluster_deletion(c
*** 579,585 ****
/*
* check_is_super_user()
*
! * Make sure we are the super-user.
*/
static void
check_is_super_user(ClusterInfo *cluster)
--- 598,604 ----
/*
* check_is_super_user()
*
! * Check we are superuser, and out user id and user count
*/
static void
check_is_super_user(ClusterInfo *cluster)
*************** check_is_super_user(ClusterInfo *cluster
*** 591,597 ****
/* 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");
--- 610,616 ----
/* Can't use pg_authid because only superusers can view it. */
res = executeQueryOrDie(conn,
! "SELECT rolsuper, oid "
"FROM pg_catalog.pg_roles "
"WHERE rolname = current_user");
*************** check_is_super_user(ClusterInfo *cluster
*** 599,604 ****
--- 618,636 ----
pg_log(PG_FATAL, "database user \"%s\" is not a superuser\n",
os_info.user);
+ cluster->install_user_oid = atooid(PQgetvalue(res, 0, 1));
+
+ PQclear(res);
+
+ res = executeQueryOrDie(conn,
+ "SELECT COUNT(*) "
+ "FROM pg_catalog.pg_roles ");
+
+ if (PQntuples(res) != 1)
+ pg_log(PG_FATAL, "could not determine the number of users\n");
+
+ cluster->user_count = atoi(PQgetvalue(res, 0, 0));
+
PQclear(res);
PQfinish(conn);
diff --git a/contrib/pg_upgrade/pg_upgrade.c b/contrib/pg_upgrade/pg_upgrade.c
new file mode 100644
index 465ecdd..ba81823
*** a/contrib/pg_upgrade/pg_upgrade.c
--- b/contrib/pg_upgrade/pg_upgrade.c
***************
*** 29,35 ****
* We control all assignments of pg_enum.oid because these oids are stored
* in user tables as enum values.
*
! * We control all assignments of pg_auth.oid because these oids are stored
* in pg_largeobject_metadata.
*/
--- 29,35 ----
* We control all assignments of pg_enum.oid because these oids are stored
* in user tables as enum values.
*
! * We control all assignments of pg_authid.oid because these oids are stored
* in pg_largeobject_metadata.
*/
diff --git a/contrib/pg_upgrade/pg_upgrade.h b/contrib/pg_upgrade/pg_upgrade.h
new file mode 100644
index 26aa7bb..528eea7
*** a/contrib/pg_upgrade/pg_upgrade.h
--- b/contrib/pg_upgrade/pg_upgrade.h
*************** typedef struct
*** 230,235 ****
--- 230,237 ----
char major_version_str[64]; /* string PG_VERSION of cluster */
uint32 bin_version; /* version returned from pg_ctl */
Oid pg_database_oid; /* OID of pg_database relation */
+ Oid install_user_oid; /* OID of connected user */
+ Oid user_count; /* number of users defined in the cluster */
char *tablespace_suffix; /* directory specification */
} ClusterInfo;
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
