[prev in list] [next in list] [prev in thread] [next in thread] 

List:       pgsql-bugs
Subject:    Re: [BUGS] BUG #11090: Unclear error message in pg_upgrade
From:       Bruce Momjian <bruce () momjian ! us>
Date:       2014-07-30 14:58:03
Message-ID: 20140730145803.GG2791 () momjian ! us
[Download RAW message or body]

On Tue, Jul 29, 2014 at 10:33:17PM -0400, Tom Lane wrote:
> I think you missed my point.  I'm suggesting actually restricting what
> pg_upgrade will accept, so that the error conditions become simpler to
> understand.
> 
> Furthermore, if your description of the restrictions is accurate, then
> restricting to "the user must be the bootstrap superuser in both clusters"
> is actually not an extra restriction.  You said:
> 
> > o  only one user can be defined in the new cluster (most likely the
> > install user)
> 
> If there is only one user in the new cluster, it *is* the bootstrap
> superuser, because that user cannot be deleted.
> 
> > o  the oids of the two users must be the same (likely the install user
> > on the old cluster as well)
> 
> If the OIDs are the same, then the old-cluster user is also the bootstrap
> superuser, because the bootstrap superuser's hard-wired OID is 10.  QED.
> 
> So I think you should get rid of the existing error check and simplify it
> to "user must be bootstrap superuser (ie, OID 10) in both clusters".
> The existing approach adds only confusion, not flexibility.

OK, I had never made this logical conclusion that it had to be the
install user.  Since you can't remove the install user, and the new
cluster can only have one user (the install user), and because we
preserve pg_authid.oid, the old cluster has to be the install user too.

I have developed the 9.5-only patch which simplifies these checks and
error messages;  attached.

> > What I think you actually can do is to run pg_upgrade with a user that
> > is not the install user in either cluster, as long as the oids match.
> 
> I don't think you actually can, and if you could, I frankly would not
> trust the results.

Agreed.

-- 
  Bruce Momjian  <bruce@momjian.us>        http://momjian.us
  EnterpriseDB                             http://enterprisedb.com

  + Everyone has their own god. +

["user.diff" (text/x-diff)]

diff --git a/contrib/pg_upgrade/check.c b/contrib/pg_upgrade/check.c
new file mode 100644
index 79b6c3c..b216e61
*** a/contrib/pg_upgrade/check.c
--- b/contrib/pg_upgrade/check.c
***************
*** 9,14 ****
--- 9,15 ----
  
  #include "postgres_fe.h"
  
+ #include "catalog/pg_authid.h"
  #include "mb/pg_wchar.h"
  #include "pg_upgrade.h"
  
*************** static void check_locale_and_encoding(Co
*** 19,25 ****
  						  ControlData *newctrl);
  static bool equivalent_locale(const char *loca, const char *locb);
  static bool equivalent_encoding(const char *chara, const char *charb);
! static void check_is_super_user(ClusterInfo *cluster);
  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);
--- 20,26 ----
  						  ControlData *newctrl);
  static bool equivalent_locale(const char *loca, const char *locb);
  static bool equivalent_encoding(const char *chara, const char *charb);
! static void check_is_install_user(ClusterInfo *cluster);
  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);
*************** check_and_dump_old_cluster(bool live_che
*** 94,100 ****
  	/*
  	 * Check for various failure cases
  	 */
! 	check_is_super_user(&old_cluster);
  	check_for_prepared_transactions(&old_cluster);
  	check_for_reg_data_type_usage(&old_cluster);
  	check_for_isn_and_int8_passing_mismatch(&old_cluster);
--- 95,101 ----
  	/*
  	 * Check for various failure cases
  	 */
! 	check_is_install_user(&old_cluster);
  	check_for_prepared_transactions(&old_cluster);
  	check_for_reg_data_type_usage(&old_cluster);
  	check_for_isn_and_int8_passing_mismatch(&old_cluster);
*************** check_new_cluster(void)
*** 158,179 ****
  	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_role_oid != new_cluster.install_role_oid)
! 		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.role_count != 1)
! 		pg_fatal("Only the install user can be defined in the new cluster.\n");
  
  	check_for_prepared_transactions(&new_cluster);
  }
--- 159,165 ----
  	if (user_opts.transfer_mode == TRANSFER_MODE_LINK)
  		check_hard_link();
  
! 	check_is_install_user(&new_cluster);
  
  	check_for_prepared_transactions(&new_cluster);
  }
*************** create_script_for_old_cluster_deletion(c
*** 698,714 ****
  
  
  /*
!  *	check_is_super_user()
   *
!  *	Check we are superuser, and output user id and user count
   */
  static void
! check_is_super_user(ClusterInfo *cluster)
  {
  	PGresult   *res;
  	PGconn	   *conn = connectToServer(cluster, "template1");
  
! 	prep_status("Checking database user is a superuser");
  
  	/* Can't use pg_authid because only superusers can view it. */
  	res = executeQueryOrDie(conn,
--- 684,701 ----
  
  
  /*
!  *	check_is_install_user()
   *
!  *	Check we are the install user, and that the new cluster
!  *	has no other users.
   */
  static void
! check_is_install_user(ClusterInfo *cluster)
  {
  	PGresult   *res;
  	PGconn	   *conn = connectToServer(cluster, "template1");
  
! 	prep_status("Checking database user is the install user");
  
  	/* Can't use pg_authid because only superusers can view it. */
  	res = executeQueryOrDie(conn,
*************** check_is_super_user(ClusterInfo *cluster
*** 716,727 ****
  							"FROM pg_catalog.pg_roles "
  							"WHERE rolname = current_user");
  
! 	if (PQntuples(res) != 1 || strcmp(PQgetvalue(res, 0, 0), "t") != 0)
! 		pg_fatal("database user \"%s\" is not a superuser\n",
  				 os_info.user);
  
- 	cluster->install_role_oid = atooid(PQgetvalue(res, 0, 1));
- 
  	PQclear(res);
  
  	res = executeQueryOrDie(conn,
--- 703,718 ----
  							"FROM pg_catalog.pg_roles "
  							"WHERE rolname = current_user");
  
! 	/*
! 	 * We only allow the install user in the new cluster (see comment below)
! 	 * and we preserve pg_authid.oid, so this must be the install user in
! 	 * the old cluster too.
! 	 */
! 	if (PQntuples(res) != 1 ||
! 		atooid(PQgetvalue(res, 0, 1)) != BOOTSTRAP_SUPERUSERID)
! 		pg_fatal("database user \"%s\" is not the install user\n",
  				 os_info.user);
  
  	PQclear(res);
  
  	res = executeQueryOrDie(conn,
*************** check_is_super_user(ClusterInfo *cluster
*** 731,737 ****
  	if (PQntuples(res) != 1)
  		pg_fatal("could not determine the number of users\n");
  
! 	cluster->role_count = atoi(PQgetvalue(res, 0, 0));
  
  	PQclear(res);
  
--- 722,734 ----
  	if (PQntuples(res) != 1)
  		pg_fatal("could not determine the number of users\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 (cluster == &new_cluster && atooid(PQgetvalue(res, 0, 0)) != 1)
! 		pg_fatal("Only the install user can be defined in the new cluster.\n");
  
  	PQclear(res);
  
diff --git a/contrib/pg_upgrade/pg_upgrade.h b/contrib/pg_upgrade/pg_upgrade.h
new file mode 100644
index 0410b02..61e5de0
*** a/contrib/pg_upgrade/pg_upgrade.h
--- b/contrib/pg_upgrade/pg_upgrade.h
*************** typedef struct
*** 256,263 ****
  	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 */
  	const char *tablespace_suffix;		/* directory specification */
  } ClusterInfo;
  
--- 256,261 ----


-- 
Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-bugs


[prev in list] [next in list] [prev in thread] [next in thread]