Horton

Thread

Commits

  1. pgbench: Simplify some port, host, user and dbname assignments

  1. [PATCH] pgbench: Bug fix for the -d option

    miyake_kouta <miyake_kouta@oss.nttdata.com> — 2021-02-26T04:18:20Z

    Hi.

I found a bug in pgbench's -d option and created a patch.

The bug is the following:
pgbench's option -d can display debug log about connection, which is 
like "pghost: foo pgport: 5432 nclients: 1 nxacts: 10 dbName: bar".
This configuration is supplied by other options or environment variables 
like PGUSER or PGPORT.
When there is no PGDATABASE, pgbench will use PGUSER as the dbName.
However, when there is PGPORT environment variable, this debug logger 
doesn't refer PGUSER even if there is PGUSER.
In other words, even if you are setting both PGPORT and PGUSER, 
pgbench's option -d will display like this: "pghost: foo pgport: 5432 
nclients: 1 nxact: 10 dbName: ".
I think this is a bug that dbName is displayed as if it's not specified.
Note that this bug is only related to this debug logger. The main unit 
of pgbench can establish a connection by complementing dbName with 
PGUSER despite this bug.

So I made a patch (only one line changed).
As shown in this patch file, I just changed the else if statement to an 
if statement.
I'm suggesting this bug fix because I think it's a bug, but if there's 
any other intent to this else if statement, could you let me know?

Regards
---
Kota Miyake

  2. Re: [PATCH] pgbench: Bug fix for the -d option

    Michael Paquier <michael@paquier.xyz> — 2021-02-26T08:16:17Z

    On Fri, Feb 26, 2021 at 01:18:20PM +0900, miyake_kouta wrote:
> I'm suggesting this bug fix because I think it's a bug, but if there's any
> other intent to this else if statement, could you let me know?

Yes, the existing code could mess up with the selection logic if
PGPORT and PGUSER are both specified in an environment, masking the
value of PGUSER, so let's fix that.  This is as old as 412893b.
--
Michael

  3. Re: [PATCH] pgbench: Bug fix for the -d option

    Michael Paquier <michael@paquier.xyz> — 2021-02-26T11:30:27Z

    On Fri, Feb 26, 2021 at 05:16:17PM +0900, Michael Paquier wrote:
> Yes, the existing code could mess up with the selection logic if
> PGPORT and PGUSER are both specified in an environment, masking the
> value of PGUSER, so let's fix that.  This is as old as 412893b.

By the way, I can help but wonder why pgbench has such a different
handling for the user name, fetching first PGUSER and then looking at
the options while most of the other binaries use get_user_name().  It
seems to me that we could simplify the handling around "login" without
really impacting the usefulness of the tool, no?
--
Michael

  4. Re: [PATCH] pgbench: Bug fix for the -d option

    miyake_kouta <miyake_kouta@oss.nttdata.com> — 2021-03-02T02:52:33Z

    2021-02-26 20:30, Michael Paquier wrote:
> By the way, I can help but wonder why pgbench has such a different
> handling for the user name, fetching first PGUSER and then looking at
> the options while most of the other binaries use get_user_name().  It
> seems to me that we could simplify the handling around "login" without
> really impacting the usefulness of the tool, no?

Hi.

Thank you for your comment.
I modified the patch based on other binaries.
In this new patch, if there is a $PGUSER, then it's set to login.
If not, get_user_name_or_exit is excuted.
Plese let me know what you think about this change.
--
Kota Miyake

  5. Re: [PATCH] pgbench: Bug fix for the -d option

    Michael Paquier <michael@paquier.xyz> — 2021-03-04T12:11:54Z

    On Tue, Mar 02, 2021 at 11:52:33AM +0900, miyake_kouta wrote:
> I modified the patch based on other binaries.
> In this new patch, if there is a $PGUSER, then it's set to login.
> If not, get_user_name_or_exit is excuted.
> Plese let me know what you think about this change.

Your patch makes the database selection slightly better, but I think
that we can do better and simpler than that.  So please see the
attached.

One thing on HEAD that looks like a bug to me is that if one uses a
pgbench command without specifying user, port and/or name in the
command for an environment without PGDATABASE, PGPORT and PGHOST set,
then the debug log just before doConnect() prints empty strings for
all that, which is basically useless so one has no idea where the
connection happens. Like any other src/bin/ facilities, let's instead
remove all the getenv() calls at the beginning of pgbench.c and let's
libpq handle those environment variables if the parameters are NULL
(aka in the case of no values given directly in the options).  This
requires to move the debug log after doConnect(), which is no big deal
anyway as a failure results in an exit(1) immediately with a log
telling where the connection failed.

What do you think?
--
Michael

  6. Re: [PATCH] pgbench: Bug fix for the -d option

    miyake_kouta <miyake_kouta@oss.nttdata.com> — 2021-03-05T02:26:45Z

    2021-03-04 21:11, Michael Paquier wrote:
> Like any other src/bin/ facilities, let's instead
> remove all the getenv() calls at the beginning of pgbench.c and let's
> libpq handle those environment variables if the parameters are NULL
> (aka in the case of no values given directly in the options).  This
> requires to move the debug log after doConnect(), which is no big deal
> anyway as a failure results in an exit(1) immediately with a log
> telling where the connection failed.

Thank you for improving my patch.
I agree that we should remove getenv() from pgbench.c and let libpq 
complement parameters with environment variables.
As you said, it's not a big problem that the debug log output  comes 
after doConnect(), I think.
Your patch is simpler and more ideal.

Regards.
--
Kota Miyake

  7. Re: [PATCH] pgbench: Bug fix for the -d option

    Fujii Masao <masao.fujii@oss.nttdata.com> — 2021-03-05T04:30:11Z

    
On 2021/03/05 11:26, miyake_kouta wrote:
> 2021-03-04 21:11, Michael Paquier wrote:
>> Like any other src/bin/ facilities, let's instead
>> remove all the getenv() calls at the beginning of pgbench.c and let's
>> libpq handle those environment variables if the parameters are NULL
>> (aka in the case of no values given directly in the options).  This
>> requires to move the debug log after doConnect(), which is no big deal
>> anyway as a failure results in an exit(1) immediately with a log
>> telling where the connection failed.

  		if ((env = getenv("PGDATABASE")) != NULL && *env != '\0')
  			dbName = env;
-		else if (login != NULL && *login != '\0')
-			dbName = login;
+		else if ((env = getenv("PGUSER")) != NULL && *env != '\0')
+			dbName = env;
  		else
-			dbName = "";
+			dbName = get_user_name_or_exit(progname);

If dbName is set by libpq, the above also is not necessary?

Regards,

-- 
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION

  8. Re: [PATCH] pgbench: Bug fix for the -d option

    Michael Paquier <michael@paquier.xyz> — 2021-03-05T07:33:12Z

    On Fri, Mar 05, 2021 at 01:30:11PM +0900, Fujii Masao wrote:
>  		if ((env = getenv("PGDATABASE")) != NULL && *env != '\0')
>  			dbName = env;
> -		else if (login != NULL && *login != '\0')
> -			dbName = login;
> +		else if ((env = getenv("PGUSER")) != NULL && *env != '\0')
> +			dbName = env;
>  		else
> -			dbName = "";
> +			dbName = get_user_name_or_exit(progname);
> 
> If dbName is set by libpq, the above also is not necessary?

If you remove this part, pgbench loses some log information if
PQconnectdbParams() returns NULL, like on OOM if the database name is
NULL.  Perhaps that's not worth caring about here for a single log
failure, but that's the reason is why I left this part around.

Now, simplifying the code is one goal of this patch, so I would not
mind shaving a bit more of it :)
--
Michael

  9. Re: [PATCH] pgbench: Bug fix for the -d option

    Fujii Masao <masao.fujii@oss.nttdata.com> — 2021-03-05T09:35:47Z

    
On 2021/03/05 16:33, Michael Paquier wrote:
> On Fri, Mar 05, 2021 at 01:30:11PM +0900, Fujii Masao wrote:
>>   		if ((env = getenv("PGDATABASE")) != NULL && *env != '\0')
>>   			dbName = env;
>> -		else if (login != NULL && *login != '\0')
>> -			dbName = login;
>> +		else if ((env = getenv("PGUSER")) != NULL && *env != '\0')
>> +			dbName = env;
>>   		else
>> -			dbName = "";
>> +			dbName = get_user_name_or_exit(progname);
>>
>> If dbName is set by libpq, the above also is not necessary?
> 
> If you remove this part, pgbench loses some log information if
> PQconnectdbParams() returns NULL, like on OOM if the database name is
> NULL.  Perhaps that's not worth caring about here for a single log
> failure, but that's the reason is why I left this part around.

Understood. Thanks!

Regards,

-- 
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION

  10. Re: [PATCH] pgbench: Bug fix for the -d option

    Michael Paquier <michael@paquier.xyz> — 2021-03-06T13:00:38Z

    On Fri, Mar 05, 2021 at 06:35:47PM +0900, Fujii Masao wrote:
> Understood. Thanks!

Okay, so I have gone through this stuff today, and applied the
simplification.  Thanks.
--
Michael

  11. Re: [PATCH] pgbench: Bug fix for the -d option

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-03-06T18:19:44Z

    Michael Paquier <michael@paquier.xyz> writes:
> On Fri, Mar 05, 2021 at 06:35:47PM +0900, Fujii Masao wrote:
>> Understood. Thanks!

> Okay, so I have gone through this stuff today, and applied the
> simplification.  Thanks.

This item is still open according to the commitfest app ---
should that entry be closed?

			regards, tom lane

  12. Re: [PATCH] pgbench: Bug fix for the -d option

    Michael Paquier <michael@paquier.xyz> — 2021-03-06T23:45:28Z

    On Sat, Mar 06, 2021 at 01:19:44PM -0500, Tom Lane wrote:
> This item is still open according to the commitfest app ---
> should that entry be closed?

Thanks.  Done.
--
Michael