BUG #13741: vacuumdb does not accept valid password
The following bug has been logged on the website:
Bug reference: 13741
Logged by: Eric Brown
Email address: brown@fastmail.com
PostgreSQL version: 9.5beta1
Operating system: CentOS 6.6
Description:
I am trying to vacuum a database from a remote computer. It appears that
vacuumdb is not accepting my password, though psql connects just fine.
______
browne1@reslin3 ~/Desktop> vacuumdb -z -j 24 -h reslin1.enhnet.org -p 5433
127
vacuumdb: vacuuming database "browne1"
Password:
Password:
Password:
vacuumdb: could not connect to database browne1: FATAL: password
authentication failed for user "browne1"
browne1@reslin3 ~/Desktop> psql -h reslin1.enhnet.org -p 5433
1
Password:
psql (9.5beta1)
Type "help" for help.
browne1=#
______
--
Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-bugs
On Wed, Oct 28, 2015 at 6:39 AM, <brown@fastmail.com> wrote:
The following bug has been logged on the website:
Bug reference: 13741
Logged by: Eric Brown
Email address: brown@fastmail.com
PostgreSQL version: 9.5beta1
Operating system: CentOS 6.6
Description:I am trying to vacuum a database from a remote computer. It appears that
vacuumdb is not accepting my password, though psql connects just fine.______
browne1@reslin3 ~/Desktop> vacuumdb -z -j 24 -h reslin1.enhnet.org -p 5433
127
vacuumdb: vacuuming database "browne1"
Password:
Password:
Password:
vacuumdb: could not connect to database browne1: FATAL: password
authentication failed for user "browne1"
browne1@reslin3 ~/Desktop> psql -h reslin1.enhnet.org -p 5433
1
Password:
psql (9.5beta1)
Type "help" for help.browne1=#
______
No. vacuumdb is accepting your password correctly. But you gave the number
of client connections as 24, to connect all these 24 clients to the
database, it is requesting
the password to enter again.
you can try with vacuumb -z -j2 to verify the same.
I feel we need to accept the password once and reuse it for all client
connections.
Provided working password can be returned for that database.
Regards,
Hari Babu
Fujitsu Australia
--
Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-bugs
On Wed, Oct 28, 2015 at 2:53 PM, Haribabu Kommi
<kommi.haribabu@gmail.com> wrote:
On Wed, Oct 28, 2015 at 6:39 AM, <brown@fastmail.com> wrote:
The following bug has been logged on the website:
Bug reference: 13741
Logged by: Eric Brown
Email address: brown@fastmail.com
PostgreSQL version: 9.5beta1
Operating system: CentOS 6.6
Description:I am trying to vacuum a database from a remote computer. It appears that
vacuumdb is not accepting my password, though psql connects just fine.______
browne1@reslin3 ~/Desktop> vacuumdb -z -j 24 -h reslin1.enhnet.org -p 5433
127
vacuumdb: vacuuming database "browne1"
Password:
Password:
Password:
vacuumdb: could not connect to database browne1: FATAL: password
authentication failed for user "browne1"
browne1@reslin3 ~/Desktop> psql -h reslin1.enhnet.org -p 5433
1
Password:
psql (9.5beta1)
Type "help" for help.browne1=#
______No. vacuumdb is accepting your password correctly. But you gave the number
of client connections as 24, to connect all these 24 clients to the
database, it is requesting
the password to enter again.you can try with vacuumb -z -j2 to verify the same.
I feel we need to accept the password once and reuse it for all client
connections.
Provided working password can be returned for that database.
Here I attached a patch that saves the password provided by the user
from the connectDatabase function and reuse it for connecting all clients
to the same database.
Regards,
Hari Babu
Fujitsu Australia
Attachments:
reuse_password_for_vacuum_db_v1.patchapplication/octet-stream; name=reuse_password_for_vacuum_db_v1.patchDownload
*** a/src/bin/scripts/clusterdb.c
--- b/src/bin/scripts/clusterdb.c
***************
*** 203,209 **** cluster_one_database(const char *dbname, bool verbose, const char *table,
appendPQExpBuffer(&sql, " %s", table);
appendPQExpBufferChar(&sql, ';');
! conn = connectDatabase(dbname, host, port, username, prompt_password,
progname, false);
if (!executeMaintenanceCommand(conn, sql.data, echo))
{
--- 203,209 ----
appendPQExpBuffer(&sql, " %s", table);
appendPQExpBufferChar(&sql, ';');
! conn = connectDatabase(dbname, host, port, username, NULL, prompt_password,
progname, false);
if (!executeMaintenanceCommand(conn, sql.data, echo))
{
*** a/src/bin/scripts/common.c
--- b/src/bin/scripts/common.c
***************
*** 54,70 **** handle_help_version_opts(int argc, char *argv[],
/*
* Make a database connection with the given parameters. An
* interactive password prompt is automatically issued if required.
*/
PGconn *
connectDatabase(const char *dbname, const char *pghost, const char *pgport,
! const char *pguser, enum trivalue prompt_password,
const char *progname, bool fail_ok)
{
PGconn *conn;
! char *password = NULL;
bool new_pass;
! if (prompt_password == TRI_YES)
password = simple_prompt("Password: ", 100, false);
/*
--- 54,71 ----
/*
* Make a database connection with the given parameters. An
* interactive password prompt is automatically issued if required.
+ * Caller has to free the password if the output returns a password.
*/
PGconn *
connectDatabase(const char *dbname, const char *pghost, const char *pgport,
! const char *pguser, char **saved_password, enum trivalue prompt_password,
const char *progname, bool fail_ok)
{
PGconn *conn;
! char *password = saved_password ? *saved_password : NULL;
bool new_pass;
! if (prompt_password == TRI_YES && password == NULL)
password = simple_prompt("Password: ", 100, false);
/*
***************
*** 116,122 **** connectDatabase(const char *dbname, const char *pghost, const char *pgport,
}
} while (new_pass);
! if (password)
free(password);
/* check to see that the backend connection was successfully made */
--- 117,129 ----
}
} while (new_pass);
! /*
! * save the password for future reference,
! * in case if caller requests it.
! */
! if (saved_password)
! *saved_password = password;
! else if (password)
free(password);
/* check to see that the backend connection was successfully made */
***************
*** 149,162 **** connectMaintenanceDatabase(const char *maintenance_db, const char *pghost,
/* If a maintenance database name was specified, just connect to it. */
if (maintenance_db)
return connectDatabase(maintenance_db, pghost, pgport, pguser,
! prompt_password, progname, false);
/* Otherwise, try postgres first and then template1. */
! conn = connectDatabase("postgres", pghost, pgport, pguser, prompt_password,
progname, true);
if (!conn)
conn = connectDatabase("template1", pghost, pgport, pguser,
! prompt_password, progname, false);
return conn;
}
--- 156,169 ----
/* If a maintenance database name was specified, just connect to it. */
if (maintenance_db)
return connectDatabase(maintenance_db, pghost, pgport, pguser,
! NULL, prompt_password, progname, false);
/* Otherwise, try postgres first and then template1. */
! conn = connectDatabase("postgres", pghost, pgport, pguser, NULL, prompt_password,
progname, true);
if (!conn)
conn = connectDatabase("template1", pghost, pgport, pguser,
! NULL, prompt_password, progname, false);
return conn;
}
*** a/src/bin/scripts/common.h
--- b/src/bin/scripts/common.h
***************
*** 30,36 **** extern void handle_help_version_opts(int argc, char *argv[],
help_handler hlp);
extern PGconn *connectDatabase(const char *dbname, const char *pghost,
! const char *pgport, const char *pguser,
enum trivalue prompt_password, const char *progname,
bool fail_ok);
--- 30,36 ----
help_handler hlp);
extern PGconn *connectDatabase(const char *dbname, const char *pghost,
! const char *pgport, const char *pguser, char *saved_password,
enum trivalue prompt_password, const char *progname,
bool fail_ok);
*** a/src/bin/scripts/createlang.c
--- b/src/bin/scripts/createlang.c
***************
*** 140,146 **** main(int argc, char *argv[])
printQueryOpt popt;
static const bool translate_columns[] = {false, true};
! conn = connectDatabase(dbname, host, port, username, prompt_password,
progname, false);
printfPQExpBuffer(&sql, "SELECT lanname as \"%s\", "
--- 140,146 ----
printQueryOpt popt;
static const bool translate_columns[] = {false, true};
! conn = connectDatabase(dbname, host, port, username, NULL, prompt_password,
progname, false);
printfPQExpBuffer(&sql, "SELECT lanname as \"%s\", "
***************
*** 180,186 **** main(int argc, char *argv[])
if (*p >= 'A' && *p <= 'Z')
*p += ('a' - 'A');
! conn = connectDatabase(dbname, host, port, username, prompt_password,
progname, false);
/*
--- 180,186 ----
if (*p >= 'A' && *p <= 'Z')
*p += ('a' - 'A');
! conn = connectDatabase(dbname, host, port, username, NULL, prompt_password,
progname, false);
/*
*** a/src/bin/scripts/createuser.c
--- b/src/bin/scripts/createuser.c
***************
*** 250,256 **** main(int argc, char *argv[])
if (login == 0)
login = TRI_YES;
! conn = connectDatabase("postgres", host, port, username, prompt_password,
progname, false);
initPQExpBuffer(&sql);
--- 250,256 ----
if (login == 0)
login = TRI_YES;
! conn = connectDatabase("postgres", host, port, username, NULL, prompt_password,
progname, false);
initPQExpBuffer(&sql);
*** a/src/bin/scripts/droplang.c
--- b/src/bin/scripts/droplang.c
***************
*** 139,145 **** main(int argc, char *argv[])
printQueryOpt popt;
static const bool translate_columns[] = {false, true};
! conn = connectDatabase(dbname, host, port, username, prompt_password,
progname, false);
printfPQExpBuffer(&sql, "SELECT lanname as \"%s\", "
--- 139,145 ----
printQueryOpt popt;
static const bool translate_columns[] = {false, true};
! conn = connectDatabase(dbname, host, port, username, NULL, prompt_password,
progname, false);
printfPQExpBuffer(&sql, "SELECT lanname as \"%s\", "
***************
*** 181,187 **** main(int argc, char *argv[])
if (*p >= 'A' && *p <= 'Z')
*p += ('a' - 'A');
! conn = connectDatabase(dbname, host, port, username, prompt_password,
progname, false);
/*
--- 181,187 ----
if (*p >= 'A' && *p <= 'Z')
*p += ('a' - 'A');
! conn = connectDatabase(dbname, host, port, username, NULL, prompt_password,
progname, false);
/*
*** a/src/bin/scripts/dropuser.c
--- b/src/bin/scripts/dropuser.c
***************
*** 128,134 **** main(int argc, char *argv[])
appendPQExpBuffer(&sql, "DROP ROLE %s%s;",
(if_exists ? "IF EXISTS " : ""), fmtId(dropuser));
! conn = connectDatabase("postgres", host, port, username, prompt_password,
progname, false);
if (echo)
--- 128,134 ----
appendPQExpBuffer(&sql, "DROP ROLE %s%s;",
(if_exists ? "IF EXISTS " : ""), fmtId(dropuser));
! conn = connectDatabase("postgres", host, port, username, NULL, prompt_password,
progname, false);
if (echo)
*** a/src/bin/scripts/reindexdb.c
--- b/src/bin/scripts/reindexdb.c
***************
*** 297,303 **** reindex_one_database(const char *name, const char *dbname, const char *type,
appendPQExpBuffer(&sql, " DATABASE %s", fmtId(name));
appendPQExpBufferChar(&sql, ';');
! conn = connectDatabase(dbname, host, port, username, prompt_password,
progname, false);
if (!executeMaintenanceCommand(conn, sql.data, echo))
--- 297,303 ----
appendPQExpBuffer(&sql, " DATABASE %s", fmtId(name));
appendPQExpBufferChar(&sql, ';');
! conn = connectDatabase(dbname, host, port, username, NULL, prompt_password,
progname, false);
if (!executeMaintenanceCommand(conn, sql.data, echo))
***************
*** 372,378 **** reindex_system_catalogs(const char *dbname, const char *host, const char *port,
appendPQExpBuffer(&sql, " SYSTEM %s;", dbname);
! conn = connectDatabase(dbname, host, port, username, prompt_password,
progname, false);
if (!executeMaintenanceCommand(conn, sql.data, echo))
{
--- 372,378 ----
appendPQExpBuffer(&sql, " SYSTEM %s;", dbname);
! conn = connectDatabase(dbname, host, port, username, NULL, prompt_password,
progname, false);
if (!executeMaintenanceCommand(conn, sql.data, echo))
{
*** a/src/bin/scripts/vacuumdb.c
--- b/src/bin/scripts/vacuumdb.c
***************
*** 341,346 **** vacuum_one_database(const char *dbname, vacuumingOptions *vacopts,
--- 341,347 ----
int i;
bool failed = false;
bool parallel = concurrentCons > 1;
+ char *saved_password = NULL;
const char *stage_commands[] = {
"SET default_statistics_target=1; SET vacuum_cost_delay=0;",
"SET default_statistics_target=10; RESET vacuum_cost_delay;",
***************
*** 365,371 **** vacuum_one_database(const char *dbname, vacuumingOptions *vacopts,
fflush(stdout);
}
! conn = connectDatabase(dbname, host, port, username, prompt_password,
progname, false);
initPQExpBuffer(&sql);
--- 366,372 ----
fflush(stdout);
}
! conn = connectDatabase(dbname, host, port, username, &saved_password, prompt_password,
progname, false);
initPQExpBuffer(&sql);
***************
*** 426,432 **** vacuum_one_database(const char *dbname, vacuumingOptions *vacopts,
{
for (i = 1; i < concurrentCons; i++)
{
! conn = connectDatabase(dbname, host, port, username, prompt_password,
progname, false);
init_slot(slots + i, conn);
}
--- 427,433 ----
{
for (i = 1; i < concurrentCons; i++)
{
! conn = connectDatabase(dbname, host, port, username, &saved_password, prompt_password,
progname, false);
init_slot(slots + i, conn);
}
On Wed, Oct 28, 2015 at 4:07 PM, Haribabu Kommi
<kommi.haribabu@gmail.com> wrote:
Here I attached a patch that saves the password provided by the user
from the connectDatabase function and reuse it for connecting all clients
to the same database.
Attached a wrong patch, it is having some compilation problems,
Here is the new version with the fixed problems.
Regards,
Hari Babu
Fujitsu Australia
Attachments:
reuse_password_for_vacuum_db_v2.patchapplication/octet-stream; name=reuse_password_for_vacuum_db_v2.patchDownload
*** a/src/bin/scripts/clusterdb.c
--- b/src/bin/scripts/clusterdb.c
***************
*** 203,209 **** cluster_one_database(const char *dbname, bool verbose, const char *table,
appendPQExpBuffer(&sql, " %s", table);
appendPQExpBufferChar(&sql, ';');
! conn = connectDatabase(dbname, host, port, username, prompt_password,
progname, false);
if (!executeMaintenanceCommand(conn, sql.data, echo))
{
--- 203,209 ----
appendPQExpBuffer(&sql, " %s", table);
appendPQExpBufferChar(&sql, ';');
! conn = connectDatabase(dbname, host, port, username, NULL, prompt_password,
progname, false);
if (!executeMaintenanceCommand(conn, sql.data, echo))
{
*** a/src/bin/scripts/common.c
--- b/src/bin/scripts/common.c
***************
*** 54,70 **** handle_help_version_opts(int argc, char *argv[],
/*
* Make a database connection with the given parameters. An
* interactive password prompt is automatically issued if required.
*/
PGconn *
connectDatabase(const char *dbname, const char *pghost, const char *pgport,
! const char *pguser, enum trivalue prompt_password,
const char *progname, bool fail_ok)
{
PGconn *conn;
! char *password = NULL;
bool new_pass;
! if (prompt_password == TRI_YES)
password = simple_prompt("Password: ", 100, false);
/*
--- 54,71 ----
/*
* Make a database connection with the given parameters. An
* interactive password prompt is automatically issued if required.
+ * Caller has to free the password if the output returns a password.
*/
PGconn *
connectDatabase(const char *dbname, const char *pghost, const char *pgport,
! const char *pguser, char **saved_password, enum trivalue prompt_password,
const char *progname, bool fail_ok)
{
PGconn *conn;
! char *password = saved_password ? *saved_password : NULL;
bool new_pass;
! if (prompt_password == TRI_YES && password == NULL)
password = simple_prompt("Password: ", 100, false);
/*
***************
*** 116,122 **** connectDatabase(const char *dbname, const char *pghost, const char *pgport,
}
} while (new_pass);
! if (password)
free(password);
/* check to see that the backend connection was successfully made */
--- 117,129 ----
}
} while (new_pass);
! /*
! * save the password for future reference,
! * in case if caller requests it.
! */
! if (saved_password)
! *saved_password = password;
! else if (password)
free(password);
/* check to see that the backend connection was successfully made */
***************
*** 149,162 **** connectMaintenanceDatabase(const char *maintenance_db, const char *pghost,
/* If a maintenance database name was specified, just connect to it. */
if (maintenance_db)
return connectDatabase(maintenance_db, pghost, pgport, pguser,
! prompt_password, progname, false);
/* Otherwise, try postgres first and then template1. */
! conn = connectDatabase("postgres", pghost, pgport, pguser, prompt_password,
progname, true);
if (!conn)
conn = connectDatabase("template1", pghost, pgport, pguser,
! prompt_password, progname, false);
return conn;
}
--- 156,169 ----
/* If a maintenance database name was specified, just connect to it. */
if (maintenance_db)
return connectDatabase(maintenance_db, pghost, pgport, pguser,
! NULL, prompt_password, progname, false);
/* Otherwise, try postgres first and then template1. */
! conn = connectDatabase("postgres", pghost, pgport, pguser, NULL, prompt_password,
progname, true);
if (!conn)
conn = connectDatabase("template1", pghost, pgport, pguser,
! NULL, prompt_password, progname, false);
return conn;
}
*** a/src/bin/scripts/common.h
--- b/src/bin/scripts/common.h
***************
*** 30,36 **** extern void handle_help_version_opts(int argc, char *argv[],
help_handler hlp);
extern PGconn *connectDatabase(const char *dbname, const char *pghost,
! const char *pgport, const char *pguser,
enum trivalue prompt_password, const char *progname,
bool fail_ok);
--- 30,36 ----
help_handler hlp);
extern PGconn *connectDatabase(const char *dbname, const char *pghost,
! const char *pgport, const char *pguser, char **saved_password,
enum trivalue prompt_password, const char *progname,
bool fail_ok);
*** a/src/bin/scripts/createlang.c
--- b/src/bin/scripts/createlang.c
***************
*** 140,146 **** main(int argc, char *argv[])
printQueryOpt popt;
static const bool translate_columns[] = {false, true};
! conn = connectDatabase(dbname, host, port, username, prompt_password,
progname, false);
printfPQExpBuffer(&sql, "SELECT lanname as \"%s\", "
--- 140,146 ----
printQueryOpt popt;
static const bool translate_columns[] = {false, true};
! conn = connectDatabase(dbname, host, port, username, NULL, prompt_password,
progname, false);
printfPQExpBuffer(&sql, "SELECT lanname as \"%s\", "
***************
*** 180,186 **** main(int argc, char *argv[])
if (*p >= 'A' && *p <= 'Z')
*p += ('a' - 'A');
! conn = connectDatabase(dbname, host, port, username, prompt_password,
progname, false);
/*
--- 180,186 ----
if (*p >= 'A' && *p <= 'Z')
*p += ('a' - 'A');
! conn = connectDatabase(dbname, host, port, username, NULL, prompt_password,
progname, false);
/*
*** a/src/bin/scripts/createuser.c
--- b/src/bin/scripts/createuser.c
***************
*** 250,256 **** main(int argc, char *argv[])
if (login == 0)
login = TRI_YES;
! conn = connectDatabase("postgres", host, port, username, prompt_password,
progname, false);
initPQExpBuffer(&sql);
--- 250,256 ----
if (login == 0)
login = TRI_YES;
! conn = connectDatabase("postgres", host, port, username, NULL, prompt_password,
progname, false);
initPQExpBuffer(&sql);
*** a/src/bin/scripts/droplang.c
--- b/src/bin/scripts/droplang.c
***************
*** 139,145 **** main(int argc, char *argv[])
printQueryOpt popt;
static const bool translate_columns[] = {false, true};
! conn = connectDatabase(dbname, host, port, username, prompt_password,
progname, false);
printfPQExpBuffer(&sql, "SELECT lanname as \"%s\", "
--- 139,145 ----
printQueryOpt popt;
static const bool translate_columns[] = {false, true};
! conn = connectDatabase(dbname, host, port, username, NULL, prompt_password,
progname, false);
printfPQExpBuffer(&sql, "SELECT lanname as \"%s\", "
***************
*** 181,187 **** main(int argc, char *argv[])
if (*p >= 'A' && *p <= 'Z')
*p += ('a' - 'A');
! conn = connectDatabase(dbname, host, port, username, prompt_password,
progname, false);
/*
--- 181,187 ----
if (*p >= 'A' && *p <= 'Z')
*p += ('a' - 'A');
! conn = connectDatabase(dbname, host, port, username, NULL, prompt_password,
progname, false);
/*
*** a/src/bin/scripts/dropuser.c
--- b/src/bin/scripts/dropuser.c
***************
*** 128,134 **** main(int argc, char *argv[])
appendPQExpBuffer(&sql, "DROP ROLE %s%s;",
(if_exists ? "IF EXISTS " : ""), fmtId(dropuser));
! conn = connectDatabase("postgres", host, port, username, prompt_password,
progname, false);
if (echo)
--- 128,134 ----
appendPQExpBuffer(&sql, "DROP ROLE %s%s;",
(if_exists ? "IF EXISTS " : ""), fmtId(dropuser));
! conn = connectDatabase("postgres", host, port, username, NULL, prompt_password,
progname, false);
if (echo)
*** a/src/bin/scripts/reindexdb.c
--- b/src/bin/scripts/reindexdb.c
***************
*** 297,303 **** reindex_one_database(const char *name, const char *dbname, const char *type,
appendPQExpBuffer(&sql, " DATABASE %s", fmtId(name));
appendPQExpBufferChar(&sql, ';');
! conn = connectDatabase(dbname, host, port, username, prompt_password,
progname, false);
if (!executeMaintenanceCommand(conn, sql.data, echo))
--- 297,303 ----
appendPQExpBuffer(&sql, " DATABASE %s", fmtId(name));
appendPQExpBufferChar(&sql, ';');
! conn = connectDatabase(dbname, host, port, username, NULL, prompt_password,
progname, false);
if (!executeMaintenanceCommand(conn, sql.data, echo))
***************
*** 372,378 **** reindex_system_catalogs(const char *dbname, const char *host, const char *port,
appendPQExpBuffer(&sql, " SYSTEM %s;", dbname);
! conn = connectDatabase(dbname, host, port, username, prompt_password,
progname, false);
if (!executeMaintenanceCommand(conn, sql.data, echo))
{
--- 372,378 ----
appendPQExpBuffer(&sql, " SYSTEM %s;", dbname);
! conn = connectDatabase(dbname, host, port, username, NULL, prompt_password,
progname, false);
if (!executeMaintenanceCommand(conn, sql.data, echo))
{
*** a/src/bin/scripts/vacuumdb.c
--- b/src/bin/scripts/vacuumdb.c
***************
*** 341,346 **** vacuum_one_database(const char *dbname, vacuumingOptions *vacopts,
--- 341,347 ----
int i;
bool failed = false;
bool parallel = concurrentCons > 1;
+ char *saved_password = NULL;
const char *stage_commands[] = {
"SET default_statistics_target=1; SET vacuum_cost_delay=0;",
"SET default_statistics_target=10; RESET vacuum_cost_delay;",
***************
*** 365,371 **** vacuum_one_database(const char *dbname, vacuumingOptions *vacopts,
fflush(stdout);
}
! conn = connectDatabase(dbname, host, port, username, prompt_password,
progname, false);
initPQExpBuffer(&sql);
--- 366,372 ----
fflush(stdout);
}
! conn = connectDatabase(dbname, host, port, username, &saved_password, prompt_password,
progname, false);
initPQExpBuffer(&sql);
***************
*** 426,437 **** vacuum_one_database(const char *dbname, vacuumingOptions *vacopts,
{
for (i = 1; i < concurrentCons; i++)
{
! conn = connectDatabase(dbname, host, port, username, prompt_password,
progname, false);
init_slot(slots + i, conn);
}
}
/*
* Prepare all the connections to run the appropriate analyze stage, if
* caller requested that mode.
--- 427,442 ----
{
for (i = 1; i < concurrentCons; i++)
{
! conn = connectDatabase(dbname, host, port, username, &saved_password, prompt_password,
progname, false);
init_slot(slots + i, conn);
}
}
+ /* Free up the saved password after it's use */
+ if (saved_password)
+ free(saved_password);
+
/*
* Prepare all the connections to run the appropriate analyze stage, if
* caller requested that mode.
On Thu, Oct 29, 2015 at 9:13 AM, Haribabu Kommi
<kommi.haribabu@gmail.com> wrote:
On Wed, Oct 28, 2015 at 4:07 PM, Haribabu Kommi
<kommi.haribabu@gmail.com> wrote:Here I attached a patch that saves the password provided by the user
from the connectDatabase function and reuse it for connecting all clients
to the same database.Attached a wrong patch, it is having some compilation problems,
Here is the new version with the fixed problems.
I have added an entry in this CF:
https://commitfest.postgresql.org/7/417/
Let's not lose track of this patch.
--
Michael
--
Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-bugs
On Mon, Nov 2, 2015 at 3:56 PM, Michael Paquier
<michael.paquier@gmail.com> wrote:
On Thu, Oct 29, 2015 at 9:13 AM, Haribabu Kommi
<kommi.haribabu@gmail.com> wrote:On Wed, Oct 28, 2015 at 4:07 PM, Haribabu Kommi
<kommi.haribabu@gmail.com> wrote:Here I attached a patch that saves the password provided by the user
from the connectDatabase function and reuse it for connecting all clients
to the same database.Attached a wrong patch, it is having some compilation problems,
Here is the new version with the fixed problems.I have added an entry in this CF:
https://commitfest.postgresql.org/7/417/
Let's not lose track of this patch.
Regarding this patch, wouldn't it be clearer to pass the password as a
variable of connectDatabase()? Then we could use NULL at the first
call of connectDatabase so as we enforce the prompt if requested by
the user. For successive calls of connectDatabase for each worker, we
then fetch the password from the parent connection using that:
if (PQconnectionUsedPassword(con))
password = PQpass(conn);
And pass it as an argument of connectDatabase. In short, I think that
this approach would make a more portable routine because one could
enforce a password at the first call of connectDatabase() without
having to save it once.
--
Michael
--
Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-bugs
On Mon, Nov 2, 2015 at 6:53 PM, Michael Paquier
<michael.paquier@gmail.com> wrote:
On Mon, Nov 2, 2015 at 3:56 PM, Michael Paquier
<michael.paquier@gmail.com> wrote:On Thu, Oct 29, 2015 at 9:13 AM, Haribabu Kommi
<kommi.haribabu@gmail.com> wrote:On Wed, Oct 28, 2015 at 4:07 PM, Haribabu Kommi
<kommi.haribabu@gmail.com> wrote:Here I attached a patch that saves the password provided by the user
from the connectDatabase function and reuse it for connecting all clients
to the same database.Attached a wrong patch, it is having some compilation problems,
Here is the new version with the fixed problems.I have added an entry in this CF:
https://commitfest.postgresql.org/7/417/
Let's not lose track of this patch.
Thanks for adding the patch to commitfest.
Regarding this patch, wouldn't it be clearer to pass the password as a
variable of connectDatabase()? Then we could use NULL at the first
call of connectDatabase so as we enforce the prompt if requested by
the user. For successive calls of connectDatabase for each worker, we
then fetch the password from the parent connection using that:
if (PQconnectionUsedPassword(con))
password = PQpass(conn);
And pass it as an argument of connectDatabase. In short, I think that
this approach would make a more portable routine because one could
enforce a password at the first call of connectDatabase() without
having to save it once.
Thanks for the review.
Here I attached modified patch that gets the password from parent connection
if exists and use it for the child connection also.
Regards,
Hari Babu
Fujitsu Australia
Attachments:
reuse_password_for_vacuum_db_v3.patchapplication/octet-stream; name=reuse_password_for_vacuum_db_v3.patchDownload
*** a/src/bin/scripts/clusterdb.c
--- b/src/bin/scripts/clusterdb.c
***************
*** 203,209 **** cluster_one_database(const char *dbname, bool verbose, const char *table,
appendPQExpBuffer(&sql, " %s", table);
appendPQExpBufferChar(&sql, ';');
! conn = connectDatabase(dbname, host, port, username, prompt_password,
progname, false);
if (!executeMaintenanceCommand(conn, sql.data, echo))
{
--- 203,209 ----
appendPQExpBuffer(&sql, " %s", table);
appendPQExpBufferChar(&sql, ';');
! conn = connectDatabase(NULL, dbname, host, port, username, prompt_password,
progname, false);
if (!executeMaintenanceCommand(conn, sql.data, echo))
{
*** a/src/bin/scripts/common.c
--- b/src/bin/scripts/common.c
***************
*** 56,70 **** handle_help_version_opts(int argc, char *argv[],
* interactive password prompt is automatically issued if required.
*/
PGconn *
! connectDatabase(const char *dbname, const char *pghost, const char *pgport,
! const char *pguser, enum trivalue prompt_password,
const char *progname, bool fail_ok)
{
PGconn *conn;
char *password = NULL;
bool new_pass;
! if (prompt_password == TRI_YES)
password = simple_prompt("Password: ", 100, false);
/*
--- 56,78 ----
* interactive password prompt is automatically issued if required.
*/
PGconn *
! connectDatabase(PGconn *parent_conn, const char *dbname, const char *pghost,
! const char *pgport, const char *pguser, enum trivalue prompt_password,
const char *progname, bool fail_ok)
{
PGconn *conn;
char *password = NULL;
bool new_pass;
! /*
! * Check if any parent connection exists and used a password to
! * connect to the server. If exists, use the same password for the
! * child connection also.
! */
! if (PQconnectionUsedPassword(parent_conn))
! password = PQpass(parent_conn);
!
! if (prompt_password == TRI_YES && password == NULL)
password = simple_prompt("Password: ", 100, false);
/*
***************
*** 116,122 **** connectDatabase(const char *dbname, const char *pghost, const char *pgport,
}
} while (new_pass);
! if (password)
free(password);
/* check to see that the backend connection was successfully made */
--- 124,131 ----
}
} while (new_pass);
! /* Free the password, if it is allocated locally */
! if (password && !PQconnectionUsedPassword(parent_conn))
free(password);
/* check to see that the backend connection was successfully made */
***************
*** 148,161 **** connectMaintenanceDatabase(const char *maintenance_db, const char *pghost,
/* If a maintenance database name was specified, just connect to it. */
if (maintenance_db)
! return connectDatabase(maintenance_db, pghost, pgport, pguser,
prompt_password, progname, false);
/* Otherwise, try postgres first and then template1. */
! conn = connectDatabase("postgres", pghost, pgport, pguser, prompt_password,
progname, true);
if (!conn)
! conn = connectDatabase("template1", pghost, pgport, pguser,
prompt_password, progname, false);
return conn;
--- 157,170 ----
/* If a maintenance database name was specified, just connect to it. */
if (maintenance_db)
! return connectDatabase(NULL, maintenance_db, pghost, pgport, pguser,
prompt_password, progname, false);
/* Otherwise, try postgres first and then template1. */
! conn = connectDatabase(NULL, "postgres", pghost, pgport, pguser, prompt_password,
progname, true);
if (!conn)
! conn = connectDatabase(NULL, "template1", pghost, pgport, pguser,
prompt_password, progname, false);
return conn;
*** a/src/bin/scripts/common.h
--- b/src/bin/scripts/common.h
***************
*** 29,36 **** extern void handle_help_version_opts(int argc, char *argv[],
const char *fixed_progname,
help_handler hlp);
! extern PGconn *connectDatabase(const char *dbname, const char *pghost,
! const char *pgport, const char *pguser,
enum trivalue prompt_password, const char *progname,
bool fail_ok);
--- 29,36 ----
const char *fixed_progname,
help_handler hlp);
! extern PGconn *connectDatabase(PGconn *parent_conn, const char *dbname,
! const char *pghost, const char *pgport, const char *pguser,
enum trivalue prompt_password, const char *progname,
bool fail_ok);
*** a/src/bin/scripts/createlang.c
--- b/src/bin/scripts/createlang.c
***************
*** 140,146 **** main(int argc, char *argv[])
printQueryOpt popt;
static const bool translate_columns[] = {false, true};
! conn = connectDatabase(dbname, host, port, username, prompt_password,
progname, false);
printfPQExpBuffer(&sql, "SELECT lanname as \"%s\", "
--- 140,146 ----
printQueryOpt popt;
static const bool translate_columns[] = {false, true};
! conn = connectDatabase(NULL, dbname, host, port, username, prompt_password,
progname, false);
printfPQExpBuffer(&sql, "SELECT lanname as \"%s\", "
***************
*** 180,186 **** main(int argc, char *argv[])
if (*p >= 'A' && *p <= 'Z')
*p += ('a' - 'A');
! conn = connectDatabase(dbname, host, port, username, prompt_password,
progname, false);
/*
--- 180,186 ----
if (*p >= 'A' && *p <= 'Z')
*p += ('a' - 'A');
! conn = connectDatabase(NULL, dbname, host, port, username, prompt_password,
progname, false);
/*
*** a/src/bin/scripts/createuser.c
--- b/src/bin/scripts/createuser.c
***************
*** 250,256 **** main(int argc, char *argv[])
if (login == 0)
login = TRI_YES;
! conn = connectDatabase("postgres", host, port, username, prompt_password,
progname, false);
initPQExpBuffer(&sql);
--- 250,256 ----
if (login == 0)
login = TRI_YES;
! conn = connectDatabase(NULL, "postgres", host, port, username, prompt_password,
progname, false);
initPQExpBuffer(&sql);
*** a/src/bin/scripts/droplang.c
--- b/src/bin/scripts/droplang.c
***************
*** 139,145 **** main(int argc, char *argv[])
printQueryOpt popt;
static const bool translate_columns[] = {false, true};
! conn = connectDatabase(dbname, host, port, username, prompt_password,
progname, false);
printfPQExpBuffer(&sql, "SELECT lanname as \"%s\", "
--- 139,145 ----
printQueryOpt popt;
static const bool translate_columns[] = {false, true};
! conn = connectDatabase(NULL, dbname, host, port, username, prompt_password,
progname, false);
printfPQExpBuffer(&sql, "SELECT lanname as \"%s\", "
***************
*** 181,187 **** main(int argc, char *argv[])
if (*p >= 'A' && *p <= 'Z')
*p += ('a' - 'A');
! conn = connectDatabase(dbname, host, port, username, prompt_password,
progname, false);
/*
--- 181,187 ----
if (*p >= 'A' && *p <= 'Z')
*p += ('a' - 'A');
! conn = connectDatabase(NULL, dbname, host, port, username, prompt_password,
progname, false);
/*
*** a/src/bin/scripts/dropuser.c
--- b/src/bin/scripts/dropuser.c
***************
*** 128,134 **** main(int argc, char *argv[])
appendPQExpBuffer(&sql, "DROP ROLE %s%s;",
(if_exists ? "IF EXISTS " : ""), fmtId(dropuser));
! conn = connectDatabase("postgres", host, port, username, prompt_password,
progname, false);
if (echo)
--- 128,134 ----
appendPQExpBuffer(&sql, "DROP ROLE %s%s;",
(if_exists ? "IF EXISTS " : ""), fmtId(dropuser));
! conn = connectDatabase(NULL, "postgres", host, port, username, prompt_password,
progname, false);
if (echo)
*** a/src/bin/scripts/reindexdb.c
--- b/src/bin/scripts/reindexdb.c
***************
*** 297,303 **** reindex_one_database(const char *name, const char *dbname, const char *type,
appendPQExpBuffer(&sql, " DATABASE %s", fmtId(name));
appendPQExpBufferChar(&sql, ';');
! conn = connectDatabase(dbname, host, port, username, prompt_password,
progname, false);
if (!executeMaintenanceCommand(conn, sql.data, echo))
--- 297,303 ----
appendPQExpBuffer(&sql, " DATABASE %s", fmtId(name));
appendPQExpBufferChar(&sql, ';');
! conn = connectDatabase(NULL, dbname, host, port, username, prompt_password,
progname, false);
if (!executeMaintenanceCommand(conn, sql.data, echo))
***************
*** 372,378 **** reindex_system_catalogs(const char *dbname, const char *host, const char *port,
appendPQExpBuffer(&sql, " SYSTEM %s;", dbname);
! conn = connectDatabase(dbname, host, port, username, prompt_password,
progname, false);
if (!executeMaintenanceCommand(conn, sql.data, echo))
{
--- 372,378 ----
appendPQExpBuffer(&sql, " SYSTEM %s;", dbname);
! conn = connectDatabase(NULL, dbname, host, port, username, prompt_password,
progname, false);
if (!executeMaintenanceCommand(conn, sql.data, echo))
{
*** a/src/bin/scripts/vacuumdb.c
--- b/src/bin/scripts/vacuumdb.c
***************
*** 365,371 **** vacuum_one_database(const char *dbname, vacuumingOptions *vacopts,
fflush(stdout);
}
! conn = connectDatabase(dbname, host, port, username, prompt_password,
progname, false);
initPQExpBuffer(&sql);
--- 365,371 ----
fflush(stdout);
}
! conn = connectDatabase(NULL, dbname, host, port, username, prompt_password,
progname, false);
initPQExpBuffer(&sql);
***************
*** 424,432 **** vacuum_one_database(const char *dbname, vacuumingOptions *vacopts,
init_slot(slots, conn);
if (parallel)
{
for (i = 1; i < concurrentCons; i++)
{
! conn = connectDatabase(dbname, host, port, username, prompt_password,
progname, false);
init_slot(slots + i, conn);
}
--- 424,434 ----
init_slot(slots, conn);
if (parallel)
{
+ PGconn *parent_conn = conn;
+
for (i = 1; i < concurrentCons; i++)
{
! conn = connectDatabase(parent_conn, dbname, host, port, username, prompt_password,
progname, false);
init_slot(slots + i, conn);
}
On Mon, Nov 2, 2015 at 8:10 PM, Haribabu Kommi wrote:
Here I attached modified patch that gets the password from parent connection
if exists and use it for the child connection also.
Meh? Why the parent connection? You could simply have the password as
an argument of connectDatabase, per se the attached. That looks just
more simple.
--
Michael
Attachments:
20151102_scripts_parallel.patchtext/x-patch; charset=US-ASCII; name=20151102_scripts_parallel.patchDownload
diff --git a/src/bin/scripts/clusterdb.c b/src/bin/scripts/clusterdb.c
index 8c0e7cf..2f15c82 100644
--- a/src/bin/scripts/clusterdb.c
+++ b/src/bin/scripts/clusterdb.c
@@ -203,7 +203,7 @@ cluster_one_database(const char *dbname, bool verbose, const char *table,
appendPQExpBuffer(&sql, " %s", table);
appendPQExpBufferChar(&sql, ';');
- conn = connectDatabase(dbname, host, port, username, prompt_password,
+ conn = connectDatabase(dbname, host, port, username, NULL, prompt_password,
progname, false);
if (!executeMaintenanceCommand(conn, sql.data, echo))
{
diff --git a/src/bin/scripts/common.c b/src/bin/scripts/common.c
index 0deadec..cfac32d 100644
--- a/src/bin/scripts/common.c
+++ b/src/bin/scripts/common.c
@@ -53,18 +53,21 @@ handle_help_version_opts(int argc, char *argv[],
/*
* Make a database connection with the given parameters. An
- * interactive password prompt is automatically issued if required.
+ * interactive password prompt is automatically issued if required
+ * depending on if a user password is defined directly by the caller
+ * of this routine via password_user.
*/
PGconn *
connectDatabase(const char *dbname, const char *pghost, const char *pgport,
- const char *pguser, enum trivalue prompt_password,
- const char *progname, bool fail_ok)
+ const char *pguser, const char *password_user,
+ enum trivalue prompt_password, const char *progname,
+ bool fail_ok)
{
PGconn *conn;
- char *password = NULL;
+ char *password = password_user ? strdup(password_user) : NULL;
bool new_pass;
- if (prompt_password == TRI_YES)
+ if (prompt_password == TRI_YES && password == NULL)
password = simple_prompt("Password: ", 100, false);
/*
@@ -148,14 +151,14 @@ connectMaintenanceDatabase(const char *maintenance_db, const char *pghost,
/* If a maintenance database name was specified, just connect to it. */
if (maintenance_db)
- return connectDatabase(maintenance_db, pghost, pgport, pguser,
+ return connectDatabase(maintenance_db, pghost, pgport, pguser, NULL,
prompt_password, progname, false);
/* Otherwise, try postgres first and then template1. */
- conn = connectDatabase("postgres", pghost, pgport, pguser, prompt_password,
- progname, true);
+ conn = connectDatabase("postgres", pghost, pgport, pguser, NULL,
+ prompt_password, progname, true);
if (!conn)
- conn = connectDatabase("template1", pghost, pgport, pguser,
+ conn = connectDatabase("template1", pghost, pgport, pguser, NULL,
prompt_password, progname, false);
return conn;
diff --git a/src/bin/scripts/common.h b/src/bin/scripts/common.h
index b5ce1ed..13d9d8c 100644
--- a/src/bin/scripts/common.h
+++ b/src/bin/scripts/common.h
@@ -31,8 +31,8 @@ extern void handle_help_version_opts(int argc, char *argv[],
extern PGconn *connectDatabase(const char *dbname, const char *pghost,
const char *pgport, const char *pguser,
- enum trivalue prompt_password, const char *progname,
- bool fail_ok);
+ const char *password_user, enum trivalue prompt_password,
+ const char *progname, bool fail_ok);
extern PGconn *connectMaintenanceDatabase(const char *maintenance_db,
const char *pghost, const char *pgport, const char *pguser,
diff --git a/src/bin/scripts/createlang.c b/src/bin/scripts/createlang.c
index 228215c..38cc20d 100644
--- a/src/bin/scripts/createlang.c
+++ b/src/bin/scripts/createlang.c
@@ -140,8 +140,8 @@ main(int argc, char *argv[])
printQueryOpt popt;
static const bool translate_columns[] = {false, true};
- conn = connectDatabase(dbname, host, port, username, prompt_password,
- progname, false);
+ conn = connectDatabase(dbname, host, port, username, NULL,
+ prompt_password, progname, false);
printfPQExpBuffer(&sql, "SELECT lanname as \"%s\", "
"(CASE WHEN lanpltrusted THEN '%s' ELSE '%s' END) as \"%s\" "
@@ -180,8 +180,8 @@ main(int argc, char *argv[])
if (*p >= 'A' && *p <= 'Z')
*p += ('a' - 'A');
- conn = connectDatabase(dbname, host, port, username, prompt_password,
- progname, false);
+ conn = connectDatabase(dbname, host, port, username, NULL,
+ prompt_password, progname, false);
/*
* Make sure the language isn't already installed
diff --git a/src/bin/scripts/createuser.c b/src/bin/scripts/createuser.c
index c8bcf0d..9e7f84d 100644
--- a/src/bin/scripts/createuser.c
+++ b/src/bin/scripts/createuser.c
@@ -250,8 +250,8 @@ main(int argc, char *argv[])
if (login == 0)
login = TRI_YES;
- conn = connectDatabase("postgres", host, port, username, prompt_password,
- progname, false);
+ conn = connectDatabase("postgres", host, port, username, NULL,
+ prompt_password, progname, false);
initPQExpBuffer(&sql);
diff --git a/src/bin/scripts/droplang.c b/src/bin/scripts/droplang.c
index 7467328..502ec64 100644
--- a/src/bin/scripts/droplang.c
+++ b/src/bin/scripts/droplang.c
@@ -139,8 +139,8 @@ main(int argc, char *argv[])
printQueryOpt popt;
static const bool translate_columns[] = {false, true};
- conn = connectDatabase(dbname, host, port, username, prompt_password,
- progname, false);
+ conn = connectDatabase(dbname, host, port, username, NULL,
+ prompt_password, progname, false);
printfPQExpBuffer(&sql, "SELECT lanname as \"%s\", "
"(CASE WHEN lanpltrusted THEN '%s' ELSE '%s' END) as \"%s\" "
@@ -181,8 +181,8 @@ main(int argc, char *argv[])
if (*p >= 'A' && *p <= 'Z')
*p += ('a' - 'A');
- conn = connectDatabase(dbname, host, port, username, prompt_password,
- progname, false);
+ conn = connectDatabase(dbname, host, port, username, NULL,
+ prompt_password, progname, false);
/*
* Force schema search path to be just pg_catalog, so that we don't have
diff --git a/src/bin/scripts/dropuser.c b/src/bin/scripts/dropuser.c
index 9160566..10d0691 100644
--- a/src/bin/scripts/dropuser.c
+++ b/src/bin/scripts/dropuser.c
@@ -128,8 +128,8 @@ main(int argc, char *argv[])
appendPQExpBuffer(&sql, "DROP ROLE %s%s;",
(if_exists ? "IF EXISTS " : ""), fmtId(dropuser));
- conn = connectDatabase("postgres", host, port, username, prompt_password,
- progname, false);
+ conn = connectDatabase("postgres", host, port, username, NULL,
+ prompt_password, progname, false);
if (echo)
printf("%s\n", sql.data);
diff --git a/src/bin/scripts/reindexdb.c b/src/bin/scripts/reindexdb.c
index decb753..fbf436c 100644
--- a/src/bin/scripts/reindexdb.c
+++ b/src/bin/scripts/reindexdb.c
@@ -297,8 +297,8 @@ reindex_one_database(const char *name, const char *dbname, const char *type,
appendPQExpBuffer(&sql, " DATABASE %s", fmtId(name));
appendPQExpBufferChar(&sql, ';');
- conn = connectDatabase(dbname, host, port, username, prompt_password,
- progname, false);
+ conn = connectDatabase(dbname, host, port, username, NULL,
+ prompt_password, progname, false);
if (!executeMaintenanceCommand(conn, sql.data, echo))
{
@@ -372,8 +372,8 @@ reindex_system_catalogs(const char *dbname, const char *host, const char *port,
appendPQExpBuffer(&sql, " SYSTEM %s;", dbname);
- conn = connectDatabase(dbname, host, port, username, prompt_password,
- progname, false);
+ conn = connectDatabase(dbname, host, port, username, NULL,
+ prompt_password, progname, false);
if (!executeMaintenanceCommand(conn, sql.data, echo))
{
fprintf(stderr, _("%s: reindexing of system catalogs failed: %s"),
diff --git a/src/bin/scripts/vacuumdb.c b/src/bin/scripts/vacuumdb.c
index 4ce27b7..897d8aa 100644
--- a/src/bin/scripts/vacuumdb.c
+++ b/src/bin/scripts/vacuumdb.c
@@ -365,8 +365,8 @@ vacuum_one_database(const char *dbname, vacuumingOptions *vacopts,
fflush(stdout);
}
- conn = connectDatabase(dbname, host, port, username, prompt_password,
- progname, false);
+ conn = connectDatabase(dbname, host, port, username, NULL,
+ prompt_password, progname, false);
initPQExpBuffer(&sql);
@@ -424,10 +424,15 @@ vacuum_one_database(const char *dbname, vacuumingOptions *vacopts,
init_slot(slots, conn);
if (parallel)
{
+ const char *password = NULL;
+
+ if (PQconnectionUsedPassword(conn))
+ password = PQpass(conn);
+
for (i = 1; i < concurrentCons; i++)
{
- conn = connectDatabase(dbname, host, port, username, prompt_password,
- progname, false);
+ conn = connectDatabase(dbname, host, port, username, password,
+ prompt_password, progname, false);
init_slot(slots + i, conn);
}
}
Michael Paquier wrote:
On Mon, Nov 2, 2015 at 8:10 PM, Haribabu Kommi wrote:
Here I attached modified patch that gets the password from parent connection
if exists and use it for the child connection also.Meh? Why the parent connection? You could simply have the password as
an argument of connectDatabase, per se the attached. That looks just
more simple.
Thanks! This is almost there, but there's an important missing piece --
vacuumdb --all will ask you for a password for each database. I think
vacuum_one_database needs to receive a password from its caller too.
(Also, please use pg_strdup.)
--
�lvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
--
Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-bugs
On Tue, Nov 3, 2015 at 6:26 AM, Alvaro Herrera <alvherre@2ndquadrant.com> wrote:
Michael Paquier wrote:
On Mon, Nov 2, 2015 at 8:10 PM, Haribabu Kommi wrote:
Here I attached modified patch that gets the password from parent connection
if exists and use it for the child connection also.Meh? Why the parent connection? You could simply have the password as
an argument of connectDatabase, per se the attached. That looks just
more simple.Thanks! This is almost there, but there's an important missing piece --
vacuumdb --all will ask you for a password for each database. I think
vacuum_one_database needs to receive a password from its caller too.
Here I attached a separate patch to handle the reuse of password for
vacuumdb -all
case. The same behavior exists in all supported branches.
(Also, please use pg_strdup.)
The simple_prompt function in connectDatabase allocates memory for password
using malloc. Because of this reason he used strdup instead of pstrdup to avoid
adding a separate code to handle both the cases.
Regards,
Hari Babu
Fujitsu Australia
Attachments:
reuse_vacuumdb_all_password.patchapplication/octet-stream; name=reuse_vacuumdb_all_password.patchDownload
*** a/src/bin/scripts/vacuumdb.c
--- b/src/bin/scripts/vacuumdb.c
***************
*** 41,48 **** static void vacuum_one_database(const char *dbname, vacuumingOptions *vacopts,
int stage,
SimpleStringList *tables,
const char *host, const char *port,
! const char *username, enum trivalue prompt_password,
! int concurrentCons,
const char *progname, bool echo, bool quiet);
static void vacuum_all_databases(vacuumingOptions *vacopts,
--- 41,48 ----
int stage,
SimpleStringList *tables,
const char *host, const char *port,
! const char *username, char **password_user,
! enum trivalue prompt_password, int concurrentCons,
const char *progname, bool echo, bool quiet);
static void vacuum_all_databases(vacuumingOptions *vacopts,
***************
*** 275,280 **** main(int argc, char *argv[])
--- 275,282 ----
}
else
{
+ char *password = NULL;
+
if (dbname == NULL)
{
if (getenv("PGDATABASE"))
***************
*** 294,300 **** main(int argc, char *argv[])
vacuum_one_database(dbname, &vacopts,
stage,
&tables,
! host, port, username, prompt_password,
concurrentCons,
progname, echo, quiet);
}
--- 296,303 ----
vacuum_one_database(dbname, &vacopts,
stage,
&tables,
! host, port, username,
! &password, prompt_password,
concurrentCons,
progname, echo, quiet);
}
***************
*** 303,311 **** main(int argc, char *argv[])
vacuum_one_database(dbname, &vacopts,
ANALYZE_NO_STAGE,
&tables,
! host, port, username, prompt_password,
concurrentCons,
progname, echo, quiet);
}
exit(0);
--- 306,319 ----
vacuum_one_database(dbname, &vacopts,
ANALYZE_NO_STAGE,
&tables,
! host, port, username,
! &password, prompt_password,
concurrentCons,
progname, echo, quiet);
+
+ /* Free the password memory in case if it supplied */
+ if (password)
+ pfree(password);
}
exit(0);
***************
*** 329,336 **** vacuum_one_database(const char *dbname, vacuumingOptions *vacopts,
int stage,
SimpleStringList *tables,
const char *host, const char *port,
! const char *username, enum trivalue prompt_password,
! int concurrentCons,
const char *progname, bool echo, bool quiet)
{
PQExpBufferData sql;
--- 337,344 ----
int stage,
SimpleStringList *tables,
const char *host, const char *port,
! const char *username, char **password_user,
! enum trivalue prompt_password, int concurrentCons,
const char *progname, bool echo, bool quiet)
{
PQExpBufferData sql;
***************
*** 354,359 **** vacuum_one_database(const char *dbname, vacuumingOptions *vacopts,
--- 362,368 ----
Assert(stage == ANALYZE_NO_STAGE ||
(stage >= 0 && stage < ANALYZE_NUM_STAGES));
+ Assert(password_user != NULL);
if (!quiet)
{
***************
*** 365,373 **** vacuum_one_database(const char *dbname, vacuumingOptions *vacopts,
fflush(stdout);
}
! conn = connectDatabase(dbname, host, port, username, NULL,
prompt_password, progname, false);
initPQExpBuffer(&sql);
/*
--- 374,392 ----
fflush(stdout);
}
! conn = connectDatabase(dbname, host, port, username, *password_user,
prompt_password, progname, false);
+ /*
+ * The following code is required to handle the case of vacuumdb --all.
+ * By the configuration, if user doesn't supply a password during the
+ * connection to the maintainance database and it may be required for
+ * the user database. So we will use this password for the rest of the
+ * connections to the server.
+ */
+ if (PQconnectionUsedPassword(conn) && (*password_user == NULL))
+ *password_user = pstrdup(PQpass(conn));
+
initPQExpBuffer(&sql);
/*
***************
*** 547,555 **** vacuum_all_databases(vacuumingOptions *vacopts,
--- 566,578 ----
PGresult *result;
int stage;
int i;
+ char *password = NULL;
conn = connectMaintenanceDatabase(maintenance_db, host, port,
username, prompt_password, progname);
+ if (PQconnectionUsedPassword(conn))
+ password = pstrdup(PQpass(conn));
+
result = executeQuery(conn,
"SELECT datname FROM pg_database WHERE datallowconn ORDER BY 1;",
progname, echo);
***************
*** 575,581 **** vacuum_all_databases(vacuumingOptions *vacopts,
vacuum_one_database(dbname, vacopts,
stage,
NULL,
! host, port, username, prompt_password,
concurrentCons,
progname, echo, quiet);
}
--- 598,605 ----
vacuum_one_database(dbname, vacopts,
stage,
NULL,
! host, port, username,
! &password, prompt_password,
concurrentCons,
progname, echo, quiet);
}
***************
*** 591,602 **** vacuum_all_databases(vacuumingOptions *vacopts,
vacuum_one_database(dbname, vacopts,
ANALYZE_NO_STAGE,
NULL,
! host, port, username, prompt_password,
concurrentCons,
progname, echo, quiet);
}
}
PQclear(result);
}
--- 615,629 ----
vacuum_one_database(dbname, vacopts,
ANALYZE_NO_STAGE,
NULL,
! host, port, username,
! &password, prompt_password,
concurrentCons,
progname, echo, quiet);
}
}
+ if (password)
+ pfree(password);
PQclear(result);
}
On Tue, Nov 3, 2015 at 6:10 AM, Haribabu Kommi <kommi.haribabu@gmail.com> wrote:
On Tue, Nov 3, 2015 at 6:26 AM, Alvaro Herrera <alvherre@2ndquadrant.com> wrote:
Michael Paquier wrote:
On Mon, Nov 2, 2015 at 8:10 PM, Haribabu Kommi wrote:
Here I attached modified patch that gets the password from parent connection
if exists and use it for the child connection also.Meh? Why the parent connection? You could simply have the password as
an argument of connectDatabase, per se the attached. That looks just
more simple.Thanks! This is almost there, but there's an important missing piece --
vacuumdb --all will ask you for a password for each database. I think
vacuum_one_database needs to receive a password from its caller too.
Argh, right.
Here I attached a separate patch to handle the reuse of password for
vacuumdb -all
case. The same behavior exists in all supported branches.
Sure. Still you don't actually need a double pointer as you do. You
could just reuse the password from the connection obtained via
connectMaintenanceDatabase and pass the password from this connection
as the argument to vacuum_one_database. Something like the attached
seems more elegant IMO.
(Also, please use pg_strdup.)
The simple_prompt function in connectDatabase allocates memory for password
using malloc. Because of this reason he used strdup instead of pstrdup to avoid
adding a separate code to handle both the cases.
Yep. That's the deal.
--
Michael
Attachments:
20151102_scripts_parallel_v2.patchtext/x-patch; charset=US-ASCII; name=20151102_scripts_parallel_v2.patchDownload
diff --git a/src/bin/scripts/clusterdb.c b/src/bin/scripts/clusterdb.c
index 8c0e7cf..2f15c82 100644
--- a/src/bin/scripts/clusterdb.c
+++ b/src/bin/scripts/clusterdb.c
@@ -203,7 +203,7 @@ cluster_one_database(const char *dbname, bool verbose, const char *table,
appendPQExpBuffer(&sql, " %s", table);
appendPQExpBufferChar(&sql, ';');
- conn = connectDatabase(dbname, host, port, username, prompt_password,
+ conn = connectDatabase(dbname, host, port, username, NULL, prompt_password,
progname, false);
if (!executeMaintenanceCommand(conn, sql.data, echo))
{
diff --git a/src/bin/scripts/common.c b/src/bin/scripts/common.c
index 0deadec..321fc10 100644
--- a/src/bin/scripts/common.c
+++ b/src/bin/scripts/common.c
@@ -53,18 +53,21 @@ handle_help_version_opts(int argc, char *argv[],
/*
* Make a database connection with the given parameters. An
- * interactive password prompt is automatically issued if required.
+ * interactive password prompt is automatically issued if required
+ * depending on if a user password is defined directly by the caller
+ * of this routine via password_user.
*/
PGconn *
connectDatabase(const char *dbname, const char *pghost, const char *pgport,
- const char *pguser, enum trivalue prompt_password,
- const char *progname, bool fail_ok)
+ const char *pguser, const char *password_user,
+ enum trivalue prompt_password, const char *progname,
+ bool fail_ok)
{
PGconn *conn;
- char *password = NULL;
+ char *password = password_user ? pg_strdup(password_user) : NULL;
bool new_pass;
- if (prompt_password == TRI_YES)
+ if (prompt_password == TRI_YES && password == NULL)
password = simple_prompt("Password: ", 100, false);
/*
@@ -148,14 +151,14 @@ connectMaintenanceDatabase(const char *maintenance_db, const char *pghost,
/* If a maintenance database name was specified, just connect to it. */
if (maintenance_db)
- return connectDatabase(maintenance_db, pghost, pgport, pguser,
+ return connectDatabase(maintenance_db, pghost, pgport, pguser, NULL,
prompt_password, progname, false);
/* Otherwise, try postgres first and then template1. */
- conn = connectDatabase("postgres", pghost, pgport, pguser, prompt_password,
- progname, true);
+ conn = connectDatabase("postgres", pghost, pgport, pguser, NULL,
+ prompt_password, progname, true);
if (!conn)
- conn = connectDatabase("template1", pghost, pgport, pguser,
+ conn = connectDatabase("template1", pghost, pgport, pguser, NULL,
prompt_password, progname, false);
return conn;
diff --git a/src/bin/scripts/common.h b/src/bin/scripts/common.h
index b5ce1ed..13d9d8c 100644
--- a/src/bin/scripts/common.h
+++ b/src/bin/scripts/common.h
@@ -31,8 +31,8 @@ extern void handle_help_version_opts(int argc, char *argv[],
extern PGconn *connectDatabase(const char *dbname, const char *pghost,
const char *pgport, const char *pguser,
- enum trivalue prompt_password, const char *progname,
- bool fail_ok);
+ const char *password_user, enum trivalue prompt_password,
+ const char *progname, bool fail_ok);
extern PGconn *connectMaintenanceDatabase(const char *maintenance_db,
const char *pghost, const char *pgport, const char *pguser,
diff --git a/src/bin/scripts/createlang.c b/src/bin/scripts/createlang.c
index 228215c..38cc20d 100644
--- a/src/bin/scripts/createlang.c
+++ b/src/bin/scripts/createlang.c
@@ -140,8 +140,8 @@ main(int argc, char *argv[])
printQueryOpt popt;
static const bool translate_columns[] = {false, true};
- conn = connectDatabase(dbname, host, port, username, prompt_password,
- progname, false);
+ conn = connectDatabase(dbname, host, port, username, NULL,
+ prompt_password, progname, false);
printfPQExpBuffer(&sql, "SELECT lanname as \"%s\", "
"(CASE WHEN lanpltrusted THEN '%s' ELSE '%s' END) as \"%s\" "
@@ -180,8 +180,8 @@ main(int argc, char *argv[])
if (*p >= 'A' && *p <= 'Z')
*p += ('a' - 'A');
- conn = connectDatabase(dbname, host, port, username, prompt_password,
- progname, false);
+ conn = connectDatabase(dbname, host, port, username, NULL,
+ prompt_password, progname, false);
/*
* Make sure the language isn't already installed
diff --git a/src/bin/scripts/createuser.c b/src/bin/scripts/createuser.c
index c8bcf0d..9e7f84d 100644
--- a/src/bin/scripts/createuser.c
+++ b/src/bin/scripts/createuser.c
@@ -250,8 +250,8 @@ main(int argc, char *argv[])
if (login == 0)
login = TRI_YES;
- conn = connectDatabase("postgres", host, port, username, prompt_password,
- progname, false);
+ conn = connectDatabase("postgres", host, port, username, NULL,
+ prompt_password, progname, false);
initPQExpBuffer(&sql);
diff --git a/src/bin/scripts/droplang.c b/src/bin/scripts/droplang.c
index 7467328..502ec64 100644
--- a/src/bin/scripts/droplang.c
+++ b/src/bin/scripts/droplang.c
@@ -139,8 +139,8 @@ main(int argc, char *argv[])
printQueryOpt popt;
static const bool translate_columns[] = {false, true};
- conn = connectDatabase(dbname, host, port, username, prompt_password,
- progname, false);
+ conn = connectDatabase(dbname, host, port, username, NULL,
+ prompt_password, progname, false);
printfPQExpBuffer(&sql, "SELECT lanname as \"%s\", "
"(CASE WHEN lanpltrusted THEN '%s' ELSE '%s' END) as \"%s\" "
@@ -181,8 +181,8 @@ main(int argc, char *argv[])
if (*p >= 'A' && *p <= 'Z')
*p += ('a' - 'A');
- conn = connectDatabase(dbname, host, port, username, prompt_password,
- progname, false);
+ conn = connectDatabase(dbname, host, port, username, NULL,
+ prompt_password, progname, false);
/*
* Force schema search path to be just pg_catalog, so that we don't have
diff --git a/src/bin/scripts/dropuser.c b/src/bin/scripts/dropuser.c
index 9160566..10d0691 100644
--- a/src/bin/scripts/dropuser.c
+++ b/src/bin/scripts/dropuser.c
@@ -128,8 +128,8 @@ main(int argc, char *argv[])
appendPQExpBuffer(&sql, "DROP ROLE %s%s;",
(if_exists ? "IF EXISTS " : ""), fmtId(dropuser));
- conn = connectDatabase("postgres", host, port, username, prompt_password,
- progname, false);
+ conn = connectDatabase("postgres", host, port, username, NULL,
+ prompt_password, progname, false);
if (echo)
printf("%s\n", sql.data);
diff --git a/src/bin/scripts/reindexdb.c b/src/bin/scripts/reindexdb.c
index decb753..fbf436c 100644
--- a/src/bin/scripts/reindexdb.c
+++ b/src/bin/scripts/reindexdb.c
@@ -297,8 +297,8 @@ reindex_one_database(const char *name, const char *dbname, const char *type,
appendPQExpBuffer(&sql, " DATABASE %s", fmtId(name));
appendPQExpBufferChar(&sql, ';');
- conn = connectDatabase(dbname, host, port, username, prompt_password,
- progname, false);
+ conn = connectDatabase(dbname, host, port, username, NULL,
+ prompt_password, progname, false);
if (!executeMaintenanceCommand(conn, sql.data, echo))
{
@@ -372,8 +372,8 @@ reindex_system_catalogs(const char *dbname, const char *host, const char *port,
appendPQExpBuffer(&sql, " SYSTEM %s;", dbname);
- conn = connectDatabase(dbname, host, port, username, prompt_password,
- progname, false);
+ conn = connectDatabase(dbname, host, port, username, NULL,
+ prompt_password, progname, false);
if (!executeMaintenanceCommand(conn, sql.data, echo))
{
fprintf(stderr, _("%s: reindexing of system catalogs failed: %s"),
diff --git a/src/bin/scripts/vacuumdb.c b/src/bin/scripts/vacuumdb.c
index 4ce27b7..8c8d105 100644
--- a/src/bin/scripts/vacuumdb.c
+++ b/src/bin/scripts/vacuumdb.c
@@ -41,7 +41,8 @@ static void vacuum_one_database(const char *dbname, vacuumingOptions *vacopts,
int stage,
SimpleStringList *tables,
const char *host, const char *port,
- const char *username, enum trivalue prompt_password,
+ const char *username, const char *password,
+ enum trivalue prompt_password,
int concurrentCons,
const char *progname, bool echo, bool quiet);
@@ -294,7 +295,8 @@ main(int argc, char *argv[])
vacuum_one_database(dbname, &vacopts,
stage,
&tables,
- host, port, username, prompt_password,
+ host, port, username, NULL,
+ prompt_password,
concurrentCons,
progname, echo, quiet);
}
@@ -303,7 +305,8 @@ main(int argc, char *argv[])
vacuum_one_database(dbname, &vacopts,
ANALYZE_NO_STAGE,
&tables,
- host, port, username, prompt_password,
+ host, port, username, NULL,
+ prompt_password,
concurrentCons,
progname, echo, quiet);
}
@@ -323,13 +326,18 @@ main(int argc, char *argv[])
* If concurrentCons is > 1, multiple connections are used to vacuum tables
* in parallel. In this case and if the table list is empty, we first obtain
* a list of tables from the database.
+ *
+ * If 'password' is provided by caller, enforce the password used for
+ * subsequent connections to this value without asking a new value from
+ * prompt.
*/
static void
vacuum_one_database(const char *dbname, vacuumingOptions *vacopts,
int stage,
SimpleStringList *tables,
const char *host, const char *port,
- const char *username, enum trivalue prompt_password,
+ const char *username, const char *password,
+ enum trivalue prompt_password,
int concurrentCons,
const char *progname, bool echo, bool quiet)
{
@@ -365,8 +373,8 @@ vacuum_one_database(const char *dbname, vacuumingOptions *vacopts,
fflush(stdout);
}
- conn = connectDatabase(dbname, host, port, username, prompt_password,
- progname, false);
+ conn = connectDatabase(dbname, host, port, username, password,
+ prompt_password, progname, false);
initPQExpBuffer(&sql);
@@ -424,10 +432,15 @@ vacuum_one_database(const char *dbname, vacuumingOptions *vacopts,
init_slot(slots, conn);
if (parallel)
{
+ const char *password_user = NULL;
+
+ if (PQconnectionUsedPassword(conn))
+ password_user = PQpass(conn);
+
for (i = 1; i < concurrentCons; i++)
{
- conn = connectDatabase(dbname, host, port, username, prompt_password,
- progname, false);
+ conn = connectDatabase(dbname, host, port, username, password_user,
+ prompt_password, progname, false);
init_slot(slots + i, conn);
}
}
@@ -542,12 +555,19 @@ vacuum_all_databases(vacuumingOptions *vacopts,
PGresult *result;
int stage;
int i;
+ char *password = NULL;
conn = connectMaintenanceDatabase(maintenance_db, host, port,
username, prompt_password, progname);
+
result = executeQuery(conn,
"SELECT datname FROM pg_database WHERE datallowconn ORDER BY 1;",
progname, echo);
+
+ /* Save password already given by user for subsequent operations */
+ if (PQconnectionUsedPassword(conn))
+ password = pg_strdup(PQpass(conn));
+
PQfinish(conn);
if (analyze_in_stages)
@@ -570,7 +590,8 @@ vacuum_all_databases(vacuumingOptions *vacopts,
vacuum_one_database(dbname, vacopts,
stage,
NULL,
- host, port, username, prompt_password,
+ host, port, username, password,
+ prompt_password,
concurrentCons,
progname, echo, quiet);
}
@@ -586,13 +607,16 @@ vacuum_all_databases(vacuumingOptions *vacopts,
vacuum_one_database(dbname, vacopts,
ANALYZE_NO_STAGE,
NULL,
- host, port, username, prompt_password,
+ host, port, username, password,
+ prompt_password,
concurrentCons,
progname, echo, quiet);
}
}
PQclear(result);
+ if (password)
+ free(password);
}
/*
On Wed, Nov 4, 2015 at 12:06 AM, Michael Paquier
<michael.paquier@gmail.com> wrote:
On Tue, Nov 3, 2015 at 6:10 AM, Haribabu Kommi <kommi.haribabu@gmail.com> wrote:
Here I attached a separate patch to handle the reuse of password for
vacuumdb -all
case. The same behavior exists in all supported branches.Sure. Still you don't actually need a double pointer as you do. You
could just reuse the password from the connection obtained via
connectMaintenanceDatabase and pass the password from this connection
as the argument to vacuum_one_database. Something like the attached
seems more elegant IMO.
Why I used a double pointer is to support the scenario like the following.
- There is no password requirement for Postgres, template1 and
maintenance db that is provided by the user.
- But there is a password requirement for user databases.
- If user doesn't provide the password during connection to
Maintenance database, later it prompts for
password while connecting to user database.
- Without the double pointer, further on for every database, it
prompts for the password and also
the case of --analyze-in-stages prompts for password for the all the stages.
Regards,
Hari Babu
Fujitsu Australia
--
Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-bugs
On Wed, Nov 4, 2015 at 11:24 AM, Haribabu Kommi
<kommi.haribabu@gmail.com> wrote:
On Wed, Nov 4, 2015 at 12:06 AM, Michael Paquier
<michael.paquier@gmail.com> wrote:On Tue, Nov 3, 2015 at 6:10 AM, Haribabu Kommi <kommi.haribabu@gmail.com> wrote:
Here I attached a separate patch to handle the reuse of password for
vacuumdb -all
case. The same behavior exists in all supported branches.Sure. Still you don't actually need a double pointer as you do. You
could just reuse the password from the connection obtained via
connectMaintenanceDatabase and pass the password from this connection
as the argument to vacuum_one_database. Something like the attached
seems more elegant IMO.Why I used a double pointer is to support the scenario like the following.
- There is no password requirement for Postgres, template1 and
maintenance db that is provided by the user.
- But there is a password requirement for user databases.
- If user doesn't provide the password during connection to
Maintenance database, later it prompts for
password while connecting to user database.
- Without the double pointer, further on for every database, it
prompts for the password and also
the case of --analyze-in-stages prompts for password for the all the stages.
And one more thing, the vacuumdb password behavior is present in back branches
also, is it worth back patching the vacuumdb fix to all supported
branches and apply
the jobs connection fix only to master and 9.5?
Regards,
Hari Babu
Fujitsu Australia
--
Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-bugs
Haribabu Kommi wrote:
On Wed, Nov 4, 2015 at 11:24 AM, Haribabu Kommi
<kommi.haribabu@gmail.com> wrote:
And one more thing, the vacuumdb password behavior is present in back branches
also, is it worth back patching the vacuumdb fix to all supported
branches and apply
the jobs connection fix only to master and 9.5?
Given the lack of complaints, I doubt it's worth the destabilization
risk. Let's just patch 9.5 and be done with it.
--
�lvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
--
Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-bugs
On Wed, Nov 4, 2015 at 9:24 AM, Haribabu Kommi wrote:
On Wed, Nov 4, 2015 at 12:06 AM, Michael Paquier wrote:
Why I used a double pointer is to support the scenario like the following.
- There is no password requirement for Postgres, template1 and
maintenance db that is provided by the user.
- But there is a password requirement for user databases.
- If user doesn't provide the password during connection to
Maintenance database, later it prompts for
password while connecting to user database.
- Without the double pointer, further on for every database, it
prompts for the password and also
the case of --analyze-in-stages prompts for password for the all the stages.
Yeah, I thought a bit about that while writing the last patch but
that's quite narrow, no? So I didn't think it was worth doing, the
approach to request a password only after connecting to the
maintenance database instead of relying on the first one given out by
user looked more simple and solid to me, and is able to cover all the
test scenarios either way as one can specify the maintenance database
he wishes to connect to.
Thoughts from other folks?
--
Michael
--
Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-bugs
On Wed, Nov 4, 2015 at 11:16 AM, Alvaro Herrera
<alvherre@2ndquadrant.com> wrote:
Haribabu Kommi wrote:
On Wed, Nov 4, 2015 at 11:24 AM, Haribabu Kommi
<kommi.haribabu@gmail.com> wrote:And one more thing, the vacuumdb password behavior is present in back branches
also, is it worth back patching the vacuumdb fix to all supported
branches and apply
the jobs connection fix only to master and 9.5?Given the lack of complaints, I doubt it's worth the destabilization
risk. Let's just patch 9.5 and be done with it.
Fine for me. --all is supported for ages.
--
Michael
--
Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-bugs
On Wed, Nov 4, 2015 at 4:30 PM, Michael Paquier
<michael.paquier@gmail.com> wrote:
On Wed, Nov 4, 2015 at 11:16 AM, Alvaro Herrera
<alvherre@2ndquadrant.com> wrote:Haribabu Kommi wrote:
On Wed, Nov 4, 2015 at 11:24 AM, Haribabu Kommi
<kommi.haribabu@gmail.com> wrote:And one more thing, the vacuumdb password behavior is present in back branches
also, is it worth back patching the vacuumdb fix to all supported
branches and apply
the jobs connection fix only to master and 9.5?Given the lack of complaints, I doubt it's worth the destabilization
risk. Let's just patch 9.5 and be done with it.Fine for me. --all is supported for ages.
OK, so attached is a patch aimed at master and 9.5. I reused the
suggestion of Haribabu to not rely completely on the maintenance
database after testing with a couple of database, some of them using
md5 and others trust. That's just more portable this way, and user
just needs to specify the password once to be done even with vacuumdb
--all.
Thoughts?
--
Michael
Attachments:
20151106_scripts_parallel_v3.patchtext/x-patch; charset=US-ASCII; name=20151106_scripts_parallel_v3.patchDownload
diff --git a/src/bin/scripts/clusterdb.c b/src/bin/scripts/clusterdb.c
index 8c0e7cf..2f15c82 100644
--- a/src/bin/scripts/clusterdb.c
+++ b/src/bin/scripts/clusterdb.c
@@ -203,7 +203,7 @@ cluster_one_database(const char *dbname, bool verbose, const char *table,
appendPQExpBuffer(&sql, " %s", table);
appendPQExpBufferChar(&sql, ';');
- conn = connectDatabase(dbname, host, port, username, prompt_password,
+ conn = connectDatabase(dbname, host, port, username, NULL, prompt_password,
progname, false);
if (!executeMaintenanceCommand(conn, sql.data, echo))
{
diff --git a/src/bin/scripts/common.c b/src/bin/scripts/common.c
index 0deadec..cfac32d 100644
--- a/src/bin/scripts/common.c
+++ b/src/bin/scripts/common.c
@@ -53,18 +53,21 @@ handle_help_version_opts(int argc, char *argv[],
/*
* Make a database connection with the given parameters. An
- * interactive password prompt is automatically issued if required.
+ * interactive password prompt is automatically issued if required
+ * depending on if a user password is defined directly by the caller
+ * of this routine via password_user.
*/
PGconn *
connectDatabase(const char *dbname, const char *pghost, const char *pgport,
- const char *pguser, enum trivalue prompt_password,
- const char *progname, bool fail_ok)
+ const char *pguser, const char *password_user,
+ enum trivalue prompt_password, const char *progname,
+ bool fail_ok)
{
PGconn *conn;
- char *password = NULL;
+ char *password = password_user ? strdup(password_user) : NULL;
bool new_pass;
- if (prompt_password == TRI_YES)
+ if (prompt_password == TRI_YES && password == NULL)
password = simple_prompt("Password: ", 100, false);
/*
@@ -148,14 +151,14 @@ connectMaintenanceDatabase(const char *maintenance_db, const char *pghost,
/* If a maintenance database name was specified, just connect to it. */
if (maintenance_db)
- return connectDatabase(maintenance_db, pghost, pgport, pguser,
+ return connectDatabase(maintenance_db, pghost, pgport, pguser, NULL,
prompt_password, progname, false);
/* Otherwise, try postgres first and then template1. */
- conn = connectDatabase("postgres", pghost, pgport, pguser, prompt_password,
- progname, true);
+ conn = connectDatabase("postgres", pghost, pgport, pguser, NULL,
+ prompt_password, progname, true);
if (!conn)
- conn = connectDatabase("template1", pghost, pgport, pguser,
+ conn = connectDatabase("template1", pghost, pgport, pguser, NULL,
prompt_password, progname, false);
return conn;
diff --git a/src/bin/scripts/common.h b/src/bin/scripts/common.h
index b5ce1ed..13d9d8c 100644
--- a/src/bin/scripts/common.h
+++ b/src/bin/scripts/common.h
@@ -31,8 +31,8 @@ extern void handle_help_version_opts(int argc, char *argv[],
extern PGconn *connectDatabase(const char *dbname, const char *pghost,
const char *pgport, const char *pguser,
- enum trivalue prompt_password, const char *progname,
- bool fail_ok);
+ const char *password_user, enum trivalue prompt_password,
+ const char *progname, bool fail_ok);
extern PGconn *connectMaintenanceDatabase(const char *maintenance_db,
const char *pghost, const char *pgport, const char *pguser,
diff --git a/src/bin/scripts/createlang.c b/src/bin/scripts/createlang.c
index 228215c..38cc20d 100644
--- a/src/bin/scripts/createlang.c
+++ b/src/bin/scripts/createlang.c
@@ -140,8 +140,8 @@ main(int argc, char *argv[])
printQueryOpt popt;
static const bool translate_columns[] = {false, true};
- conn = connectDatabase(dbname, host, port, username, prompt_password,
- progname, false);
+ conn = connectDatabase(dbname, host, port, username, NULL,
+ prompt_password, progname, false);
printfPQExpBuffer(&sql, "SELECT lanname as \"%s\", "
"(CASE WHEN lanpltrusted THEN '%s' ELSE '%s' END) as \"%s\" "
@@ -180,8 +180,8 @@ main(int argc, char *argv[])
if (*p >= 'A' && *p <= 'Z')
*p += ('a' - 'A');
- conn = connectDatabase(dbname, host, port, username, prompt_password,
- progname, false);
+ conn = connectDatabase(dbname, host, port, username, NULL,
+ prompt_password, progname, false);
/*
* Make sure the language isn't already installed
diff --git a/src/bin/scripts/createuser.c b/src/bin/scripts/createuser.c
index c8bcf0d..9e7f84d 100644
--- a/src/bin/scripts/createuser.c
+++ b/src/bin/scripts/createuser.c
@@ -250,8 +250,8 @@ main(int argc, char *argv[])
if (login == 0)
login = TRI_YES;
- conn = connectDatabase("postgres", host, port, username, prompt_password,
- progname, false);
+ conn = connectDatabase("postgres", host, port, username, NULL,
+ prompt_password, progname, false);
initPQExpBuffer(&sql);
diff --git a/src/bin/scripts/droplang.c b/src/bin/scripts/droplang.c
index 7467328..502ec64 100644
--- a/src/bin/scripts/droplang.c
+++ b/src/bin/scripts/droplang.c
@@ -139,8 +139,8 @@ main(int argc, char *argv[])
printQueryOpt popt;
static const bool translate_columns[] = {false, true};
- conn = connectDatabase(dbname, host, port, username, prompt_password,
- progname, false);
+ conn = connectDatabase(dbname, host, port, username, NULL,
+ prompt_password, progname, false);
printfPQExpBuffer(&sql, "SELECT lanname as \"%s\", "
"(CASE WHEN lanpltrusted THEN '%s' ELSE '%s' END) as \"%s\" "
@@ -181,8 +181,8 @@ main(int argc, char *argv[])
if (*p >= 'A' && *p <= 'Z')
*p += ('a' - 'A');
- conn = connectDatabase(dbname, host, port, username, prompt_password,
- progname, false);
+ conn = connectDatabase(dbname, host, port, username, NULL,
+ prompt_password, progname, false);
/*
* Force schema search path to be just pg_catalog, so that we don't have
diff --git a/src/bin/scripts/dropuser.c b/src/bin/scripts/dropuser.c
index 9160566..10d0691 100644
--- a/src/bin/scripts/dropuser.c
+++ b/src/bin/scripts/dropuser.c
@@ -128,8 +128,8 @@ main(int argc, char *argv[])
appendPQExpBuffer(&sql, "DROP ROLE %s%s;",
(if_exists ? "IF EXISTS " : ""), fmtId(dropuser));
- conn = connectDatabase("postgres", host, port, username, prompt_password,
- progname, false);
+ conn = connectDatabase("postgres", host, port, username, NULL,
+ prompt_password, progname, false);
if (echo)
printf("%s\n", sql.data);
diff --git a/src/bin/scripts/reindexdb.c b/src/bin/scripts/reindexdb.c
index decb753..fbf436c 100644
--- a/src/bin/scripts/reindexdb.c
+++ b/src/bin/scripts/reindexdb.c
@@ -297,8 +297,8 @@ reindex_one_database(const char *name, const char *dbname, const char *type,
appendPQExpBuffer(&sql, " DATABASE %s", fmtId(name));
appendPQExpBufferChar(&sql, ';');
- conn = connectDatabase(dbname, host, port, username, prompt_password,
- progname, false);
+ conn = connectDatabase(dbname, host, port, username, NULL,
+ prompt_password, progname, false);
if (!executeMaintenanceCommand(conn, sql.data, echo))
{
@@ -372,8 +372,8 @@ reindex_system_catalogs(const char *dbname, const char *host, const char *port,
appendPQExpBuffer(&sql, " SYSTEM %s;", dbname);
- conn = connectDatabase(dbname, host, port, username, prompt_password,
- progname, false);
+ conn = connectDatabase(dbname, host, port, username, NULL,
+ prompt_password, progname, false);
if (!executeMaintenanceCommand(conn, sql.data, echo))
{
fprintf(stderr, _("%s: reindexing of system catalogs failed: %s"),
diff --git a/src/bin/scripts/vacuumdb.c b/src/bin/scripts/vacuumdb.c
index 4ce27b7..67cb975 100644
--- a/src/bin/scripts/vacuumdb.c
+++ b/src/bin/scripts/vacuumdb.c
@@ -41,7 +41,8 @@ static void vacuum_one_database(const char *dbname, vacuumingOptions *vacopts,
int stage,
SimpleStringList *tables,
const char *host, const char *port,
- const char *username, enum trivalue prompt_password,
+ const char *username, char **password,
+ enum trivalue prompt_password,
int concurrentCons,
const char *progname, bool echo, bool quiet);
@@ -275,6 +276,8 @@ main(int argc, char *argv[])
}
else
{
+ char *password = NULL;
+
if (dbname == NULL)
{
if (getenv("PGDATABASE"))
@@ -294,7 +297,8 @@ main(int argc, char *argv[])
vacuum_one_database(dbname, &vacopts,
stage,
&tables,
- host, port, username, prompt_password,
+ host, port, username, &password,
+ prompt_password,
concurrentCons,
progname, echo, quiet);
}
@@ -303,9 +307,14 @@ main(int argc, char *argv[])
vacuum_one_database(dbname, &vacopts,
ANALYZE_NO_STAGE,
&tables,
- host, port, username, prompt_password,
+ host, port, username, &password,
+ prompt_password,
concurrentCons,
progname, echo, quiet);
+
+ /* free the password if supplied by user */
+ if (password)
+ pfree(password);
}
exit(0);
@@ -323,13 +332,18 @@ main(int argc, char *argv[])
* If concurrentCons is > 1, multiple connections are used to vacuum tables
* in parallel. In this case and if the table list is empty, we first obtain
* a list of tables from the database.
+ *
+ * If 'password' is provided by caller, enforce the password used for
+ * subsequent connections to this value without asking a new value from
+ * prompt.
*/
static void
vacuum_one_database(const char *dbname, vacuumingOptions *vacopts,
int stage,
SimpleStringList *tables,
const char *host, const char *port,
- const char *username, enum trivalue prompt_password,
+ const char *username, char **password,
+ enum trivalue prompt_password,
int concurrentCons,
const char *progname, bool echo, bool quiet)
{
@@ -365,8 +379,19 @@ vacuum_one_database(const char *dbname, vacuumingOptions *vacopts,
fflush(stdout);
}
- conn = connectDatabase(dbname, host, port, username, prompt_password,
- progname, false);
+ conn = connectDatabase(dbname, host, port, username, *password,
+ prompt_password, progname, false);
+
+ /*
+ * If no password has been specified by the caller of this routine, save
+ * any password used by the connection previously established. This is
+ * useful from the user prospective to not have to specify multiple times
+ * the same password, particularly in the case of vacuum_all_databases
+ * where the maintenance database used for the first connection may not
+ * have needed a password for connection.
+ */
+ if (PQconnectionUsedPassword(conn) && *password == NULL)
+ *password = pstrdup(PQpass(conn));
initPQExpBuffer(&sql);
@@ -424,10 +449,15 @@ vacuum_one_database(const char *dbname, vacuumingOptions *vacopts,
init_slot(slots, conn);
if (parallel)
{
+ const char *password_user = NULL;
+
+ if (PQconnectionUsedPassword(conn))
+ password_user = PQpass(conn);
+
for (i = 1; i < concurrentCons; i++)
{
- conn = connectDatabase(dbname, host, port, username, prompt_password,
- progname, false);
+ conn = connectDatabase(dbname, host, port, username, password_user,
+ prompt_password, progname, false);
init_slot(slots + i, conn);
}
}
@@ -542,12 +572,19 @@ vacuum_all_databases(vacuumingOptions *vacopts,
PGresult *result;
int stage;
int i;
+ char *password = NULL;
conn = connectMaintenanceDatabase(maintenance_db, host, port,
username, prompt_password, progname);
+
result = executeQuery(conn,
"SELECT datname FROM pg_database WHERE datallowconn ORDER BY 1;",
progname, echo);
+
+ /* Save password already given by user for subsequent operations */
+ if (PQconnectionUsedPassword(conn))
+ password = pg_strdup(PQpass(conn));
+
PQfinish(conn);
if (analyze_in_stages)
@@ -570,7 +607,8 @@ vacuum_all_databases(vacuumingOptions *vacopts,
vacuum_one_database(dbname, vacopts,
stage,
NULL,
- host, port, username, prompt_password,
+ host, port, username, &password,
+ prompt_password,
concurrentCons,
progname, echo, quiet);
}
@@ -586,13 +624,16 @@ vacuum_all_databases(vacuumingOptions *vacopts,
vacuum_one_database(dbname, vacopts,
ANALYZE_NO_STAGE,
NULL,
- host, port, username, prompt_password,
+ host, port, username, &password,
+ prompt_password,
concurrentCons,
progname, echo, quiet);
}
}
PQclear(result);
+ if (password)
+ free(password);
}
/*
On Fri, Nov 6, 2015 at 5:53 PM, Michael Paquier
<michael.paquier@gmail.com> wrote:
On Wed, Nov 4, 2015 at 4:30 PM, Michael Paquier
<michael.paquier@gmail.com> wrote:On Wed, Nov 4, 2015 at 11:16 AM, Alvaro Herrera
<alvherre@2ndquadrant.com> wrote:Haribabu Kommi wrote:
On Wed, Nov 4, 2015 at 11:24 AM, Haribabu Kommi
<kommi.haribabu@gmail.com> wrote:And one more thing, the vacuumdb password behavior is present in back branches
also, is it worth back patching the vacuumdb fix to all supported
branches and apply
the jobs connection fix only to master and 9.5?Given the lack of complaints, I doubt it's worth the destabilization
risk. Let's just patch 9.5 and be done with it.Fine for me. --all is supported for ages.
OK, so attached is a patch aimed at master and 9.5. I reused the
suggestion of Haribabu to not rely completely on the maintenance
database after testing with a couple of database, some of them using
md5 and others trust. That's just more portable this way, and user
just needs to specify the password once to be done even with vacuumdb
--all.
Thoughts?
Thanks for the patch. Yes, reusing the password helps for --analyze-in-stages
for single database case also along with -all.
Regards,
Hari Babu
Fujitsu Australia
--
Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-bugs
On Fri, Nov 6, 2015 at 3:53 PM, Michael Paquier
<michael.paquier@gmail.com> wrote:
On Wed, Nov 4, 2015 at 4:30 PM, Michael Paquier
<michael.paquier@gmail.com> wrote:On Wed, Nov 4, 2015 at 11:16 AM, Alvaro Herrera
<alvherre@2ndquadrant.com> wrote:Haribabu Kommi wrote:
On Wed, Nov 4, 2015 at 11:24 AM, Haribabu Kommi
<kommi.haribabu@gmail.com> wrote:And one more thing, the vacuumdb password behavior is present in back branches
also, is it worth back patching the vacuumdb fix to all supported
branches and apply
the jobs connection fix only to master and 9.5?Given the lack of complaints, I doubt it's worth the destabilization
risk. Let's just patch 9.5 and be done with it.Fine for me. --all is supported for ages.
OK, so attached is a patch aimed at master and 9.5. I reused the
suggestion of Haribabu to not rely completely on the maintenance
database after testing with a couple of database, some of them using
md5 and others trust. That's just more portable this way, and user
just needs to specify the password once to be done even with vacuumdb
--all.
Thoughts?
ISTM that the attached simpler patch can fix the problem.
But maybe I'm missing something...
Regards,
--
Fujii Masao
Attachments:
vacuumdb_pass_v1.patchapplication/octet-stream; name=vacuumdb_pass_v1.patchDownload
*** a/src/bin/scripts/common.c
--- b/src/bin/scripts/common.c
***************
*** 61,70 **** connectDatabase(const char *dbname, const char *pghost, const char *pgport,
const char *progname, bool fail_ok)
{
PGconn *conn;
! char *password = NULL;
bool new_pass;
! if (prompt_password == TRI_YES)
password = simple_prompt("Password: ", 100, false);
/*
--- 61,70 ----
const char *progname, bool fail_ok)
{
PGconn *conn;
! static char *password = NULL;
bool new_pass;
! if (prompt_password == TRI_YES && password == NULL)
password = simple_prompt("Password: ", 100, false);
/*
***************
*** 116,124 **** connectDatabase(const char *dbname, const char *pghost, const char *pgport,
}
} while (new_pass);
- if (password)
- free(password);
-
/* check to see that the backend connection was successfully made */
if (PQstatus(conn) == CONNECTION_BAD)
{
--- 116,121 ----
Fujii Masao wrote:
ISTM that the attached simpler patch can fix the problem.
But maybe I'm missing something...
Hmm, this is a very good and simple idea that looks like it should do
the trick. I would just add a comment explaining why we're using a
static.
--
�lvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
--
Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-bugs
Alvaro Herrera <alvherre@2ndquadrant.com> writes:
Fujii Masao wrote:
ISTM that the attached simpler patch can fix the problem.
But maybe I'm missing something...
Hmm, this is a very good and simple idea that looks like it should do
the trick. I would just add a comment explaining why we're using a
static.
NAK. This changes the behavior of connectDatabase() for *all* users
of that function, not only vacuumdb. But the proposed behavioral
change is only appropriate for calling programs in which only a single
host/port/database target is used per execution. In other contexts,
reusing the prior password is not just inappropriate but could actually
create security issues. (It's possible that this behavior would be okay
for all existing callers, but that doesn't mean we should put in a
security gotcha for future uses.)
We could make this approach work if connectDatabase() remembered all
the parameters internally, and only tried to reuse the password when
they all match. Or maybe it'd be better to alter the API so the caller
can say whether to try to reuse a saved password or not. But I'm not sure
whether either of those answers is cleaner than the previous patch.
(BTW, I notice that pg_dumpall.c has a version of connectDatabase in
which the "static" trick is already being used, sans any documentation.
That's okay for pg_dumpall, but might be an issue if anyone copies-and-
pastes that version somewhere else ... and in any case it's fair to ask
why that version hasn't been merged with common.c.)
regards, tom lane
--
Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-bugs
Michael Paquier wrote:
OK, so attached is a patch aimed at master and 9.5. I reused the
suggestion of Haribabu to not rely completely on the maintenance
database after testing with a couple of database, some of them using
md5 and others trust. That's just more portable this way, and user
just needs to specify the password once to be done even with vacuumdb
--all.
Thanks, pushed a slightly tweaked version. If you can please verify
that I didn't break anything, it'd be great.
FWIW I think the mode that generated the most connections is -jN --all
--analyze-in-stages. For additional annoyance, one sets up the
"postgres" database with trust auth and others with md5. All the cases
I tried only get a single prompt now.
Thanks Eric for reporting the problem and Haribabu and Michael for
patching.
--
�lvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
--
Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-bugs
Alvaro Herrera <alvherre@2ndquadrant.com> writes:
Thanks, pushed a slightly tweaked version.
? No push visible from here ...
regards, tom lane
--
Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-bugs
Tom Lane wrote:
Alvaro Herrera <alvherre@2ndquadrant.com> writes:
Thanks, pushed a slightly tweaked version.
? No push visible from here ...
Uh, I had only made the dry-run one. Did the real one now.
--
�lvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
--
Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-bugs
Tom Lane wrote:
NAK. This changes the behavior of connectDatabase() for *all* users
of that function, not only vacuumdb. But the proposed behavioral
change is only appropriate for calling programs in which only a single
host/port/database target is used per execution. In other contexts,
reusing the prior password is not just inappropriate but could actually
create security issues. (It's possible that this behavior would be okay
for all existing callers, but that doesn't mean we should put in a
security gotcha for future uses.)We could make this approach work if connectDatabase() remembered all
the parameters internally, and only tried to reuse the password when
they all match. Or maybe it'd be better to alter the API so the caller
can say whether to try to reuse a saved password or not. But I'm not sure
whether either of those answers is cleaner than the previous patch.
Thanks for the input. I decided to push what we had because it's less
invasive in terms of API definition. If we want to change in the
direction suggested by Masao-san, we can still do it, but perhaps only
in master -- maybe we would like to have both pg_dump and
src/bin/scripts compile a single source code file instead of having two
copies of essentially the same routine, for instance.
(BTW, I notice that pg_dumpall.c has a version of connectDatabase in
which the "static" trick is already being used, sans any documentation.
That's okay for pg_dumpall, but might be an issue if anyone copies-and-
pastes that version somewhere else ... and in any case it's fair to ask
why that version hasn't been merged with common.c.)
We'd have to have a file common to both subdirs that only contains that
routine, I think. We now have pg_dump/common.c as well ... Not
something I want to propose for 9.5.
--
�lvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
--
Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-bugs
Alvaro Herrera <alvherre@2ndquadrant.com> writes:
Thanks for the input. I decided to push what we had because it's less
invasive in terms of API definition.
I dunno, this might be easier for the callers that don't want password
re-use, but it seems quite horrid for ones that do. The changes to
vacuumdb.c are, frankly, seriously ugly; and they require vacuumdb.c
to know a lot more than before about password handling.
Other notes are that the strdup() call should surely be pg_strdup(),
and the mix of free() and pg_free() is at best unsightly.
On the whole I don't think this was ready to push.
The place I was thinking we might end up was something like Fujii-san's
patch plus a new bool parameter "allow_password_reuse", which could be
passed as false in cases where the old behavior seems preferable.
regards, tom lane
--
Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-bugs
Tom Lane wrote:
Alvaro Herrera <alvherre@2ndquadrant.com> writes:
Thanks for the input. I decided to push what we had because it's less
invasive in terms of API definition.I dunno, this might be easier for the callers that don't want password
re-use, but it seems quite horrid for ones that do. The changes to
vacuumdb.c are, frankly, seriously ugly; and they require vacuumdb.c
to know a lot more than before about password handling.
Yes, it is ugly.
Other notes are that the strdup() call should surely be pg_strdup(),
and the mix of free() and pg_free() is at best unsightly.
I had the same comment, but it turns out that free and strdup must be
used instead because sprompt.c uses malloc in the equivalent places; and
we can't use pg_malloc there because it's in src/port which isn't
allowed to use fe_memutils. We would have to move it from src/port to
src/common first.
The place I was thinking we might end up was something like Fujii-san's
patch plus a new bool parameter "allow_password_reuse", which could be
passed as false in cases where the old behavior seems preferable.
Hm, we can try that.
--
�lvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
--
Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-bugs
On Fri, Nov 13, 2015 at 6:58 AM, Alvaro Herrera wrote:
Tom Lane wrote:
Alvaro Herrera <alvherre@2ndquadrant.com> writes:
The place I was thinking we might end up was something like Fujii-san's
patch plus a new bool parameter "allow_password_reuse", which could be
passed as false in cases where the old behavior seems preferable.Hm, we can try that.
That's neater. See for example the attached.
--
Michael
Attachments:
20151114_scripts_parallel_v5.patchtext/x-diff; charset=US-ASCII; name=20151114_scripts_parallel_v5.patchDownload
diff --git a/src/bin/scripts/clusterdb.c b/src/bin/scripts/clusterdb.c
index 2f15c82..f87a9ce 100644
--- a/src/bin/scripts/clusterdb.c
+++ b/src/bin/scripts/clusterdb.c
@@ -203,8 +203,8 @@ cluster_one_database(const char *dbname, bool verbose, const char *table,
appendPQExpBuffer(&sql, " %s", table);
appendPQExpBufferChar(&sql, ';');
- conn = connectDatabase(dbname, host, port, username, NULL, prompt_password,
- progname, false);
+ conn = connectDatabase(dbname, host, port, username, prompt_password,
+ progname, false, false);
if (!executeMaintenanceCommand(conn, sql.data, echo))
{
if (table)
diff --git a/src/bin/scripts/common.c b/src/bin/scripts/common.c
index d26a4ed..83ddb00d 100644
--- a/src/bin/scripts/common.c
+++ b/src/bin/scripts/common.c
@@ -54,22 +54,27 @@ handle_help_version_opts(int argc, char *argv[],
/*
* Make a database connection with the given parameters.
*
- * A password can be given, but if not (or if user forces us to) we prompt
- * interactively for one, unless caller prohibited us from doing so.
+ * Interactive password prompt is automatically issued if required, and
+ * existing password can be reused as well by user in case of repetitive
+ * calls to this routine.
*/
PGconn *
connectDatabase(const char *dbname, const char *pghost, const char *pgport,
- const char *pguser, const char *pgpassword,
- enum trivalue prompt_password, const char *progname,
- bool fail_ok)
+ const char *pguser, enum trivalue prompt_password,
+ const char *progname, bool fail_ok, bool allow_password_reuse)
{
- PGconn *conn;
- char *password;
- bool new_pass;
+ PGconn *conn;
+ static char *password = NULL;
+ bool new_pass;
- password = pgpassword ? strdup(pgpassword) : NULL;
+ if (!allow_password_reuse)
+ {
+ if (password)
+ free(password);
+ password = NULL;
+ }
- if (prompt_password == TRI_YES && !pgpassword)
+ if (password == NULL && prompt_password == TRI_YES)
password = simple_prompt("Password: ", 100, false);
/*
@@ -100,34 +105,27 @@ connectDatabase(const char *dbname, const char *pghost, const char *pgport,
new_pass = false;
conn = PQconnectdbParams(keywords, values, true);
+ free(keywords);
+ free(values);
+
if (!conn)
{
- fprintf(stderr, _("%s: could not connect to database %s: out of memory\n"),
+ fprintf(stderr, _("%s: could not connect to database %s\n"),
progname, dbname);
exit(1);
}
- pg_free(keywords);
- pg_free(values);
-
- /*
- * No luck? Trying asking (again) for a password.
- */
if (PQstatus(conn) == CONNECTION_BAD &&
PQconnectionNeedsPassword(conn) &&
+ password == NULL &&
prompt_password != TRI_NO)
{
PQfinish(conn);
- if (password)
- free(password);
password = simple_prompt("Password: ", 100, false);
new_pass = true;
}
} while (new_pass);
- if (password)
- free(password);
-
/* check to see that the backend connection was successfully made */
if (PQstatus(conn) == CONNECTION_BAD)
{
@@ -157,15 +155,15 @@ connectMaintenanceDatabase(const char *maintenance_db, const char *pghost,
/* If a maintenance database name was specified, just connect to it. */
if (maintenance_db)
- return connectDatabase(maintenance_db, pghost, pgport, pguser, NULL,
- prompt_password, progname, false);
+ return connectDatabase(maintenance_db, pghost, pgport, pguser,
+ prompt_password, progname, false, false);
/* Otherwise, try postgres first and then template1. */
- conn = connectDatabase("postgres", pghost, pgport, pguser, NULL,
- prompt_password, progname, true);
+ conn = connectDatabase("postgres", pghost, pgport, pguser, prompt_password,
+ progname, true, false);
if (!conn)
- conn = connectDatabase("template1", pghost, pgport, pguser, NULL,
- prompt_password, progname, false);
+ conn = connectDatabase("template1", pghost, pgport, pguser,
+ prompt_password, progname, false, false);
return conn;
}
diff --git a/src/bin/scripts/common.h b/src/bin/scripts/common.h
index dafd58f..9038295 100644
--- a/src/bin/scripts/common.h
+++ b/src/bin/scripts/common.h
@@ -31,8 +31,8 @@ extern void handle_help_version_opts(int argc, char *argv[],
extern PGconn *connectDatabase(const char *dbname, const char *pghost,
const char *pgport, const char *pguser,
- const char *pgpassword, enum trivalue prompt_password,
- const char *progname, bool fail_ok);
+ enum trivalue prompt_password, const char *progname,
+ bool fail_ok, bool allow_password_reuse);
extern PGconn *connectMaintenanceDatabase(const char *maintenance_db,
const char *pghost, const char *pgport, const char *pguser,
diff --git a/src/bin/scripts/createlang.c b/src/bin/scripts/createlang.c
index 38cc20d..3120e2f 100644
--- a/src/bin/scripts/createlang.c
+++ b/src/bin/scripts/createlang.c
@@ -140,8 +140,8 @@ main(int argc, char *argv[])
printQueryOpt popt;
static const bool translate_columns[] = {false, true};
- conn = connectDatabase(dbname, host, port, username, NULL,
- prompt_password, progname, false);
+ conn = connectDatabase(dbname, host, port, username, prompt_password,
+ progname, false, false);
printfPQExpBuffer(&sql, "SELECT lanname as \"%s\", "
"(CASE WHEN lanpltrusted THEN '%s' ELSE '%s' END) as \"%s\" "
@@ -180,8 +180,8 @@ main(int argc, char *argv[])
if (*p >= 'A' && *p <= 'Z')
*p += ('a' - 'A');
- conn = connectDatabase(dbname, host, port, username, NULL,
- prompt_password, progname, false);
+ conn = connectDatabase(dbname, host, port, username, prompt_password,
+ progname, false, false);
/*
* Make sure the language isn't already installed
diff --git a/src/bin/scripts/createuser.c b/src/bin/scripts/createuser.c
index 9e7f84d..dc358e4 100644
--- a/src/bin/scripts/createuser.c
+++ b/src/bin/scripts/createuser.c
@@ -250,8 +250,8 @@ main(int argc, char *argv[])
if (login == 0)
login = TRI_YES;
- conn = connectDatabase("postgres", host, port, username, NULL,
- prompt_password, progname, false);
+ conn = connectDatabase("postgres", host, port, username, prompt_password,
+ progname, false, false);
initPQExpBuffer(&sql);
diff --git a/src/bin/scripts/droplang.c b/src/bin/scripts/droplang.c
index 502ec64..b2d2996 100644
--- a/src/bin/scripts/droplang.c
+++ b/src/bin/scripts/droplang.c
@@ -139,8 +139,8 @@ main(int argc, char *argv[])
printQueryOpt popt;
static const bool translate_columns[] = {false, true};
- conn = connectDatabase(dbname, host, port, username, NULL,
- prompt_password, progname, false);
+ conn = connectDatabase(dbname, host, port, username, prompt_password,
+ progname, false, false);
printfPQExpBuffer(&sql, "SELECT lanname as \"%s\", "
"(CASE WHEN lanpltrusted THEN '%s' ELSE '%s' END) as \"%s\" "
@@ -181,8 +181,8 @@ main(int argc, char *argv[])
if (*p >= 'A' && *p <= 'Z')
*p += ('a' - 'A');
- conn = connectDatabase(dbname, host, port, username, NULL,
- prompt_password, progname, false);
+ conn = connectDatabase(dbname, host, port, username, prompt_password,
+ progname, false, false);
/*
* Force schema search path to be just pg_catalog, so that we don't have
diff --git a/src/bin/scripts/dropuser.c b/src/bin/scripts/dropuser.c
index 10d0691..1b0cf78 100644
--- a/src/bin/scripts/dropuser.c
+++ b/src/bin/scripts/dropuser.c
@@ -128,8 +128,8 @@ main(int argc, char *argv[])
appendPQExpBuffer(&sql, "DROP ROLE %s%s;",
(if_exists ? "IF EXISTS " : ""), fmtId(dropuser));
- conn = connectDatabase("postgres", host, port, username, NULL,
- prompt_password, progname, false);
+ conn = connectDatabase("postgres", host, port, username, prompt_password,
+ progname, false, false);
if (echo)
printf("%s\n", sql.data);
diff --git a/src/bin/scripts/reindexdb.c b/src/bin/scripts/reindexdb.c
index fbf436c..b6f3dcb 100644
--- a/src/bin/scripts/reindexdb.c
+++ b/src/bin/scripts/reindexdb.c
@@ -297,8 +297,8 @@ reindex_one_database(const char *name, const char *dbname, const char *type,
appendPQExpBuffer(&sql, " DATABASE %s", fmtId(name));
appendPQExpBufferChar(&sql, ';');
- conn = connectDatabase(dbname, host, port, username, NULL,
- prompt_password, progname, false);
+ conn = connectDatabase(dbname, host, port, username, prompt_password,
+ progname, false, false);
if (!executeMaintenanceCommand(conn, sql.data, echo))
{
@@ -372,8 +372,8 @@ reindex_system_catalogs(const char *dbname, const char *host, const char *port,
appendPQExpBuffer(&sql, " SYSTEM %s;", dbname);
- conn = connectDatabase(dbname, host, port, username, NULL,
- prompt_password, progname, false);
+ conn = connectDatabase(dbname, host, port, username, prompt_password,
+ progname, false, false);
if (!executeMaintenanceCommand(conn, sql.data, echo))
{
fprintf(stderr, _("%s: reindexing of system catalogs failed: %s"),
diff --git a/src/bin/scripts/vacuumdb.c b/src/bin/scripts/vacuumdb.c
index 6c67263..c8b40de 100644
--- a/src/bin/scripts/vacuumdb.c
+++ b/src/bin/scripts/vacuumdb.c
@@ -43,8 +43,7 @@ static void vacuum_one_database(const char *dbname, vacuumingOptions *vacopts,
const char *host, const char *port,
const char *username, enum trivalue prompt_password,
int concurrentCons,
- const char *progname, bool echo, bool quiet,
- char **password);
+ const char *progname, bool echo, bool quiet);
static void vacuum_all_databases(vacuumingOptions *vacopts,
bool analyze_in_stages,
@@ -276,8 +275,6 @@ main(int argc, char *argv[])
}
else
{
- char *password = NULL;
-
if (dbname == NULL)
{
if (getenv("PGDATABASE"))
@@ -299,8 +296,7 @@ main(int argc, char *argv[])
&tables,
host, port, username, prompt_password,
concurrentCons,
- progname, echo, quiet,
- &password);
+ progname, echo, quiet);
}
}
else
@@ -309,10 +305,7 @@ main(int argc, char *argv[])
&tables,
host, port, username, prompt_password,
concurrentCons,
- progname, echo, quiet,
- &password);
-
- pg_free(password);
+ progname, echo, quiet);
}
exit(0);
@@ -330,21 +323,15 @@ main(int argc, char *argv[])
* If concurrentCons is > 1, multiple connections are used to vacuum tables
* in parallel. In this case and if the table list is empty, we first obtain
* a list of tables from the database.
- *
- * 'password' is both an input and output parameter. If one is not passed,
- * then whatever is used in a connection is returned so that caller can
- * reuse it in future connections.
*/
static void
vacuum_one_database(const char *dbname, vacuumingOptions *vacopts,
int stage,
SimpleStringList *tables,
const char *host, const char *port,
- const char *username,
- enum trivalue prompt_password,
+ const char *username, enum trivalue prompt_password,
int concurrentCons,
- const char *progname, bool echo, bool quiet,
- char **password)
+ const char *progname, bool echo, bool quiet)
{
PQExpBufferData sql;
PGconn *conn;
@@ -378,15 +365,8 @@ vacuum_one_database(const char *dbname, vacuumingOptions *vacopts,
fflush(stdout);
}
- conn = connectDatabase(dbname, host, port, username, *password,
- prompt_password, progname, false);
-
- /*
- * If no password was not specified by caller and the connection required
- * one, remember it; this suppresses further password prompts.
- */
- if (PQconnectionUsedPassword(conn) && *password == NULL)
- *password = pg_strdup(PQpass(conn));
+ conn = connectDatabase(dbname, host, port, username, prompt_password,
+ progname, false, true);
initPQExpBuffer(&sql);
@@ -444,20 +424,10 @@ vacuum_one_database(const char *dbname, vacuumingOptions *vacopts,
init_slot(slots, conn);
if (parallel)
{
- const char *pqpass;
-
- /*
- * If a password was supplied for the initial connection, use it for
- * subsequent ones too. (Note that since we're connecting to the same
- * database with the same user, there's no need to update the stored
- * password any further.)
- */
- pqpass = PQpass(conn);
-
for (i = 1; i < concurrentCons; i++)
{
- conn = connectDatabase(dbname, host, port, username, pqpass,
- prompt_password, progname, false);
+ conn = connectDatabase(dbname, host, port, username, prompt_password,
+ progname, false, true);
init_slot(slots + i, conn);
}
}
@@ -572,23 +542,12 @@ vacuum_all_databases(vacuumingOptions *vacopts,
PGresult *result;
int stage;
int i;
- char *password = NULL;
conn = connectMaintenanceDatabase(maintenance_db, host, port,
username, prompt_password, progname);
-
result = executeQuery(conn,
"SELECT datname FROM pg_database WHERE datallowconn ORDER BY 1;",
progname, echo);
-
- /*
- * Remember the password for further connections. If no password was
- * required for the maintenance db connection, this gets updated for the
- * first connection that does.
- */
- if (PQconnectionUsedPassword(conn))
- password = pg_strdup(PQpass(conn));
-
PQfinish(conn);
if (analyze_in_stages)
@@ -613,8 +572,7 @@ vacuum_all_databases(vacuumingOptions *vacopts,
NULL,
host, port, username, prompt_password,
concurrentCons,
- progname, echo, quiet,
- &password);
+ progname, echo, quiet);
}
}
}
@@ -630,13 +588,11 @@ vacuum_all_databases(vacuumingOptions *vacopts,
NULL,
host, port, username, prompt_password,
concurrentCons,
- progname, echo, quiet,
- &password);
+ progname, echo, quiet);
}
}
PQclear(result);
- pg_free(password);
}
/*
On Sun, Nov 15, 2015 at 12:39 AM, Michael Paquier
<michael.paquier@gmail.com> wrote:
On Fri, Nov 13, 2015 at 6:58 AM, Alvaro Herrera wrote:
Tom Lane wrote:
Alvaro Herrera <alvherre@2ndquadrant.com> writes:
The place I was thinking we might end up was something like Fujii-san's
patch plus a new bool parameter "allow_password_reuse", which could be
passed as false in cases where the old behavior seems preferable.Hm, we can try that.
That's neater. See for example the attached.
Yes, it is much cleaner than the previous version.
I reviewed and tested the same. It is working for all scenarios.
Regards,
Hari Babu
Fujitsu Australia
--
Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-bugs
On Mon, Nov 16, 2015 at 11:41 AM, Haribabu Kommi
<kommi.haribabu@gmail.com> wrote:
On Sun, Nov 15, 2015 at 12:39 AM, Michael Paquier
<michael.paquier@gmail.com> wrote:On Fri, Nov 13, 2015 at 6:58 AM, Alvaro Herrera wrote:
Tom Lane wrote:
Alvaro Herrera <alvherre@2ndquadrant.com> writes:
The place I was thinking we might end up was something like Fujii-san's
patch plus a new bool parameter "allow_password_reuse", which could be
passed as false in cases where the old behavior seems preferable.Hm, we can try that.
That's neater. See for example the attached.
Yes, it is much cleaner than the previous version.
I reviewed and tested the same. It is working for all scenarios.
This patch has fallen in the void and it simplifies greatly vacuumdb.c
and its password-related routines. I have added it to the next CF to
not forget about it:
https://commitfest.postgresql.org/8/449/
--
Michael
--
Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-bugs
Michael Paquier <michael.paquier@gmail.com> writes:
On Mon, Nov 16, 2015 at 11:41 AM, Haribabu Kommi
<kommi.haribabu@gmail.com> wrote:On Sun, Nov 15, 2015 at 12:39 AM, Michael Paquier
<michael.paquier@gmail.com> wrote:That's neater. See for example the attached.
Yes, it is much cleaner than the previous version.
I reviewed and tested the same. It is working for all scenarios.
This patch has fallen in the void and it simplifies greatly vacuumdb.c
and its password-related routines. I have added it to the next CF to
not forget about it:
Sorry for having lost track of this. I think if we're going to do this,
we should do it now not later, because it essentially reverts the
connectDatabase() API change made in 83dec5a71 in favor of a different API
change with the same purpose. Now, that doesn't matter if no third-party
code is using connectDatabase(), but I have a suspicion that there
probably is some. Any such users will not thank us for whacking
connectDatabase's API around in 9.5 and then whacking it around
differently in 9.6.
In short, I think we should apply this now and back-patch into 9.5,
like the prior patch, even though we're post-RC1. The reduction in
cross-version API churn is worth it.
If no objections, I'll go do that shortly.
regards, tom lane
--
Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-bugs
Tom Lane wrote:
Sorry for having lost track of this. I think if we're going to do this,
we should do it now not later, because it essentially reverts the
connectDatabase() API change made in 83dec5a71 in favor of a different API
change with the same purpose. Now, that doesn't matter if no third-party
code is using connectDatabase(), but I have a suspicion that there
probably is some. Any such users will not thank us for whacking
connectDatabase's API around in 9.5 and then whacking it around
differently in 9.6.In short, I think we should apply this now and back-patch into 9.5,
like the prior patch, even though we're post-RC1. The reduction in
cross-version API churn is worth it.If no objections, I'll go do that shortly.
No objection here.
--
�lvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
--
Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-bugs
On Thu, Dec 24, 2015 at 1:55 AM, Alvaro Herrera
<alvherre@2ndquadrant.com> wrote:
Tom Lane wrote:
Sorry for having lost track of this. I think if we're going to do this,
we should do it now not later, because it essentially reverts the
connectDatabase() API change made in 83dec5a71 in favor of a different API
change with the same purpose. Now, that doesn't matter if no third-party
code is using connectDatabase(), but I have a suspicion that there
probably is some. Any such users will not thank us for whacking
connectDatabase's API around in 9.5 and then whacking it around
differently in 9.6.In short, I think we should apply this now and back-patch into 9.5,
like the prior patch, even though we're post-RC1. The reduction in
cross-version API churn is worth it.If no objections, I'll go do that shortly.
No objection here.
Thanks for taking care of that!
--
Michael
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers