Thread: 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 <bruce@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 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",
On Sat, May 7, 2011 at 8:56 AM, Bruce Momjian <bruce@momjian.us> 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
On Sat, May 7, 2011 at 9:50 AM, Robert Haas <robertmhaas@gmail.com> wrote: > On Sat, May 7, 2011 at 8:56 AM, Bruce Momjian <bruce@momjian.us> 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
Robert Haas wrote: > On Sat, May 7, 2011 at 9:50 AM, Robert Haas <robertmhaas@gmail.com> wrote: > > On Sat, May 7, 2011 at 8:56 AM, Bruce Momjian <bruce@momjian.us> 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 systempostgres=> alteruser postgres nosuperuser;ALTER ROLE -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. +
Bruce Momjian <bruce@momjian.us> 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
Tom Lane wrote: > Bruce Momjian <bruce@momjian.us> 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 <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. +
> 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 rolesRole 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 rolesRole 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
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".
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 <bruce@momjian.us> http://momjian.us EnterpriseDB
http://enterprisedb.com
+ It's impossible for everything to be true. +
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 <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. +
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