patch: move dumpUserConfig call in dumpRoles function of pg_dumpall.c
Hello,
The attached patch changes the location of the dumpUserConfig call in
the dumpRoles function of pg_dumpall.
This is related to this thread:
http://archives.postgresql.org/pgsql-hackers/2011-02/msg02359.php
Currently if you use 'ALTER ROLE rolename SET ROLE', pg_dumpall will
dump an 'ALTER ROLE' out right after the 'CREATE ROLE' statement.
Sometimes this will cause a conflict when a dependent role is not yet
created:
--
-- Roles
--
CREATE ROLE a;
ALTER ROLE a WITH NOSUPERUSER INHERIT NOCREATEROLE NOCREATEDB NOLOGIN
NOREPLICATION;
ALTER ROLE a SET role TO 'b';
CREATE ROLE b;
ALTER ROLE b WITH NOSUPERUSER INHERIT NOCREATEROLE NOCREATEDB NOLOGIN
NOREPLICATION;
CREATE ROLE postgres;
ALTER ROLE postgres WITH SUPERUSER INHERIT CREATEROLE CREATEDB LOGIN
NOREPLICATION;
As you can see, role a is set to role b before role b is created.
This patch moves the call to dumpUserConfig to after the loop where
all the roles are created. This produces output like the this:
--
-- Roles
--
CREATE ROLE a;
ALTER ROLE a WITH NOSUPERUSER INHERIT NOCREATEROLE NOCREATEDB NOLOGIN
NOREPLICATION;
CREATE ROLE b;
ALTER ROLE b WITH NOSUPERUSER INHERIT NOCREATEROLE NOCREATEDB NOLOGIN
NOREPLICATION;
CREATE ROLE postgres;
ALTER ROLE postgres WITH SUPERUSER INHERIT CREATEROLE CREATEDB LOGIN
NOREPLICATION;
ALTER ROLE a SET role TO 'b';
Now this dump will succeed upon restore.
This passed all regression tests.
Thanks.
Attachments:
dump_user_config_last.patchtext/x-patch; charset=US-ASCII; name=dump_user_config_last.patchDownload
diff --git a/src/bin/pg_dump/pg_dumpall.c b/src/bin/pg_dump/pg_dumpall.c
new file mode 100644
index b5f64e8..ee597d5
*** a/src/bin/pg_dump/pg_dumpall.c
--- b/src/bin/pg_dump/pg_dumpall.c
*************** dumpRoles(PGconn *conn)
*** 804,814 ****
buf, "ROLE", rolename);
fprintf(OPF, "%s", buf->data);
-
- if (server_version >= 70300)
- dumpUserConfig(conn, rolename);
}
PQclear(res);
fprintf(OPF, "\n\n");
--- 804,815 ----
buf, "ROLE", rolename);
fprintf(OPF, "%s", buf->data);
}
+ if (server_version >= 70300)
+ for (i = 0; i < PQntuples(res); i++)
+ dumpUserConfig(conn, PQgetvalue(res, i, i_rolname));
+
PQclear(res);
fprintf(OPF, "\n\n");
Phil Sorber <phil@omniti.com> writes:
Currently if you use 'ALTER ROLE rolename SET ROLE', pg_dumpall will
dump an 'ALTER ROLE' out right after the 'CREATE ROLE' statement.
I think pg_dumpall is the very least of your problems if you do
something like that. We probably ought to forbid it entirely.
regards, tom lane
On Wed, Jul 27, 2011 at 7:51 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Phil Sorber <phil@omniti.com> writes:
Currently if you use 'ALTER ROLE rolename SET ROLE', pg_dumpall will
dump an 'ALTER ROLE' out right after the 'CREATE ROLE' statement.I think pg_dumpall is the very least of your problems if you do
something like that. We probably ought to forbid it entirely.
Well, we had a long discussion of that on the thread Phil linked to,
and I don't think there was any consensus that forbidding it was the
right thing to do. Phil appears to be trying to implement the
proposal you made here:
http://archives.postgresql.org/pgsql-hackers/2011-01/msg00452.php
...although I don't think that what he did quite matches what you asked for.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
Robert Haas <robertmhaas@gmail.com> writes:
On Wed, Jul 27, 2011 at 7:51 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
I think pg_dumpall is the very least of your problems if you do
something like that. �We probably ought to forbid it entirely.
Well, we had a long discussion of that on the thread Phil linked to,
and I don't think there was any consensus that forbidding it was the
right thing to do.
You're right, I was half-remembering that thread and thinking that
there are a lot of gotchas in doing an ALTER ROLE SET ROLE. Florian
claimed in the thread that he'd never hit one before, but that doesn't
convince me much.
Phil appears to be trying to implement the
proposal you made here:
http://archives.postgresql.org/pgsql-hackers/2011-01/msg00452.php
...although I don't think that what he did quite matches what you asked for.
No, the proposed patch doesn't go nearly far enough to address Florian's
problem. What I was speculating about was moving all the role (and
database) alters to the *end*, so they'd not take effect until after
we'd completed all the restore actions.
regards, tom lane
I have included two patches in this email. The first
(dump_user_config_last_with_set_role.patch) is an extension of my
first patch. In addition to moving the ALTER ROLE statements after the
CREATE ROLE statements it also inserts a SET ROLE after every connect.
It takes the role parameter from the --role command line option. This
fixes the problem of not being able to restore to a database because
of lack of permissions. This is similar to the idea proposed here:
http://archives.postgresql.org/pgsql-hackers/2010-12/msg01046.php
I think this patch is the better one for several reasons. It keeps the
ALTER ROLE and ALTER DATABASE statements in the same sections as their
related CREATE ROLE and CREATE DATABASE statements, thus not
dramatically altering the output of the command. It is a much simpler
non-invasive patch. It's concise and easy to understand. It solves all
the known scenario's that we have discussed previously.
That being said, I understand you have reservations about how it
works. You would prefer to move all of these type's of ALTER
statements to the end of the output. I also would prefer to see the
correct behavior included in the main tree over having my preference
over the changes. To that end I have also included a second patch
(dump_all_configs_last.patch) which handles it the way you prefer.
This creates new sections at the bottom for the ALTER statements.
These two patches are mutually exclusive. I would ask that you at
least consider the first patch before dismissing the idea. However, I
think applying either would would be an acceptable approach.
Thanks for your consideration.
Show quoted text
On Thu, Jul 28, 2011 at 10:06 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Robert Haas <robertmhaas@gmail.com> writes:
On Wed, Jul 27, 2011 at 7:51 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
I think pg_dumpall is the very least of your problems if you do
something like that. We probably ought to forbid it entirely.Well, we had a long discussion of that on the thread Phil linked to,
and I don't think there was any consensus that forbidding it was the
right thing to do.You're right, I was half-remembering that thread and thinking that
there are a lot of gotchas in doing an ALTER ROLE SET ROLE. Florian
claimed in the thread that he'd never hit one before, but that doesn't
convince me much.Phil appears to be trying to implement the
proposal you made here:
http://archives.postgresql.org/pgsql-hackers/2011-01/msg00452.php
...although I don't think that what he did quite matches what you asked for.No, the proposed patch doesn't go nearly far enough to address Florian's
problem. What I was speculating about was moving all the role (and
database) alters to the *end*, so they'd not take effect until after
we'd completed all the restore actions.regards, tom lane
Attachments:
dump_user_config_last_with_set_role.patchtext/x-patch; charset=US-ASCII; name=dump_user_config_last_with_set_role.patchDownload
diff --git a/src/bin/pg_dump/pg_dumpall.c b/src/bin/pg_dump/pg_dumpall.c
new file mode 100644
index b5f64e8..7f2b03e
*** a/src/bin/pg_dump/pg_dumpall.c
--- b/src/bin/pg_dump/pg_dumpall.c
*************** static int server_version;
*** 79,84 ****
--- 79,85 ----
static FILE *OPF;
static char *filename = NULL;
+ static char *use_role = NULL;
int
main(int argc, char *argv[])
*************** main(int argc, char *argv[])
*** 87,93 ****
char *pgport = NULL;
char *pguser = NULL;
char *pgdb = NULL;
- char *use_role = NULL;
enum trivalue prompt_password = TRI_DEFAULT;
bool data_only = false;
bool globals_only = false;
--- 88,93 ----
*************** main(int argc, char *argv[])
*** 443,448 ****
--- 443,452 ----
fprintf(OPF, "\\connect postgres\n\n");
+ if (use_role && server_version >= 80100)
+ fprintf(OPF, "SET ROLE %s;\n",
+ fmtId(use_role));
+
/* Replicate encoding and std_strings in output */
fprintf(OPF, "SET client_encoding = '%s';\n",
pg_encoding_to_char(encoding));
*************** dumpRoles(PGconn *conn)
*** 804,814 ****
buf, "ROLE", rolename);
fprintf(OPF, "%s", buf->data);
-
- if (server_version >= 70300)
- dumpUserConfig(conn, rolename);
}
PQclear(res);
fprintf(OPF, "\n\n");
--- 808,819 ----
buf, "ROLE", rolename);
fprintf(OPF, "%s", buf->data);
}
+ if (server_version >= 70300)
+ for (i = 0; i < PQntuples(res); i++)
+ dumpUserConfig(conn, PQgetvalue(res, i, i_rolname));
+
PQclear(res);
fprintf(OPF, "\n\n");
*************** dumpDatabases(PGconn *conn)
*** 1561,1566 ****
--- 1566,1575 ----
fprintf(OPF, "\\connect %s\n\n", fmtId(dbname));
+ if (use_role && server_version >= 80100)
+ fprintf(OPF, "SET ROLE %s;\n\n",
+ fmtId(use_role));
+
if (filename)
fclose(OPF);
dump_all_configs_last.patchtext/x-patch; charset=US-ASCII; name=dump_all_configs_last.patchDownload
diff --git a/src/bin/pg_dump/pg_dumpall.c b/src/bin/pg_dump/pg_dumpall.c
new file mode 100644
index b5f64e8..d3929f0
*** a/src/bin/pg_dump/pg_dumpall.c
--- b/src/bin/pg_dump/pg_dumpall.c
*************** static void dropTablespaces(PGconn *conn
*** 41,48 ****
static void dumpTablespaces(PGconn *conn);
static void dropDBs(PGconn *conn);
static void dumpCreateDB(PGconn *conn);
! static void dumpDatabaseConfig(PGconn *conn, const char *dbname);
! static void dumpUserConfig(PGconn *conn, const char *username);
static void dumpDbRoleConfig(PGconn *conn);
static void makeAlterConfigCommand(PGconn *conn, const char *arrayitem,
const char *type, const char *name, const char *type2,
--- 41,48 ----
static void dumpTablespaces(PGconn *conn);
static void dropDBs(PGconn *conn);
static void dumpCreateDB(PGconn *conn);
! static void dumpDatabaseConfig(PGconn *conn);
! static void dumpUserConfig(PGconn *conn);
static void dumpDbRoleConfig(PGconn *conn);
static void makeAlterConfigCommand(PGconn *conn, const char *arrayitem,
const char *type, const char *name, const char *type2,
*************** main(int argc, char *argv[])
*** 500,517 ****
/* Dump CREATE DATABASE commands */
if (!globals_only && !roles_only && !tablespaces_only)
dumpCreateDB(conn);
! /* Dump role/database settings */
! if (!tablespaces_only && !roles_only)
{
if (server_version >= 90000)
dumpDbRoleConfig(conn);
}
}
- if (!globals_only && !roles_only && !tablespaces_only)
- dumpDatabases(conn);
-
PQfinish(conn);
if (verbose)
--- 500,524 ----
/* Dump CREATE DATABASE commands */
if (!globals_only && !roles_only && !tablespaces_only)
dumpCreateDB(conn);
+ }
! if (!globals_only && !roles_only && !tablespaces_only)
! dumpDatabases(conn);
!
! if (!data_only && !tablespaces_only && server_version >= 70300)
! {
! dumpUserConfig(conn);
!
! if (!roles_only)
{
+ if (!globals_only)
+ dumpDatabaseConfig(conn);
+
if (server_version >= 90000)
dumpDbRoleConfig(conn);
}
}
PQfinish(conn);
if (verbose)
*************** dumpRoles(PGconn *conn)
*** 804,812 ****
buf, "ROLE", rolename);
fprintf(OPF, "%s", buf->data);
-
- if (server_version >= 70300)
- dumpUserConfig(conn, rolename);
}
PQclear(res);
--- 811,816 ----
*************** dumpCreateDB(PGconn *conn)
*** 1358,1366 ****
fprintf(OPF, "%s", buf->data);
- if (server_version >= 70300)
- dumpDatabaseConfig(conn, dbname);
-
free(fdbname);
}
--- 1362,1367 ----
*************** dumpCreateDB(PGconn *conn)
*** 1375,1418 ****
* Dump database-specific configuration
*/
static void
! dumpDatabaseConfig(PGconn *conn, const char *dbname)
{
! PQExpBuffer buf = createPQExpBuffer();
! int count = 1;
! for (;;)
{
! PGresult *res;
! if (server_version >= 90000)
! printfPQExpBuffer(buf, "SELECT setconfig[%d] FROM pg_db_role_setting WHERE "
! "setrole = 0 AND setdatabase = (SELECT oid FROM pg_database WHERE datname = ", count);
! else
! printfPQExpBuffer(buf, "SELECT datconfig[%d] FROM pg_database WHERE datname = ", count);
! appendStringLiteralConn(buf, dbname, conn);
! if (server_version >= 90000)
! appendPQExpBuffer(buf, ")");
! appendPQExpBuffer(buf, ";");
! res = executeQuery(conn, buf->data);
! if (PQntuples(res) == 1 &&
! !PQgetisnull(res, 0, 0))
! {
! makeAlterConfigCommand(conn, PQgetvalue(res, 0, 0),
! "DATABASE", dbname, NULL, NULL);
! PQclear(res);
! count++;
! }
! else
! {
! PQclear(res);
! break;
}
}
! destroyPQExpBuffer(buf);
}
--- 1376,1450 ----
* Dump database-specific configuration
*/
static void
! dumpDatabaseConfig(PGconn *conn)
{
! PGresult *dbres;
! int i;
! bool shown_header = false;
! if (server_version >= 70100)
! dbres = executeQuery(conn,
! "SELECT datname "
! "FROM pg_database d "
! "WHERE datallowconn ORDER BY 1");
! else
! dbres = executeQuery(conn,
! "SELECT datname "
! "FROM pg_database d "
! "ORDER BY 1");
!
! for (i = 0; i < PQntuples(dbres); i++)
{
! char *dbname = PQgetvalue(dbres, i, 0);
! PQExpBuffer buf = createPQExpBuffer();
! int count = 1;
! for (;;)
! {
! PGresult *res;
! if (server_version >= 90000)
! printfPQExpBuffer(buf, "SELECT setconfig[%d] FROM pg_db_role_setting WHERE "
! "setrole = 0 AND setdatabase = (SELECT oid FROM pg_database WHERE datname = ", count);
! else
! printfPQExpBuffer(buf, "SELECT datconfig[%d] FROM pg_database WHERE datname = ", count);
! appendStringLiteralConn(buf, dbname, conn);
! if (server_version >= 90000)
! appendPQExpBuffer(buf, ")");
!
! appendPQExpBuffer(buf, ";");
!
! res = executeQuery(conn, buf->data);
! if (PQntuples(res) == 1 &&
! !PQgetisnull(res, 0, 0))
! {
! if (!shown_header)
! {
! shown_header = true;
! fprintf(OPF, "--\n-- Database Config\n--\n\n");
! }
!
! makeAlterConfigCommand(conn, PQgetvalue(res, 0, 0),
! "DATABASE", dbname, NULL, NULL);
! PQclear(res);
! count++;
! }
! else
! {
! PQclear(res);
! break;
! }
}
+
+ destroyPQExpBuffer(buf);
}
! PQclear(dbres);
!
! if (shown_header)
! fprintf(OPF, "\n\n");
}
*************** dumpDatabaseConfig(PGconn *conn, const c
*** 1421,1464 ****
* Dump user-specific configuration
*/
static void
! dumpUserConfig(PGconn *conn, const char *username)
{
! PQExpBuffer buf = createPQExpBuffer();
! int count = 1;
! for (;;)
{
! PGresult *res;
! if (server_version >= 90000)
! printfPQExpBuffer(buf, "SELECT setconfig[%d] FROM pg_db_role_setting WHERE "
! "setdatabase = 0 AND setrole = "
! "(SELECT oid FROM pg_authid WHERE rolname = ", count);
! else if (server_version >= 80100)
! printfPQExpBuffer(buf, "SELECT rolconfig[%d] FROM pg_authid WHERE rolname = ", count);
! else
! printfPQExpBuffer(buf, "SELECT useconfig[%d] FROM pg_shadow WHERE usename = ", count);
! appendStringLiteralConn(buf, username, conn);
! if (server_version >= 90000)
! appendPQExpBuffer(buf, ")");
! res = executeQuery(conn, buf->data);
! if (PQntuples(res) == 1 &&
! !PQgetisnull(res, 0, 0))
! {
! makeAlterConfigCommand(conn, PQgetvalue(res, 0, 0),
! "ROLE", username, NULL, NULL);
! PQclear(res);
! count++;
! }
! else
{
! PQclear(res);
! break;
}
}
! destroyPQExpBuffer(buf);
}
--- 1453,1535 ----
* Dump user-specific configuration
*/
static void
! dumpUserConfig(PGconn *conn)
{
! PGresult *userres;
! int i_rolname;
! int i;
! bool shown_header = false;
! if (server_version >= 80100)
! userres = executeQuery(conn,
! "SELECT rolname "
! "FROM pg_authid "
! "ORDER BY 1");
! else
! userres = executeQuery(conn,
! "SELECT usename as rolname "
! "FROM pg_shadow "
! "UNION "
! "SELECT groname as rolname "
! "FROM pg_group "
! "ORDER BY 1");
!
! i_rolname = PQfnumber(userres, "rolname");
!
! for (i = 0; i < PQntuples(userres); i++)
{
! const char *username;
! username = PQgetvalue(userres, i, i_rolname);
! PQExpBuffer buf = createPQExpBuffer();
! int count = 1;
!
! for (;;)
{
! PGresult *res;
!
! if (server_version >= 90000)
! printfPQExpBuffer(buf, "SELECT setconfig[%d] FROM pg_db_role_setting WHERE "
! "setdatabase = 0 AND setrole = "
! "(SELECT oid FROM pg_authid WHERE rolname = ", count);
! else if (server_version >= 80100)
! printfPQExpBuffer(buf, "SELECT rolconfig[%d] FROM pg_authid WHERE rolname = ", count);
! else
! printfPQExpBuffer(buf, "SELECT useconfig[%d] FROM pg_shadow WHERE usename = ", count);
! appendStringLiteralConn(buf, username, conn);
! if (server_version >= 90000)
! appendPQExpBuffer(buf, ")");
!
! res = executeQuery(conn, buf->data);
! if (PQntuples(res) == 1 &&
! !PQgetisnull(res, 0, 0))
! {
! if (!shown_header)
! {
! shown_header = true;
! fprintf(OPF, "--\n-- Role Config\n--\n\n");
! }
!
! makeAlterConfigCommand(conn, PQgetvalue(res, 0, 0),
! "ROLE", username, NULL, NULL);
! PQclear(res);
! count++;
! }
! else
! {
! PQclear(res);
! break;
! }
}
+
+ destroyPQExpBuffer(buf);
}
! PQclear(userres);
!
! if (shown_header)
! fprintf(OPF, "\n\n");
}
Phil Sorber <phil@omniti.com> writes:
I have included two patches in this email. The first
(dump_user_config_last_with_set_role.patch) is an extension of my
first patch. In addition to moving the ALTER ROLE statements after the
CREATE ROLE statements it also inserts a SET ROLE after every connect.
It takes the role parameter from the --role command line option. This
fixes the problem of not being able to restore to a database because
of lack of permissions. This is similar to the idea proposed here:
http://archives.postgresql.org/pgsql-hackers/2010-12/msg01046.php
I don't understand why you think that that will fix anything?
The problem that Florian originally pointed out is that settings
established by ALTER DATABASE/ROLE could interfere with the restoration
script's actions. That seems to be just as much of a risk for the
--role role as the one originally used to connect. I don't see a way
around that other than not applying those settings until we are done
reconnecting to the target database.
Also, given that the --role switch is only defined to select the role
to be used at *dump* time, I'm unconvinced that forcing it to be used
at *restore* time is a good idea. You'd really need to invent a
separate switch if you were to go down this path.
regards, tom lane
On Tue, Aug 2, 2011 at 5:05 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Phil Sorber <phil@omniti.com> writes:
I have included two patches in this email. The first
(dump_user_config_last_with_set_role.patch) is an extension of my
first patch. In addition to moving the ALTER ROLE statements after the
CREATE ROLE statements it also inserts a SET ROLE after every connect.
It takes the role parameter from the --role command line option. This
fixes the problem of not being able to restore to a database because
of lack of permissions. This is similar to the idea proposed here:
http://archives.postgresql.org/pgsql-hackers/2010-12/msg01046.phpI don't understand why you think that that will fix anything?
The problem that Florian originally pointed out is that settings
established by ALTER DATABASE/ROLE could interfere with the restoration
script's actions. That seems to be just as much of a risk for the
--role role as the one originally used to connect. I don't see a way
around that other than not applying those settings until we are done
reconnecting to the target database.Also, given that the --role switch is only defined to select the role
to be used at *dump* time, I'm unconvinced that forcing it to be used
at *restore* time is a good idea. You'd really need to invent a
separate switch if you were to go down this path.regards, tom lane
Ok, here is the patch that just moves the ALTER/SET pieces to the end.
Can we get this included in the next commit fest?
Attachments:
dump_all_configs_last.patchtext/x-patch; charset=US-ASCII; name=dump_all_configs_last.patchDownload
diff --git a/src/bin/pg_dump/pg_dumpall.c b/src/bin/pg_dump/pg_dumpall.c
new file mode 100644
index b5f64e8..d3929f0
*** a/src/bin/pg_dump/pg_dumpall.c
--- b/src/bin/pg_dump/pg_dumpall.c
*************** static void dropTablespaces(PGconn *conn
*** 41,48 ****
static void dumpTablespaces(PGconn *conn);
static void dropDBs(PGconn *conn);
static void dumpCreateDB(PGconn *conn);
! static void dumpDatabaseConfig(PGconn *conn, const char *dbname);
! static void dumpUserConfig(PGconn *conn, const char *username);
static void dumpDbRoleConfig(PGconn *conn);
static void makeAlterConfigCommand(PGconn *conn, const char *arrayitem,
const char *type, const char *name, const char *type2,
--- 41,48 ----
static void dumpTablespaces(PGconn *conn);
static void dropDBs(PGconn *conn);
static void dumpCreateDB(PGconn *conn);
! static void dumpDatabaseConfig(PGconn *conn);
! static void dumpUserConfig(PGconn *conn);
static void dumpDbRoleConfig(PGconn *conn);
static void makeAlterConfigCommand(PGconn *conn, const char *arrayitem,
const char *type, const char *name, const char *type2,
*************** main(int argc, char *argv[])
*** 500,517 ****
/* Dump CREATE DATABASE commands */
if (!globals_only && !roles_only && !tablespaces_only)
dumpCreateDB(conn);
! /* Dump role/database settings */
! if (!tablespaces_only && !roles_only)
{
if (server_version >= 90000)
dumpDbRoleConfig(conn);
}
}
- if (!globals_only && !roles_only && !tablespaces_only)
- dumpDatabases(conn);
-
PQfinish(conn);
if (verbose)
--- 500,524 ----
/* Dump CREATE DATABASE commands */
if (!globals_only && !roles_only && !tablespaces_only)
dumpCreateDB(conn);
+ }
! if (!globals_only && !roles_only && !tablespaces_only)
! dumpDatabases(conn);
!
! if (!data_only && !tablespaces_only && server_version >= 70300)
! {
! dumpUserConfig(conn);
!
! if (!roles_only)
{
+ if (!globals_only)
+ dumpDatabaseConfig(conn);
+
if (server_version >= 90000)
dumpDbRoleConfig(conn);
}
}
PQfinish(conn);
if (verbose)
*************** dumpRoles(PGconn *conn)
*** 804,812 ****
buf, "ROLE", rolename);
fprintf(OPF, "%s", buf->data);
-
- if (server_version >= 70300)
- dumpUserConfig(conn, rolename);
}
PQclear(res);
--- 811,816 ----
*************** dumpCreateDB(PGconn *conn)
*** 1358,1366 ****
fprintf(OPF, "%s", buf->data);
- if (server_version >= 70300)
- dumpDatabaseConfig(conn, dbname);
-
free(fdbname);
}
--- 1362,1367 ----
*************** dumpCreateDB(PGconn *conn)
*** 1375,1418 ****
* Dump database-specific configuration
*/
static void
! dumpDatabaseConfig(PGconn *conn, const char *dbname)
{
! PQExpBuffer buf = createPQExpBuffer();
! int count = 1;
! for (;;)
{
! PGresult *res;
! if (server_version >= 90000)
! printfPQExpBuffer(buf, "SELECT setconfig[%d] FROM pg_db_role_setting WHERE "
! "setrole = 0 AND setdatabase = (SELECT oid FROM pg_database WHERE datname = ", count);
! else
! printfPQExpBuffer(buf, "SELECT datconfig[%d] FROM pg_database WHERE datname = ", count);
! appendStringLiteralConn(buf, dbname, conn);
! if (server_version >= 90000)
! appendPQExpBuffer(buf, ")");
! appendPQExpBuffer(buf, ";");
! res = executeQuery(conn, buf->data);
! if (PQntuples(res) == 1 &&
! !PQgetisnull(res, 0, 0))
! {
! makeAlterConfigCommand(conn, PQgetvalue(res, 0, 0),
! "DATABASE", dbname, NULL, NULL);
! PQclear(res);
! count++;
! }
! else
! {
! PQclear(res);
! break;
}
}
! destroyPQExpBuffer(buf);
}
--- 1376,1450 ----
* Dump database-specific configuration
*/
static void
! dumpDatabaseConfig(PGconn *conn)
{
! PGresult *dbres;
! int i;
! bool shown_header = false;
! if (server_version >= 70100)
! dbres = executeQuery(conn,
! "SELECT datname "
! "FROM pg_database d "
! "WHERE datallowconn ORDER BY 1");
! else
! dbres = executeQuery(conn,
! "SELECT datname "
! "FROM pg_database d "
! "ORDER BY 1");
!
! for (i = 0; i < PQntuples(dbres); i++)
{
! char *dbname = PQgetvalue(dbres, i, 0);
! PQExpBuffer buf = createPQExpBuffer();
! int count = 1;
! for (;;)
! {
! PGresult *res;
! if (server_version >= 90000)
! printfPQExpBuffer(buf, "SELECT setconfig[%d] FROM pg_db_role_setting WHERE "
! "setrole = 0 AND setdatabase = (SELECT oid FROM pg_database WHERE datname = ", count);
! else
! printfPQExpBuffer(buf, "SELECT datconfig[%d] FROM pg_database WHERE datname = ", count);
! appendStringLiteralConn(buf, dbname, conn);
! if (server_version >= 90000)
! appendPQExpBuffer(buf, ")");
!
! appendPQExpBuffer(buf, ";");
!
! res = executeQuery(conn, buf->data);
! if (PQntuples(res) == 1 &&
! !PQgetisnull(res, 0, 0))
! {
! if (!shown_header)
! {
! shown_header = true;
! fprintf(OPF, "--\n-- Database Config\n--\n\n");
! }
!
! makeAlterConfigCommand(conn, PQgetvalue(res, 0, 0),
! "DATABASE", dbname, NULL, NULL);
! PQclear(res);
! count++;
! }
! else
! {
! PQclear(res);
! break;
! }
}
+
+ destroyPQExpBuffer(buf);
}
! PQclear(dbres);
!
! if (shown_header)
! fprintf(OPF, "\n\n");
}
*************** dumpDatabaseConfig(PGconn *conn, const c
*** 1421,1464 ****
* Dump user-specific configuration
*/
static void
! dumpUserConfig(PGconn *conn, const char *username)
{
! PQExpBuffer buf = createPQExpBuffer();
! int count = 1;
! for (;;)
{
! PGresult *res;
! if (server_version >= 90000)
! printfPQExpBuffer(buf, "SELECT setconfig[%d] FROM pg_db_role_setting WHERE "
! "setdatabase = 0 AND setrole = "
! "(SELECT oid FROM pg_authid WHERE rolname = ", count);
! else if (server_version >= 80100)
! printfPQExpBuffer(buf, "SELECT rolconfig[%d] FROM pg_authid WHERE rolname = ", count);
! else
! printfPQExpBuffer(buf, "SELECT useconfig[%d] FROM pg_shadow WHERE usename = ", count);
! appendStringLiteralConn(buf, username, conn);
! if (server_version >= 90000)
! appendPQExpBuffer(buf, ")");
! res = executeQuery(conn, buf->data);
! if (PQntuples(res) == 1 &&
! !PQgetisnull(res, 0, 0))
! {
! makeAlterConfigCommand(conn, PQgetvalue(res, 0, 0),
! "ROLE", username, NULL, NULL);
! PQclear(res);
! count++;
! }
! else
{
! PQclear(res);
! break;
}
}
! destroyPQExpBuffer(buf);
}
--- 1453,1535 ----
* Dump user-specific configuration
*/
static void
! dumpUserConfig(PGconn *conn)
{
! PGresult *userres;
! int i_rolname;
! int i;
! bool shown_header = false;
! if (server_version >= 80100)
! userres = executeQuery(conn,
! "SELECT rolname "
! "FROM pg_authid "
! "ORDER BY 1");
! else
! userres = executeQuery(conn,
! "SELECT usename as rolname "
! "FROM pg_shadow "
! "UNION "
! "SELECT groname as rolname "
! "FROM pg_group "
! "ORDER BY 1");
!
! i_rolname = PQfnumber(userres, "rolname");
!
! for (i = 0; i < PQntuples(userres); i++)
{
! const char *username;
! username = PQgetvalue(userres, i, i_rolname);
! PQExpBuffer buf = createPQExpBuffer();
! int count = 1;
!
! for (;;)
{
! PGresult *res;
!
! if (server_version >= 90000)
! printfPQExpBuffer(buf, "SELECT setconfig[%d] FROM pg_db_role_setting WHERE "
! "setdatabase = 0 AND setrole = "
! "(SELECT oid FROM pg_authid WHERE rolname = ", count);
! else if (server_version >= 80100)
! printfPQExpBuffer(buf, "SELECT rolconfig[%d] FROM pg_authid WHERE rolname = ", count);
! else
! printfPQExpBuffer(buf, "SELECT useconfig[%d] FROM pg_shadow WHERE usename = ", count);
! appendStringLiteralConn(buf, username, conn);
! if (server_version >= 90000)
! appendPQExpBuffer(buf, ")");
!
! res = executeQuery(conn, buf->data);
! if (PQntuples(res) == 1 &&
! !PQgetisnull(res, 0, 0))
! {
! if (!shown_header)
! {
! shown_header = true;
! fprintf(OPF, "--\n-- Role Config\n--\n\n");
! }
!
! makeAlterConfigCommand(conn, PQgetvalue(res, 0, 0),
! "ROLE", username, NULL, NULL);
! PQclear(res);
! count++;
! }
! else
! {
! PQclear(res);
! break;
! }
}
+
+ destroyPQExpBuffer(buf);
}
! PQclear(userres);
!
! if (shown_header)
! fprintf(OPF, "\n\n");
}
On Thu, Aug 4, 2011 at 1:53 PM, Phil Sorber <phil@omniti.com> wrote:
Ok, here is the patch that just moves the ALTER/SET pieces to the end.
Can we get this included in the next commit fest?
Yep, just make yourself an account and add it.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
On Thu, Aug 4, 2011 at 2:04 PM, Robert Haas <robertmhaas@gmail.com> wrote:
On Thu, Aug 4, 2011 at 1:53 PM, Phil Sorber <phil@omniti.com> wrote:
Ok, here is the patch that just moves the ALTER/SET pieces to the end.
Can we get this included in the next commit fest?Yep, just make yourself an account and add it.
Unfortunately, it doesn't look like anyone ever replied to this
thread, but Tom posted some thoughts on another thread that seem to me
to be a serious problem for this patch:
http://archives.postgresql.org/message-id/13764.1315094292@sss.pgh.pa.us
I don't see any easy way around that problem, so I'm going to mark
this patch Returned with Feedback for now. It strikes me as craziness
to try to guess which settings we should restore at the beginning and
which at the end, so I think we need a better idea. I don't really
understand why it's not OK to just have pg_dump issue RESET ROLE at
appropriate points in the process; that seems like it would be
sufficient and not particularly ugly.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
Robert Haas <robertmhaas@gmail.com> writes:
I don't really
understand why it's not OK to just have pg_dump issue RESET ROLE at
appropriate points in the process; that seems like it would be
sufficient and not particularly ugly.
Well, it was alleged that that would fix this problem:
http://archives.postgresql.org/pgsql-hackers/2010-12/msg00916.php
but if it does fix it, I think that's a bug in itself:
http://archives.postgresql.org/pgsql-hackers/2010-12/msg01031.php
But more to the point, I think the specific case of "ALTER DATABASE SET
ROLE" is just one element of a class of problems, namely that settings
attached to either databases or roles could create issues for restoring
a dump. Issuing RESET ROLE would fix only that one single case.
regards, tom lane
On Mon, Oct 10, 2011 at 12:14 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Robert Haas <robertmhaas@gmail.com> writes:
I don't really
understand why it's not OK to just have pg_dump issue RESET ROLE at
appropriate points in the process; that seems like it would be
sufficient and not particularly ugly.Well, it was alleged that that would fix this problem:
http://archives.postgresql.org/pgsql-hackers/2010-12/msg00916.php
but if it does fix it, I think that's a bug in itself:
http://archives.postgresql.org/pgsql-hackers/2010-12/msg01031.php
Hmm.
But more to the point, I think the specific case of "ALTER DATABASE SET
ROLE" is just one element of a class of problems, namely that settings
attached to either databases or roles could create issues for restoring
a dump. Issuing RESET ROLE would fix only that one single case.
It's not very clear to me that we're going to find a fix that reaches
across every setting, though. I mean, for something like
maintenance_work_mem, there's no correctness issue regardless of when
the new setting takes effect, but there might very well be a
performance issue, and it's not really all that clear when the "right"
time to put the old setting back is. And that ambiguity about what's
actually correct is, perhaps, the root of the problem.
There are related cases where we have a clear-cut policy. For
example, we are clear that you must use the newer pg_dump against the
older server for best results. That's not always convenient, but at
least it's a line in the sand.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
On Mon, Oct 10, 2011 at 11:54 AM, Robert Haas <robertmhaas@gmail.com> wrote:
On Thu, Aug 4, 2011 at 2:04 PM, Robert Haas <robertmhaas@gmail.com> wrote:
On Thu, Aug 4, 2011 at 1:53 PM, Phil Sorber <phil@omniti.com> wrote:
Ok, here is the patch that just moves the ALTER/SET pieces to the end.
Can we get this included in the next commit fest?Yep, just make yourself an account and add it.
Unfortunately, it doesn't look like anyone ever replied to this
thread, but Tom posted some thoughts on another thread that seem to me
to be a serious problem for this patch:http://archives.postgresql.org/message-id/13764.1315094292@sss.pgh.pa.us
I don't see any easy way around that problem, so I'm going to mark
this patch Returned with Feedback for now. It strikes me as craziness
to try to guess which settings we should restore at the beginning and
which at the end, so I think we need a better idea. I don't really
understand why it's not OK to just have pg_dump issue RESET ROLE at
appropriate points in the process; that seems like it would be
sufficient and not particularly ugly.--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
I am going to remove that patch from the commit fest because we all
agree that it does not solve the problem satisfactorily. I would like
to re-iterate a few points, and submit a new patch.
First off, there are two distinct problems here that we have been
lumping into one. There is the issue of the 'ALTER DATABASE SET ROLE'
and then there is the 'ALTER ROLE SET ROLE' case. The former is the
one that has been causing us so many problems, and the latter is the
one that I really care about.
Also, there are more people that are hitting this issue as well:
http://archives.postgresql.org/pgsql-hackers/2011-02/msg02362.php
My proposal would be to table the discussion about the 'ALTER DATABASE
SET ROLE' case because there seems to be a bit of a philosophical
debate behind this that needs to be sorted out first.
If we focus only on the 'ALTER ROLE' case I think there is a trivial
solution. We already separate the CREATE from the ALTER in a single
loop. We also already separate out the user config in a separate
function called from this same loop. The problem is that the user
config can be dependent upon a later CREATE. So all I suggest is to
move the user config dumping into a new loop afterward so that the
user config ALTER's come after all the other CREATE's and ALTER's. It
amounts to a 7 line change and solves our problem rather elegantly.
Attachments:
dump_user_config.patchtext/x-patch; charset=US-ASCII; name=dump_user_config.patchDownload
diff --git a/src/bin/pg_dump/pg_dumpall.c b/src/bin/pg_dump/pg_dumpall.c
new file mode 100644
index b5f64e8..ee597d5
*** a/src/bin/pg_dump/pg_dumpall.c
--- b/src/bin/pg_dump/pg_dumpall.c
*************** dumpRoles(PGconn *conn)
*** 804,814 ****
buf, "ROLE", rolename);
fprintf(OPF, "%s", buf->data);
-
- if (server_version >= 70300)
- dumpUserConfig(conn, rolename);
}
PQclear(res);
fprintf(OPF, "\n\n");
--- 804,815 ----
buf, "ROLE", rolename);
fprintf(OPF, "%s", buf->data);
}
+ if (server_version >= 70300)
+ for (i = 0; i < PQntuples(res); i++)
+ dumpUserConfig(conn, PQgetvalue(res, i, i_rolname));
+
PQclear(res);
fprintf(OPF, "\n\n");
On Wed, Oct 12, 2011 at 2:27 PM, Phil Sorber <phil@omniti.com> wrote:
I am going to remove that patch from the commit fest because we all
agree that it does not solve the problem satisfactorily. I would like
to re-iterate a few points, and submit a new patch.First off, there are two distinct problems here that we have been
lumping into one. There is the issue of the 'ALTER DATABASE SET ROLE'
and then there is the 'ALTER ROLE SET ROLE' case. The former is the
one that has been causing us so many problems, and the latter is the
one that I really care about.Also, there are more people that are hitting this issue as well:
http://archives.postgresql.org/pgsql-hackers/2011-02/msg02362.php
My proposal would be to table the discussion about the 'ALTER DATABASE
SET ROLE' case because there seems to be a bit of a philosophical
debate behind this that needs to be sorted out first.If we focus only on the 'ALTER ROLE' case I think there is a trivial
solution. We already separate the CREATE from the ALTER in a single
loop. We also already separate out the user config in a separate
function called from this same loop. The problem is that the user
config can be dependent upon a later CREATE. So all I suggest is to
move the user config dumping into a new loop afterward so that the
user config ALTER's come after all the other CREATE's and ALTER's. It
amounts to a 7 line change and solves our problem rather elegantly.
I'm not sure I completely follow this explanation of the problem, but
it does seem better to me to set all the role attributes after dumping
all the create role statements, and I don't see how that can break
anything that works now.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
On Wed, Oct 12, 2011 at 3:48 PM, Robert Haas <robertmhaas@gmail.com> wrote:
On Wed, Oct 12, 2011 at 2:27 PM, Phil Sorber <phil@omniti.com> wrote:
I am going to remove that patch from the commit fest because we all
agree that it does not solve the problem satisfactorily. I would like
to re-iterate a few points, and submit a new patch.First off, there are two distinct problems here that we have been
lumping into one. There is the issue of the 'ALTER DATABASE SET ROLE'
and then there is the 'ALTER ROLE SET ROLE' case. The former is the
one that has been causing us so many problems, and the latter is the
one that I really care about.Also, there are more people that are hitting this issue as well:
http://archives.postgresql.org/pgsql-hackers/2011-02/msg02362.php
My proposal would be to table the discussion about the 'ALTER DATABASE
SET ROLE' case because there seems to be a bit of a philosophical
debate behind this that needs to be sorted out first.If we focus only on the 'ALTER ROLE' case I think there is a trivial
solution. We already separate the CREATE from the ALTER in a single
loop. We also already separate out the user config in a separate
function called from this same loop. The problem is that the user
config can be dependent upon a later CREATE. So all I suggest is to
move the user config dumping into a new loop afterward so that the
user config ALTER's come after all the other CREATE's and ALTER's. It
amounts to a 7 line change and solves our problem rather elegantly.I'm not sure I completely follow this explanation of the problem, but
it does seem better to me to set all the role attributes after dumping
all the create role statements, and I don't see how that can break
anything that works now.--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
Just to be clear, this doesn't move all the ALTER's. It will still do
CREATE/ALTER pair's for attributes that could go in the CREATE.
Example:
CREATE ROLE bob;
ALTER ROLE bob WITH LOGIN PASSWORD 'blah';
You could turn those in to one statement, but for various reasons you
do not want to. None of these have any dependencies other than the
actual creation of the role.
Then there is a separate section of code that is called as a separate
function 'dumpUserConfig()' that does other role attributes like
'ALTER ROLE bob SET role TO charlie'. These are the ALTER's that can
have dependencies on other roles.
I agree this is a little confusing, and if you prefer to see all the
ALTER's in one section together directly after the CREATE statements I
can make the patch do that, but it isn't necessary to solve the
problem.
Phil Sorber <phil@omniti.com> writes:
Then there is a separate section of code that is called as a separate
function 'dumpUserConfig()' that does other role attributes like
'ALTER ROLE bob SET role TO charlie'. These are the ALTER's that can
have dependencies on other roles.
Right. Phrased that way, this is an obvious improvement, and I concur
that it doesn't look like it could break anything that works now.
The more general problem remains to be solved, but we might as well
apply this.
regards, tom lane
On Wed, Oct 12, 2011 at 02:27:51PM -0400, Phil Sorber wrote:
My proposal would be to table the discussion about the 'ALTER DATABASE
SET ROLE' case because there seems to be a bit of a philosophical
debate behind this that needs to be sorted out first.
Note: "to table the discussion" means opposite things in American and
British english. I think the rest of the sentence makes it clear what
you mean though :).
Have a nice day,
--
Martijn van Oosterhout <kleptog@svana.org> http://svana.org/kleptog/
He who writes carelessly confesses thereby at the very outset that he does
not attach much importance to his own thoughts.
-- Arthur Schopenhauer
On Wed, Oct 12, 2011 at 7:07 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Phil Sorber <phil@omniti.com> writes:
Then there is a separate section of code that is called as a separate
function 'dumpUserConfig()' that does other role attributes like
'ALTER ROLE bob SET role TO charlie'. These are the ALTER's that can
have dependencies on other roles.Right. Phrased that way, this is an obvious improvement, and I concur
that it doesn't look like it could break anything that works now.
The more general problem remains to be solved, but we might as well
apply this.
OK, done.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company