WIP patch for Todo Item : Provide fallback_application_name in contrib/pgbench, oid2name, and dblink
This patch is to provide support for fallback application name for contrib/pgbench, oid2name, and dblink.
Currently I have done the implementation for pgbench. The implementation is same as in psql.
Before creating a final patch, I wanted to check whether my direction for creating a patch is what is expected from this Todo item.
I have done the basic testing for following 2 scenario's
1. After implementation, if during run of pgbench, we query pg_stat_activity it displays the application name as pgbench
2. It displays the application name in log file also.
Suggestions?
Attachments:
fallback_application_name.patchapplication/octet-stream; name=fallback_application_name.patchDownload
diff --git a/contrib/pgbench/pgbench.c b/contrib/pgbench/pgbench.c
index b0e6991..b6b6a3c 100644
--- a/contrib/pgbench/pgbench.c
+++ b/contrib/pgbench/pgbench.c
@@ -150,6 +150,7 @@ char *pgoptions = NULL;
char *pgtty = NULL;
char *login = NULL;
char *dbName;
+const char *progname;
volatile bool timer_exceeded = false; /* flag from signal handler */
@@ -334,7 +335,7 @@ xstrdup(const char *s)
static void
-usage(const char *progname)
+usage(void)
{
printf("%s is a benchmarking tool for PostgreSQL.\n\n"
"Usage:\n"
@@ -424,10 +425,33 @@ doConnect(void)
*/
do
{
+#define PARAMS_ARRAY_SIZE 7
+
+ const char **keywords = xmalloc(PARAMS_ARRAY_SIZE * sizeof(*keywords));
+ const char **values = xmalloc(PARAMS_ARRAY_SIZE * sizeof(*values));
+
+ keywords[0] = "host";
+ values[0] = pghost;
+ keywords[1] = "port";
+ values[1] = pgport;
+ keywords[2] = "user";
+ values[2] = login;
+ keywords[3] = "password";
+ values[3] = password;
+ keywords[4] = "dbname";
+ values[4] = dbName;
+ keywords[5] = "fallback_application_name";
+ values[5] = progname;
+ keywords[6] = NULL;
+ values[6] = NULL;
+
new_pass = false;
- conn = PQsetdbLogin(pghost, pgport, pgoptions, pgtty, dbName,
- login, password);
+ conn = PQconnectdbParams(keywords, values, true);
+
+ free(keywords);
+ free(values);
+
if (!conn)
{
fprintf(stderr, "Connection to database \"%s\" failed\n",
@@ -1877,15 +1901,13 @@ main(int argc, char **argv)
char val[64];
- const char *progname;
-
progname = get_progname(argv[0]);
if (argc > 1)
{
if (strcmp(argv[1], "--help") == 0 || strcmp(argv[1], "-?") == 0)
{
- usage(progname);
+ usage();
exit(0);
}
if (strcmp(argv[1], "--version") == 0 || strcmp(argv[1], "-V") == 0)
As per the previous discussion in link below, it seems that fallback
application name needs to be provided for only
pgbench and oid2name.
http://archives.postgresql.org/message-id/w2g9837222c1004070216u3bc46b3ahbdd
fdffdbfb46212@mail.gmail.com
However the title of Todo Item suggests it needs to be done for dblink as
well.
Please suggest if it needs to be done for dblink, if yes then what should be
fallback_application_name for it?
From: pgsql-hackers-owner@postgresql.org
[mailto:pgsql-hackers-owner@postgresql.org] On Behalf Of Amit kapila
Sent: Friday, June 08, 2012 10:45 PM
To: pgsql-hackers@postgresql.org
Subject: [HACKERS] WIP patch for Todo Item : Provide
fallback_application_name in contrib/pgbench, oid2name, and dblink
This patch is to provide support for fallback application name for
contrib/pgbench, oid2name, and dblink.
Currently I have done the implementation for pgbench. The implementation is
same as in psql.
Before creating a final patch, I wanted to check whether my direction for
creating a patch is what is expected from this Todo item.
I have done the basic testing for following 2 scenario's
1. After implementation, if during run of pgbench, we query pg_stat_activity
it displays the application name as pgbench
2. It displays the application name in log file also.
Suggestions?
On Mon, Jun 11, 2012 at 5:30 AM, Amit Kapila <amit.kapila@huawei.com> wrote:
As per the previous discussion in link below, it seems that fallback
application name needs to be provided for onlypgbench and oid2name.
However the title of Todo Item suggests it needs to be done for dblink as
well.Please suggest if it needs to be done for dblink, if yes then what should be
fallback_application_name for it?
Why not 'dblink'?
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
Import Notes
Reply to msg id not found: 4fd5bb30.1581650a.01d2.4d8bSMTPIN_ADDED@mx.google.com
Why not 'dblink'?
We can do for dblink as well. I just wanted to check before implementing in
dblink.
I have checked the dblink_connect() function, it receives the connect string
and used in most cases that string to
call libpq connect which is different from pgbench and oid2name where
connection parameters are formed in main function and then call libpq
connect.
To achieve the same in dblink, we need to parse the passed connection string
and check if it contains fallback_application_name, if yes then its okay,
otherwise we need to append fallback_application_name in connection string.
The doubt I have is that what name to use as fallback_application_name
because here we cannot have argv as in pgbench or oid2name?
The 2 options which I can think of are:
1. Hard-coded name - dblink
2. postgres - which I think will be caller of dblink functionality
Please suggest if any of this option is okay or what is other way to get the
program name.
-----Original Message-----
From: Robert Haas [mailto:robertmhaas@gmail.com]
Sent: Wednesday, June 13, 2012 12:13 AM
To: Amit Kapila
Cc: pgsql-hackers@postgresql.org
Subject: Re: [HACKERS] WIP patch for Todo Item : Provide
fallback_application_name in contrib/pgbench, oid2name, and dblink
On Mon, Jun 11, 2012 at 5:30 AM, Amit Kapila <amit.kapila@huawei.com> wrote:
As per the previous discussion in link below, it seems that fallback
application name needs to be provided for onlypgbench and oid2name.
http://archives.postgresql.org/message-id/w2g9837222c1004070216u3bc46b3ahbdd
fdffdbfb46212@mail.gmail.com
However the title of Todo Item suggests it needs to be done for dblink as
well.Please suggest if it needs to be done for dblink, if yes then what should
be
fallback_application_name for it?
Why not 'dblink'?
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
I have created the patch by including fallback_application_name for dblink
as well.
In this I have used the name of fallback_application_name as dblink.
Please let me know your suggestions regarding the same.
-----Original Message-----
From: Robert Haas [mailto:robertmhaas@gmail.com]
Sent: Wednesday, June 13, 2012 12:13 AM
To: Amit Kapila
Cc: pgsql-hackers@postgresql.org
Subject: Re: [HACKERS] WIP patch for Todo Item : Provide
fallback_application_name in contrib/pgbench, oid2name, and dblink
On Mon, Jun 11, 2012 at 5:30 AM, Amit Kapila <amit.kapila@huawei.com> wrote:
As per the previous discussion in link below, it seems that fallback
application name needs to be provided for onlypgbench and oid2name.
http://archives.postgresql.org/message-id/w2g9837222c1004070216u3bc46b3ahbdd
fdffdbfb46212@mail.gmail.com
However the title of Todo Item suggests it needs to be done for dblink as
well.Please suggest if it needs to be done for dblink, if yes then what should
be
fallback_application_name for it?
Why not 'dblink'?
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
Attachments:
fallback_application_name.patchapplication/octet-stream; name=fallback_application_name.patchDownload
diff --git a/contrib/dblink/dblink.c b/contrib/dblink/dblink.c
index 71acb35..414ae20 100644
--- a/contrib/dblink/dblink.c
+++ b/contrib/dblink/dblink.c
@@ -100,6 +100,7 @@ static HeapTuple get_tuple_of_interest(Relation rel, int *pkattnums, int pknumat
static Relation get_rel_from_relname(text *relname_text, LOCKMODE lockmode, AclMode aclmode);
static char *generate_relation_name(Relation rel);
static void dblink_connstr_check(const char *connstr);
+static char *dblink_connstr_append(const char *connstr);
static void dblink_security_check(PGconn *conn, remoteConn *rconn);
static void dblink_res_error(const char *conname, PGresult *res, const char *dblink_context_msg, bool fail);
static char *get_connect_string(const char *servername);
@@ -255,6 +256,12 @@ dblink_connect(PG_FUNCTION_ARGS)
/* check password in connection string if not superuser */
dblink_connstr_check(connstr);
+
+ /* check if fallback_application_name doesn't exist then append
+ * fallback_application_name='dblink'
+ */
+ connstr = dblink_connstr_append(connstr);
+
conn = PQconnectdb(connstr);
if (PQstatus(conn) == CONNECTION_BAD)
@@ -2524,6 +2531,53 @@ dblink_connstr_check(const char *connstr)
}
}
+
+/*
+ * To append fallback_application_name in connection string if it
+ * doesn't exist.
+ * This is to ensure that incase connection string
+ * doesn't contain application_name, fallback_application_name 'dblink'
+ * can be used to display as application_name at appropriate places.
+ */
+static char *
+dblink_connstr_append(const char *connstr)
+{
+ PQconninfoOption *options;
+ PQconninfoOption *option;
+ bool connstr_has_fallback_appname = false;
+
+ options = PQconninfoParse(connstr, NULL);
+ if (options)
+ {
+ for (option = options; option->keyword != NULL; option++)
+ {
+ if (strcmp(option->keyword, "fallback_application_name") == 0)
+ {
+ if (option->val != NULL && option->val[0] != '\0')
+ {
+ connstr_has_fallback_appname = true;
+ break;
+ }
+ }
+ }
+ PQconninfoFree(options);
+ }
+
+ if (!connstr_has_fallback_appname)
+ {
+ StringInfo buf = makeStringInfo();
+
+ appendStringInfo(buf, "%s ", connstr);
+ appendStringInfo(buf, "%s='%s' ", "fallback_application_name", "dblink");
+
+ return buf->data;
+ }
+
+ return connstr;
+}
+
+
+
static void
dblink_res_error(const char *conname, PGresult *res, const char *dblink_context_msg, bool fail)
{
diff --git a/contrib/oid2name/oid2name.c b/contrib/oid2name/oid2name.c
index c7ba1bd..4b3db40 100644
--- a/contrib/oid2name/oid2name.c
+++ b/contrib/oid2name/oid2name.c
@@ -44,6 +44,7 @@ struct options
char *hostname;
char *port;
char *username;
+ char *progname;
};
/* function prototypes */
@@ -80,6 +81,7 @@ get_opts(int argc, char **argv, struct options * my_opts)
my_opts->hostname = NULL;
my_opts->port = NULL;
my_opts->username = NULL;
+ my_opts->progname = progname;
if (argc > 1)
{
@@ -308,14 +310,32 @@ sql_conn(struct options * my_opts)
*/
do
{
+#define PARAMS_ARRAY_SIZE 7
+
+ const char **keywords = myalloc(PARAMS_ARRAY_SIZE * sizeof(*keywords));
+ const char **values = myalloc(PARAMS_ARRAY_SIZE * sizeof(*values));
+
+ keywords[0] = "host";
+ values[0] = my_opts->hostname;
+ keywords[1] = "port";
+ values[1] = my_opts->port;
+ keywords[2] = "user";
+ values[2] = my_opts->username;
+ keywords[3] = "password";
+ values[3] = password;
+ keywords[4] = "dbname";
+ values[4] = my_opts->dbname;
+ keywords[5] = "fallback_application_name";
+ values[5] = my_opts->progname;
+ keywords[6] = NULL;
+ values[6] = NULL;
+
new_pass = false;
- conn = PQsetdbLogin(my_opts->hostname,
- my_opts->port,
- NULL, /* options */
- NULL, /* tty */
- my_opts->dbname,
- my_opts->username,
- password);
+ conn = PQconnectdbParams(keywords, values, true);
+
+ free(keywords);
+ free(values);
+
if (!conn)
{
fprintf(stderr, "%s: could not connect to database %s\n",
diff --git a/contrib/pgbench/pgbench.c b/contrib/pgbench/pgbench.c
index b0e6991..cfdf0f6 100644
--- a/contrib/pgbench/pgbench.c
+++ b/contrib/pgbench/pgbench.c
@@ -146,10 +146,9 @@ int main_pid; /* main process id used in log filename */
char *pghost = "";
char *pgport = "";
-char *pgoptions = NULL;
-char *pgtty = NULL;
char *login = NULL;
char *dbName;
+const char *progname;
volatile bool timer_exceeded = false; /* flag from signal handler */
@@ -334,7 +333,7 @@ xstrdup(const char *s)
static void
-usage(const char *progname)
+usage(void)
{
printf("%s is a benchmarking tool for PostgreSQL.\n\n"
"Usage:\n"
@@ -424,10 +423,33 @@ doConnect(void)
*/
do
{
+#define PARAMS_ARRAY_SIZE 7
+
+ const char **keywords = xmalloc(PARAMS_ARRAY_SIZE * sizeof(*keywords));
+ const char **values = xmalloc(PARAMS_ARRAY_SIZE * sizeof(*values));
+
+ keywords[0] = "host";
+ values[0] = pghost;
+ keywords[1] = "port";
+ values[1] = pgport;
+ keywords[2] = "user";
+ values[2] = login;
+ keywords[3] = "password";
+ values[3] = password;
+ keywords[4] = "dbname";
+ values[4] = dbName;
+ keywords[5] = "fallback_application_name";
+ values[5] = progname;
+ keywords[6] = NULL;
+ values[6] = NULL;
+
new_pass = false;
- conn = PQsetdbLogin(pghost, pgport, pgoptions, pgtty, dbName,
- login, password);
+ conn = PQconnectdbParams(keywords, values, true);
+
+ free(keywords);
+ free(values);
+
if (!conn)
{
fprintf(stderr, "Connection to database \"%s\" failed\n",
@@ -1877,15 +1899,13 @@ main(int argc, char **argv)
char val[64];
- const char *progname;
-
progname = get_progname(argv[0]);
if (argc > 1)
{
if (strcmp(argv[1], "--help") == 0 || strcmp(argv[1], "-?") == 0)
{
- usage(progname);
+ usage();
exit(0);
}
if (strcmp(argv[1], "--version") == 0 || strcmp(argv[1], "-V") == 0)
On Wed, Jun 13, 2012 at 12:07 AM, Amit Kapila <amit.kapila@huawei.com> wrote:
Why not 'dblink'?
We can do for dblink as well. I just wanted to check before implementing in
dblink.I have checked the dblink_connect() function, it receives the connect string
and used in most cases that string to
call libpq connect which is different from pgbench and oid2name where
connection parameters are formed in main function and then call libpq
connect.To achieve the same in dblink, we need to parse the passed connection string
and check if it contains fallback_application_name, if yes then its okay,
otherwise we need to append fallback_application_name in connection string.
That seems undesirable. I don't think this is important enough to be
worth reparsing the connection string for. I'd just forget about
doing it for dblink if there's no cheaper way.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
Import Notes
Reply to msg id not found: 4fd81224.8584440a.3cb7.ffffad10SMTPIN_ADDED@mx.google.com
That seems undesirable. I don't think this is important enough to be
worth reparsing > the connection string for.
The connect string should not be long, so parsing is not a big cost
performance wise.
I have currently modified the code for dblink in the patch I have uploaded
in CF.
However as per your suggestion, I will remove it during handling of other
Review comments for patch unless somebody asks to keep it.
-----Original Message-----
From: Robert Haas [mailto:robertmhaas@gmail.com]
Sent: Thursday, June 14, 2012 7:25 PM
To: Amit Kapila
Cc: pgsql-hackers@postgresql.org
Subject: Re: [HACKERS] WIP patch for Todo Item : Provide
fallback_application_name in contrib/pgbench, oid2name, and dblink
On Wed, Jun 13, 2012 at 12:07 AM, Amit Kapila <amit.kapila@huawei.com>
wrote:
Why not 'dblink'?
We can do for dblink as well. I just wanted to check before implementing
in
dblink.
I have checked the dblink_connect() function, it receives the connect
string
and used in most cases that string to
call libpq connect which is different from pgbench and oid2name where
connection parameters are formed in main function and then call libpq
connect.To achieve the same in dblink, we need to parse the passed connection
string
and check if it contains fallback_application_name, if yes then its okay,
otherwise we need to append fallback_application_name in connection
string.
That seems undesirable. I don't think this is important enough to be
worth reparsing the connection string for. I'd just forget about
doing it for dblink if there's no cheaper way.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
On Thu, Jun 14, 2012 at 10:55 PM, Robert Haas <robertmhaas@gmail.com> wrote:
On Wed, Jun 13, 2012 at 12:07 AM, Amit Kapila <amit.kapila@huawei.com> wrote:
To achieve the same in dblink, we need to parse the passed connection string
and check if it contains fallback_application_name, if yes then its okay,
otherwise we need to append fallback_application_name in connection string.That seems undesirable. I don't think this is important enough to be
worth reparsing the connection string for. I'd just forget about
doing it for dblink if there's no cheaper way.
Indeed reparsing connection string is not cheap, but dblink does it for
checking password requirement for non-in dblink_connstr_check when the
local user was not a superuser. So Amit's idea doesn't seem
unreasonable to me, if we can avoid extra PQconninfoParse call.
Just an idea, but how about pushing fallback_application_name handling
into dblink_connstr_check? We reparse connection string unless local
user was a superuser, so it would not be serious overhead in most cases.
Although it might require changes in DBLINK_GET_CONN macro...
Regards,
--
Shigeru Hanada
On Wed, Jun 27, 2012 at 9:57 PM, Shigeru HANADA
<shigeru.hanada@gmail.com> wrote:
On Thu, Jun 14, 2012 at 10:55 PM, Robert Haas <robertmhaas@gmail.com> wrote:
On Wed, Jun 13, 2012 at 12:07 AM, Amit Kapila <amit.kapila@huawei.com> wrote:
To achieve the same in dblink, we need to parse the passed connection string
and check if it contains fallback_application_name, if yes then its okay,
otherwise we need to append fallback_application_name in connection string.That seems undesirable. I don't think this is important enough to be
worth reparsing the connection string for. I'd just forget about
doing it for dblink if there's no cheaper way.Indeed reparsing connection string is not cheap, but dblink does it for
checking password requirement for non-in dblink_connstr_check when the
local user was not a superuser. So Amit's idea doesn't seem
unreasonable to me, if we can avoid extra PQconninfoParse call.Just an idea, but how about pushing fallback_application_name handling
into dblink_connstr_check? We reparse connection string unless local
user was a superuser, so it would not be serious overhead in most cases.
Although it might require changes in DBLINK_GET_CONN macro...
*shrug*
If it can be done without costing anything meaningful, I don't object,
but I would humbly suggest that this is not hugely important one way
or the other. application_name is primarily a monitoring convenience,
so it's not hugely important to have it set anyway, and
fallback_application_name is only going to apply in cases where the
user doesn't care enough to bother setting application_name. Let's
not knock ourselves out to solve a problem that may not be that
important to begin with.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
From: Robert Haas [mailto:robertmhaas@gmail.com]
Sent: Thursday, June 28, 2012 7:46 AM
On Wed, Jun 27, 2012 at 9:57 PM, Shigeru HANADA
<shigeru.hanada@gmail.com> wrote:
On Thu, Jun 14, 2012 at 10:55 PM, Robert Haas <robertmhaas@gmail.com>
wrote:
Indeed reparsing connection string is not cheap, but dblink does it for
checking password requirement for non-in dblink_connstr_check when the
local user was not a superuser. So Amit's idea doesn't seem
unreasonable to me, if we can avoid extra PQconninfoParse call.Just an idea, but how about pushing fallback_application_name handling
into dblink_connstr_check? We reparse connection string unless local
user was a superuser, so it would not be serious overhead in most cases.
Although it might require changes in DBLINK_GET_CONN macro...
If it can be done without costing anything meaningful, I don't object,
but I would humbly suggest that this is not hugely important one way
or the other. application_name is primarily a monitoring convenience,
so it's not hugely important to have it set anyway,
In some cases like when DBA does the monitoring in night or sometimes when
he doesn't expect much load on database to do some maintenance tasks,
it can be helpful for him to know if there is any application which he
doesn't expect
to be there. This can be one of the ways he can know the which applications
are currently active.
Users are not bothered to set application_name in general cases as it
doesn't server any
big purpose for them.
I understand this is good to have feature, but if it doesn't cost much then
according to me
it can be done.
With Regards,
Amit Kapila.
On fre, 2012-06-08 at 17:14 +0000, Amit kapila wrote:
This patch is to provide support for fallback application name for
contrib/pgbench, oid2name, and dblink.
vacuumlo should also be treated, I think.
(2012/06/28 11:16), Robert Haas wrote:
If it can be done without costing anything meaningful, I don't object,
but I would humbly suggest that this is not hugely important one way
or the other. application_name is primarily a monitoring convenience,
so it's not hugely important to have it set anyway, and
fallback_application_name is only going to apply in cases where the
user doesn't care enough to bother setting application_name. Let's
not knock ourselves out to solve a problem that may not be that
important to begin with.
Thanks for clarification. I got the point.
The way fixing oid2name and pgbench seems reasonable, so applying it to
vacuumlo (as Peter mentioned) would be enough for this issue.
Regards,
--
Shigeru HANADA
Hi Shigeru/Robert,
-----Original Message-----
From: Shigeru HANADA [mailto:shigeru.hanada@gmail.com]
Sent: Wednesday, July 04, 2012 6:57 AM
(2012/06/28 11:16), Robert Haas wrote:
If it can be done without costing anything meaningful, I don't object,
but I would humbly suggest that this is not hugely important one way
or the other. application_name is primarily a monitoring convenience,
so it's not hugely important to have it set anyway, and
fallback_application_name is only going to apply in cases where the
user doesn't care enough to bother setting application_name. Let's
not knock ourselves out to solve a problem that may not be that
important to begin with.
Thanks for clarification. I got the point.
The way fixing oid2name and pgbench seems reasonable, so applying it to
vacuumlo (as Peter mentioned) would be enough for this issue.
Shall I consider following 2 points to update the patch:
1. Apply changes similar to pgbench and oid2name for vacuumlo
2. Remove the modifications for dblink.
With Regards,
Amit Kapila.
On Tue, Jul 3, 2012 at 11:36 PM, Amit Kapila <amit.kapila@huawei.com> wrote:
Hi Shigeru/Robert,
The way fixing oid2name and pgbench seems reasonable, so applying it to
vacuumlo (as Peter mentioned) would be enough for this issue.Shall I consider following 2 points to update the patch:
1. Apply changes similar to pgbench and oid2name for vacuumlo
2. Remove the modifications for dblink.
I've done these two things and committed this. Along the way, I also
fixed it to use a stack-allocated array instead of using malloc, since
there's no need to malloc a fixed-size array with 7 elements.
Thanks for the patch.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
Import Notes
Reply to msg id not found: 4ff3bb07.e7f2440a.13c4.ffffae3bSMTPIN_ADDED@mx.google.com
On Wed, Jul 4, 2012 at 12:41 PM, Robert Haas <robertmhaas@gmail.com> wrote:
On Tue, Jul 3, 2012 at 11:36 PM, Amit Kapila <amit.kapila@huawei.com>
wrote:Hi Shigeru/Robert,
The way fixing oid2name and pgbench seems reasonable, so applying it to
vacuumlo (as Peter mentioned) would be enough for this issue.Shall I consider following 2 points to update the patch:
1. Apply changes similar to pgbench and oid2name for vacuumlo
2. Remove the modifications for dblink.I've done these two things and committed this. Along the way, I also
fixed it to use a stack-allocated array instead of using malloc, since
there's no need to malloc a fixed-size array with 7 elements.Thanks for the patch.
Since this commit (17676c785a95b2598c573), pgbench no longer uses .pgpass
to obtain passwords, but instead prompts for a password
This problem is in 9.3 and 9.4dev
According to strace, it is reading the .pgpass file, it just seem like it
is not using it.
Cheers,
Jeff
On Sun, Feb 9, 2014 at 6:33 PM, Jeff Janes <jeff.janes@gmail.com> wrote:
On Wed, Jul 4, 2012 at 12:41 PM, Robert Haas <robertmhaas@gmail.com> wrote:
On Tue, Jul 3, 2012 at 11:36 PM, Amit Kapila <amit.kapila@huawei.com>
wrote:Hi Shigeru/Robert,
The way fixing oid2name and pgbench seems reasonable, so applying it to
vacuumlo (as Peter mentioned) would be enough for this issue.Shall I consider following 2 points to update the patch:
1. Apply changes similar to pgbench and oid2name for vacuumlo
2. Remove the modifications for dblink.I've done these two things and committed this. Along the way, I also
fixed it to use a stack-allocated array instead of using malloc, since
there's no need to malloc a fixed-size array with 7 elements.Thanks for the patch.
Since this commit (17676c785a95b2598c573), pgbench no longer uses .pgpass to
obtain passwords, but instead prompts for a passwordThis problem is in 9.3 and 9.4dev
According to strace, it is reading the .pgpass file, it just seem like it is
not using it.
Hmm. I tried pgbench with the following .pgpass file and it worked
OK. Removing the file caused pgbench to prompt for a password.
*:*:*:*:foo
Presumably whatever behavior difference you are seeing is somehow
related to the use of PQconnectdbParams() rather than PQsetdbLogin(),
but the fine manual does not appear to document a different between
those functions as regards password-prompting behavior or .pgpass
usage. My guess is that it's getting as far as PasswordFromFile() and
then judging whatever entry you have in the file not to match the
connection attempt. If you're correct about this commit being to
blame, then my guess is that one of hostname, port, dbname, and
username must end up initialized differently when PQconnectdbParams()
rather than PQsetdbLogin() is used...
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Sun, Feb 9, 2014 at 4:56 PM, Robert Haas <robertmhaas@gmail.com> wrote:
On Sun, Feb 9, 2014 at 6:33 PM, Jeff Janes <jeff.janes@gmail.com> wrote:
Since this commit (17676c785a95b2598c573), pgbench no longer uses
.pgpass to
obtain passwords, but instead prompts for a password
This problem is in 9.3 and 9.4dev
According to strace, it is reading the .pgpass file, it just seem like
it is
not using it.
Hmm. I tried pgbench with the following .pgpass file and it worked
OK. Removing the file caused pgbench to prompt for a password.*:*:*:*:foo
OK, that works for me. I had it completely specified. Playing with
variations on this, I see that the key is pgport. Set to * it works, set
to 5432 it prompts for the password. (If I specify -p 5432 to pgbench,
that would work with the original file)
Presumably whatever behavior difference you are seeing is somehow
related to the use of PQconnectdbParams() rather than PQsetdbLogin(),
but the fine manual does not appear to document a different between
those functions as regards password-prompting behavior or .pgpass
usage.
It looks like PQsetdbLogin() has either NULL or empty string passed to it
match 5432 in pgpass, while PQconnectdbParams() only has NULL match 5432
and empty string does not. pgbench uses empty string if no -p is specified.
This make pgbench behave the way I think is correct, but it hardly seems
like the right way to fix it.
*** a/contrib/pgbench/pgbench.c
--- b/contrib/pgbench/pgbench.c
*************** doConnect(void)
*** 528,533 ****
--- 528,535 ----new_pass = false;
+ if (values[1][0] == 0) values[1]=NULL;
+
conn = PQconnectdbParams(keywords, values, true);if (!conn)
Cheers,
Jeff
On Mon, Feb 10, 2014 at 12:14 PM, Jeff Janes <jeff.janes@gmail.com> wrote:
Presumably whatever behavior difference you are seeing is somehow
related to the use of PQconnectdbParams() rather than PQsetdbLogin(),
but the fine manual does not appear to document a different between
those functions as regards password-prompting behavior or .pgpass
usage.It looks like PQsetdbLogin() has either NULL or empty string passed to it
match 5432 in pgpass, while PQconnectdbParams() only has NULL match 5432 and
empty string does not. pgbench uses empty string if no -p is specified.This make pgbench behave the way I think is correct, but it hardly seems
like the right way to fix it.[ kludge ]
Well, it seems to me that the threshold issue here is whether or not
we should try to change the behavior of libpq. If not, then your
kludge might be the best option. But if so then it isn't necessary.
However, I don't feel confident to pass judgement on the what the
libpq semantics should be.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Hi,
Am Wed, 12 Feb 2014 13:47:41 -0500
schrieb Robert Haas <robertmhaas@gmail.com>:
On Mon, Feb 10, 2014 at 12:14 PM, Jeff Janes <jeff.janes@gmail.com>
wrote:Presumably whatever behavior difference you are seeing is somehow
related to the use of PQconnectdbParams() rather than
PQsetdbLogin(), but the fine manual does not appear to document a
different between those functions as regards password-prompting
behavior or .pgpass usage.It looks like PQsetdbLogin() has either NULL or empty string passed
to it match 5432 in pgpass, while PQconnectdbParams() only has NULL
match 5432 and empty string does not. pgbench uses empty string if
no -p is specified.This make pgbench behave the way I think is correct, but it hardly
seems like the right way to fix it.[ kludge ]i
Well, it seems to me that the threshold issue here is whether or not
we should try to change the behavior of libpq. If not, then your
kludge might be the best option. But if so then it isn't necessary.
However, I don't feel confident to pass judgement on the what the
libpq semantics should be.
I noticed that pgbnech doesn't use all variables from my PGSERVICE
definition. Then I came around and find this Thread.
export PGSERVICE=test_db_head
cat ~/.pg_service.conf
[test_db_head]
host=/tmp
user=avo
port=5496
dbname=pgbench
/usr/local/postgresql/head/bin/pgbench -s 1 -i
Connection to database "" failed:
could not connect to server: No such file or directory
Is the server running locally and accepting
connections on Unix domain socket "/tmp/.s.PGSQL.5432"?
As you noticed before pgbench initialises some of its connection
parameters with empty string, this overwrites variables defined
in .pg_service.conf.
I patched the function conninfo_array_parse() which is used by
PQconnectStartParams to behave like PQsetdbLogin. The patch also
contains a document patch which clarify "unspecified" parameters.
Now, PQconnectStartParams will handle empty strings in exactly the same
way as it handles NULL and pgbench run as expected:
usr/local/postgresql/head/bin/pgbench -s 1 -i
NOTICE: table "pgbench_history" does not exist, skipping
NOTICE: table "pgbench_tellers" does not exist, skipping
NOTICE: table "pgbench_accounts" does not exist, skipping
NOTICE: table "pgbench_branches" does not exist, skipping
creating tables...
100000 of 100000 tuples (100%) done (elapsed 0.21 s, remaining 0.00
s). vacuum...
set primary keys...
done.
Kind Regards
- Adrian
Attachments:
libpq_conninfo_array_parse_empty_string_v1.patchtext/x-patchDownload
diff --git a/src/interfaces/libpq/fe-connect.c b/src/interfaces/libpq/fe-connect.c
index d53c41f..253615e 100644
--- a/src/interfaces/libpq/fe-connect.c
+++ b/src/interfaces/libpq/fe-connect.c
@@ -4321,7 +4321,7 @@ conninfo_array_parse(const char *const * keywords, const char *const * values,
const char *pname = keywords[i];
const char *pvalue = values[i];
- if (pvalue != NULL)
+ if (pvalue != NULL && pvalue[0] != '\0')
{
/* Search for the param record */
for (option = options; option->keyword != NULL; option++)
Adrian Vondendriesch <adrian.vondendriesch@credativ.de> writes:
I patched the function conninfo_array_parse() which is used by
PQconnectStartParams to behave like PQsetdbLogin. The patch also
contains a document patch which clarify "unspecified" parameters.
I see no documentation update here. I'm also fairly concerned about the
implication that no connection parameter, now or in future, can ever have
an empty string as the correct value.
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Am 01.04.2014 16:32, schrieb Tom Lane:
Adrian Vondendriesch <adrian.vondendriesch@credativ.de> writes:
I patched the function conninfo_array_parse() which is used by
PQconnectStartParams to behave like PQsetdbLogin. The patch also
contains a document patch which clarify "unspecified" parameters.I see no documentation update here. I'm also fairly concerned about the
implication that no connection parameter, now or in future, can ever have
an empty string as the correct value.
If we want to preserve the possibility to except an empty string as
correct value, then pgbench should initialise some variables with
NULL instead of empty string.
Moreover it should be documented that "unspecified" means NULL and not
empty string, like in PQsetdbLogin.
However, attached you will find the whole patch, including documentation.
Kind Regards
- Adrian
Attachments:
libpq_conninfo_array_parse_empty_string_v1.patchtext/x-patch; name=libpq_conninfo_array_parse_empty_string_v1.patchDownload
diff --git a/doc/src/sgml/libpq.sgml b/doc/src/sgml/libpq.sgml
index be0d602..0bac166 100644
--- a/doc/src/sgml/libpq.sgml
+++ b/doc/src/sgml/libpq.sgml
@@ -136,10 +136,11 @@ PGconn *PQconnectdbParams(const char * const *keywords,
</para>
<para>
- If any parameter is unspecified, then the corresponding
- environment variable (see <xref linkend="libpq-envars">)
- is checked. If the environment variable is not set either,
- then the indicated built-in defaults are used.
+ If any parameter is unspecified, then the corresponding environment
+ variable (see <xref linkend="libpq-envars">) is checked. Parameters are
+ treated as unspecified if they are either NULL or contain an empty string
+ (""). If the environment variable is not set either, then the
+ indicated built-in defaults are used.
</para>
<para>
diff --git a/src/interfaces/libpq/fe-connect.c b/src/interfaces/libpq/fe-connect.c
index d53c41f..253615e 100644
--- a/src/interfaces/libpq/fe-connect.c
+++ b/src/interfaces/libpq/fe-connect.c
@@ -4321,7 +4321,7 @@ conninfo_array_parse(const char *const * keywords, const char *const * values,
const char *pname = keywords[i];
const char *pvalue = values[i];
- if (pvalue != NULL)
+ if (pvalue != NULL && pvalue[0] != '\0')
{
/* Search for the param record */
for (option = options; option->keyword != NULL; option++)
On Tue, Apr 1, 2014 at 05:06:08PM +0200, Adrian Vondendriesch wrote:
Am 01.04.2014 16:32, schrieb Tom Lane:
Adrian Vondendriesch <adrian.vondendriesch@credativ.de> writes:
I patched the function conninfo_array_parse() which is used by
PQconnectStartParams to behave like PQsetdbLogin. The patch also
contains a document patch which clarify "unspecified" parameters.I see no documentation update here. I'm also fairly concerned about the
implication that no connection parameter, now or in future, can ever have
an empty string as the correct value.If we want to preserve the possibility to except an empty string as
correct value, then pgbench should initialise some variables with
NULL instead of empty string.Moreover it should be documented that "unspecified" means NULL and not
empty string, like in PQsetdbLogin.However, attached you will find the whole patch, including documentation.
Where do we want to go with this? Right now, PQsetdbLogin() and
PQconnectdbParams() handle zero-length strings differently.
PQsetdbLogin() treats it as unspecified, while PQconnectdbParams() does
not, and our documentation on PQconnectdbParams() is unclear on this.
It seems we can either change pgbench or libpq, and we should document
libpq in either case.
Also, the second sentence seems wrong:
The passed arrays can be empty to use all default parameters,
or can contain one or more parameter settings. They should
be matched in length. Processing will stop with the last
non-<symbol>NULL</symbol> element of the <literal>keywords</literal>
array.
Doesn't it stop with first NULL value? For example, if the array is
"a", NULL, "b", NULL, it stops at the first NULL, not the second one.
--
Bruce Momjian <bruce@momjian.us> http://momjian.us
EnterpriseDB http://enterprisedb.com
+ Everyone has their own god. +
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Tue, Apr 1, 2014 at 10:32:01AM -0400, Tom Lane wrote:
Adrian Vondendriesch <adrian.vondendriesch@credativ.de> writes:
I patched the function conninfo_array_parse() which is used by
PQconnectStartParams to behave like PQsetdbLogin. The patch also
contains a document patch which clarify "unspecified" parameters.I see no documentation update here. I'm also fairly concerned about the
implication that no connection parameter, now or in future, can ever have
an empty string as the correct value.
I thought about this. We have never needed PQsetdbLogin() to handle
zero-length strings specially in all the years we used it, so I am not
sure why we would ever need PQconnectdbParams() to handle it. I am
thinking we should make PQconnectdbParams() handle zero-length strings
the same as NULL, and document it.
Attached is a slightly-modified version of Adrian Vondendriesch's patch.
--
Bruce Momjian <bruce@momjian.us> http://momjian.us
EnterpriseDB http://enterprisedb.com
+ Everyone has their own god. +
Attachments:
libpq.difftext/x-diff; charset=us-asciiDownload
diff --git a/doc/src/sgml/libpq.sgml b/doc/src/sgml/libpq.sgml
new file mode 100644
index be0d602..1f0975a
*** a/doc/src/sgml/libpq.sgml
--- b/doc/src/sgml/libpq.sgml
*************** PGconn *PQconnectdbParams(const char * c
*** 131,145 ****
<para>
The passed arrays can be empty to use all default parameters, or can
contain one or more parameter settings. They should be matched in length.
! Processing will stop with the last non-<symbol>NULL</symbol> element
! of the <literal>keywords</literal> array.
</para>
<para>
! If any parameter is unspecified, then the corresponding
! environment variable (see <xref linkend="libpq-envars">)
! is checked. If the environment variable is not set either,
! then the indicated built-in defaults are used.
</para>
<para>
--- 131,145 ----
<para>
The passed arrays can be empty to use all default parameters, or can
contain one or more parameter settings. They should be matched in length.
! Processing will stop at the first <symbol>NULL</symbol> element
! in the <literal>keywords</literal> array.
</para>
<para>
! If any parameter is NULL or an emptry string, the corresponding
! environment variable (see <xref linkend="libpq-envars">) is checked.
! If the environment variable is not set either, then the indicated
! built-in defaults are used.
</para>
<para>
diff --git a/src/interfaces/libpq/fe-connect.c b/src/interfaces/libpq/fe-connect.c
new file mode 100644
index 10cc0e6..45df6ce
*** a/src/interfaces/libpq/fe-connect.c
--- b/src/interfaces/libpq/fe-connect.c
*************** conninfo_array_parse(const char *const *
*** 4352,4358 ****
const char *pname = keywords[i];
const char *pvalue = values[i];
! if (pvalue != NULL)
{
/* Search for the param record */
for (option = options; option->keyword != NULL; option++)
--- 4352,4358 ----
const char *pname = keywords[i];
const char *pvalue = values[i];
! if (pvalue != NULL && pvalue[0] != '\0')
{
/* Search for the param record */
for (option = options; option->keyword != NULL; option++)
On Wed, Apr 16, 2014 at 11:01:56PM -0400, Bruce Momjian wrote:
On Tue, Apr 1, 2014 at 10:32:01AM -0400, Tom Lane wrote:
Adrian Vondendriesch <adrian.vondendriesch@credativ.de> writes:
I patched the function conninfo_array_parse() which is used by
PQconnectStartParams to behave like PQsetdbLogin. The patch also
contains a document patch which clarify "unspecified" parameters.I see no documentation update here. I'm also fairly concerned about the
implication that no connection parameter, now or in future, can ever have
an empty string as the correct value.I thought about this. We have never needed PQsetdbLogin() to handle
zero-length strings specially in all the years we used it, so I am not
sure why we would ever need PQconnectdbParams() to handle it. I am
thinking we should make PQconnectdbParams() handle zero-length strings
the same as NULL, and document it.Attached is a slightly-modified version of Adrian Vondendriesch's patch.
Patch applied.
--
Bruce Momjian <bruce@momjian.us> http://momjian.us
EnterpriseDB http://enterprisedb.com
+ Everyone has their own god. +
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers