Re: [HACKERS] Re: [GENERAL] pg_upgrade from 9.0.7 to 9.1.3: duplicate key pg_authid_oid_index

2012-06-13 Thread Bruce Momjian 
On Mon, Jun 04, 2012 at 10:16:45AM -0400, Bruce Momjian wrote:
> > 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.

Applied and back-patched to Postgres 9.1.

-- 
  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] Re: [GENERAL] pg_upgrade from 9.0.7 to 9.1.3: duplicate key pg_authid_oid_index

2012-06-04 Thread Bruce Momjian 
On Sat, Jun 02, 2012 at 05:10:03PM -0400, Tom Lane wrote:
> Bruce Momjian  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  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

Re: [HACKERS] Re: [GENERAL] pg_upgrade from 9.0.7 to 9.1.3: duplicate key pg_authid_oid_index

2012-06-02 Thread Tom Lane 
Bruce Momjian  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.

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] Re: [GENERAL] pg_upgrade from 9.0.7 to 9.1.3: duplicate key pg_authid_oid_index

2012-06-02 Thread Bruce Momjian 
On Fri, Jun 01, 2012 at 09:52:59AM -0400, Tom Lane wrote:
> Bryan Murphy  writes:
> > The old 9.0 cluster was created by ubuntu.  In this cluster there was an
> > ubuntu user with an oid of 10 and a postgres user with an oid of 16386.
> 
> > The new 9.1 cluster was created with a custom build of postgres 9.1. This
> > did not have an ubuntu user, and it had a postgres user with an oid of 10.
> 
> OID 10 is the bootstrap superuser, which is created with the name of the
> operating system user that ran initdb.  So the above does not sound like
> anything to do with custom vs stock builds, but with who did initdb.
> 
> It seems that pg_upgrade needs a check to make sure that the bootstrap
> superuser is named the same in old and new clusters.

[ Thread moved to hackers.]

OK, I have studied this.  First we preserve pg_authid.oid because oids
are stored in pg_largeobject_metadata.  Second, we dumpall all users,
even the install user because (from pg_dumpall.c):

 * We dump CREATE ROLE followed by ALTER ROLE to ensure that the role
 * will acquire the right properties even if it already exists (ie, it
 * won't hurt for the CREATE to fail).  This is particularly important
 * for the role we are connected as, since even with --clean we will
 * have failed to drop it.

So, pg_upgrade has to strip out restoring the install user because that
would cause an error on restore.  That is done in
dump.c::split_old_dump().

The problem is if the old and new install users have different oids, as
the reporter verified.

The attached patch adds checks to verify the the old/new servers have
the same install-user oid.

-- 
  Bruce Momjian  http://momjian.us
  EnterpriseDB http://enterprisedb.com

  + It's impossible for everything to be true. +
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/check.c b/contrib/pg_upgrade/check.c
new file mode 100644
index 2669c09..df77f53
*** a/contrib/pg_upgrade/check.c
--- b/contrib/pg_upgrade/check.c
*** static void set_locale_and_encoding(Clus
*** 16,22 
  static void check_new_cluster_is_empty(void);
  static void check_locale_and_encoding(ControlData *oldctrl,
  		  ControlData *newctrl);
! 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);
--- 16,22 
  static void check_new_cluster_is_empty(void);
  static void check_locale_and_encoding(ControlData *oldctrl,
  		  ControlData *newctrl);
! static void check_is_super_user_get_oid(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_old_cluster(bool live_check, char
*** 69,75 
  	/*
  	 * 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);
--- 69,75 
  	/*
  	 * Check for various failure cases
  	 */
! 	check_is_super_user_get_oid(&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)
*** 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,144 
  {
  	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)
  		chec