Thread: 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 only
>
> pgbench and oid2name.
>
> http://archives.postgresql.org/message-id/w2g9837222c1004070216u3bc46b3ahbddfdffdbfb46212@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

> 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 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?

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 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?

Why not 'dblink'?

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

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

> 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_CONNmacro...
 

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

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 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

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

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

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

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

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

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.
Theyshould      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. +

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. +

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. +