BUG #12379: pgbench should hint to pgbench -i

The following bug has been logged on the website:

Bug reference: 12379
Logged by: Benjamin
Email address: mail(at)bwe(dot)im
PostgreSQL version: 9.3.5
Operating system: 3.15.3-1-ARCH
Description:

$ pgbench bench -s 90000000 -j 1 -c 10
ERROR: relation "pgbench_branches" does not exist
LINE 1: select count(*) from pgbench_branches

11:51 MatheusOl$ bwe: You need to run `pgbench -i` first

Why does the error message does not tell me this plainly? :)

> $ pgbench bench -s 90000000 -j 1 -c 10
> ERROR: relation "pgbench_branches" does not exist
> LINE 1: select count(*) from pgbench_branches
>
> 11:51 MatheusOl$ bwe: You need to run `pgbench -i` first
>
> Why does the error message does not tell me this plainly? :)

Let us be nice to users. Attached is a very small patch which adds a
simple HINT on this initial failure, for submission to the next
commitfest.

sh> ./pgbench foo
ERROR: relation "pgbench_branches" does not exist
LINE 1: select count(*) from pgbench_branches
^
HINT: is "foo" the right database? did you initialize first (pgbench -i)?

--
Fabien.

> sh> ./pgbench foo
> ERROR: relation "pgbench_branches" does not exist
> LINE 1: select count(*) from pgbench_branches
> ^
> HINT: is "foo" the right database? did you initialize first (pgbench -i)?

Here is a rebase, and it uses two spaces after the colon like it seems it
is done elsewhere.

HINT: is "foo" the right database? did you initialize first (pgbench -i)?

--
Fabien.

Fabien COELHO <coelho(at)cri(dot)ensmp(dot)fr> writes:
>
>> sh> ./pgbench foo
>> ERROR: relation "pgbench_branches" does not exist
>> LINE 1: select count(*) from pgbench_branches
>> ^
>> HINT: is "foo" the right database? did you initialize first (pgbench -i)?
>
> Here is a rebase, and it uses two spaces after the colon like it seems it
> is done elsewhere.
>
> HINT: is "foo" the right database? did you initialize first (pgbench -i)?

I don't think there is precedent for spitting out "HINT:" in client code
(at least grep didn't find it). So this message might look like it's
coming from the server, while it doesn't.

I think the following is closer to pre-existing experience, IMO:

diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c
index 8b8b591..8d8c1fc 100644
--- a/src/bin/pgbench/pgbench.c
+++ b/src/bin/pgbench/pgbench.c
@@ -3250,6 +3250,7 @@ main(int argc, char **argv)
if (PQresultStatus(res) != PGRES_TUPLES_OK)
{
fprintf(stderr, "%s", PQerrorMessage(con));
+ fprintf(stderr, "Is \"%s\" the right database? Did you initialize it first (pgbench -i)?\n", dbName);
exit(1);
}
scale = atoi(PQgetvalue(res, 0, 0));

Hello Oleksandr,

>> HINT: is "foo" the right database? did you initialize first (pgbench -i)?
>
> I don't think there is precedent for spitting out "HINT:" in client code
> (at least grep didn't find it). So this message might look like it's
> coming from the server, while it doesn't.

I do not see that as a problem. From my point of view a "HINT" is just
what it is, and the message says clearly that it comes from pgbench.

> I think the following is closer to pre-existing experience, IMO:
> + fprintf(stderr, "Is \"%s\" the right database? Did you initialize it first (pgbench -i)?\n", dbName);

Hmmm. I liked to have a prefix on the line, that gives a purpose to the
sentence. Maybe:

HINT (pgbench): Is "foo" the right database? Did you initialize (-i) first?

Or

pgbench: ...

Or

pgbench HINT: ...

--
Fabien.

On Thu, May 21, 2015 at 10:10 AM, Fabien COELHO <coelho(at)cri(dot)ensmp(dot)fr> wrote:

>
> Hello Oleksandr,
>
> HINT: is "foo" the right database? did you initialize first (pgbench -i)?
>>>
>>
>> I don't think there is precedent for spitting out "HINT:" in client code
>> (at least grep didn't find it). So this message might look like it's
>> coming from the server, while it doesn't.
>>
>
> I do not see that as a problem. From my point of view a "HINT" is just
> what it is, and the message says clearly that it comes from pgbench.
>
> I think the following is closer to pre-existing experience, IMO:
>> + fprintf(stderr, "Is \"%s\" the right database? Did you initialize it
>> first (pgbench -i)?\n", dbName);
>>
>
> Hmmm. I liked to have a prefix on the line, that gives a purpose to the
> sentence. Maybe:
>
> HINT (pgbench): Is "foo" the right database? Did you initialize (-i)
> first?
>
> Or
>
> pgbench: ...
>
> Or
>
> pgbench HINT: ...

Well, prefixing with "<binaryname>: " makes sense to me in the general
case. But would it not be odd to do that for this message, and not the
whole lot of the rest of stuff pgbench can print to stderr, which are not
fatal errors, that is: hints?

--
Alex

>>>> HINT: is "foo" the right database? did you initialize first (pgbench -i)?

>>> Is "doo" the right database? Did you initialize it first (pgbench -i)?

>> HINT (pgbench): Is "foo" the right database? Did you initialize (-i) first?

>> pgbench: ...

>> pgbench HINT: ...

> Well, prefixing with "<binaryname>: " makes sense to me in the general
> case. But would it not be odd to do that for this message, and not the
> whole lot of the rest of stuff pgbench can print to stderr, which are not
> fatal errors, that is: hints?

Sure, it would not be strictly homogeneous with other stuff.

On the other hand, ISTM desirable that the "hint"ness quality of the
message is clear, because this is directly actionnable help, where other
messages just report a failure (cannot open log file, failed to set timer,
whatever), but do not provide much help for solving it, so I would rather
keep something with the "hint" word somewhere.

--
Fabien.

>> HINT: is "foo" the right database? did you initialize first (pgbench -i)?
>
> I don't think there is precedent for spitting out "HINT:" in client code
> (at least grep didn't find it). So this message might look like it's
> coming from the server, while it doesn't.

Here is a v3, which makes it clearer that the hint comes from pgbench.

HINT pgbench: is "foo" the right database? did you initialize first (-i)?

--
Fabien.

On 22/05/2015 11:33, Fabien COELHO wrote:
>
>>> HINT: is "foo" the right database? did you initialize first (pgbench
>>> -i)?
>>
>> I don't think there is precedent for spitting out "HINT:" in client code
>> (at least grep didn't find it). So this message might look like it's
>> coming from the server, while it doesn't.
>
> Here is a v3, which makes it clearer that the hint comes from pgbench.
>
> HINT pgbench: is "foo" the right database? did you initialize first (-i)?
>

Hello,

I've just reviewed this patch. First, I've attached a rebased version
(v4), as the v3 doesn't apply anymore.

The patch is pretty simple, so everything is ok.

But I've noticed that if you don't specify any database to pgbench, you
get an empty database information:

HINT pgbench: is "" the right database? did you initialize
first (-i)?

This isn't related to this patch specifically, but it may be confusing
for users. At least more than the other places it could already appear
(debug mode and failed connection attempt).

As the database name is optional, wouldn't it be better to fix this
behavior?

Regards.

--
Julien Rouhaud
http://dalibo.com - http://dalibo.org

Le 4 juil. 2015 10:22 PM, "Julien Rouhaud" <julien(dot)rouhaud(at)dalibo(dot)com> a
écrit :
>
> On 22/05/2015 11:33, Fabien COELHO wrote:
> >
> >>> HINT: is "foo" the right database? did you initialize first (pgbench
> >>> -i)?
> >>
> >> I don't think there is precedent for spitting out "HINT:" in client
code
> >> (at least grep didn't find it). So this message might look like it's
> >> coming from the server, while it doesn't.
> >
> > Here is a v3, which makes it clearer that the hint comes from pgbench.
> >
> > HINT pgbench: is "foo" the right database? did you initialize first
(-i)?
> >
>
> Hello,
>
> I've just reviewed this patch. First, I've attached a rebased version
> (v4), as the v3 doesn't apply anymore.
>
> The patch is pretty simple, so everything is ok.
>
>
> But I've noticed that if you don't specify any database to pgbench, you
> get an empty database information:
>
> HINT pgbench: is "" the right database? did you initialize
> first (-i)?
>
>
> This isn't related to this patch specifically, but it may be confusing
> for users. At least more than the other places it could already appear
> (debug mode and failed connection attempt).
>
> As the database name is optional, wouldn't it be better to fix this
> behavior?
>

I can't look at the code right now, but doesn't it use PQdb() to get the
name of the database? If it doesn't, it probably should.

--
Guillaume

On 05/07/2015 08:57, Guillaume Lelarge wrote:
> Le 4 juil. 2015 10:22 PM, "Julien Rouhaud" <julien(dot)rouhaud(at)dalibo(dot)com
> <mailto:julien(dot)rouhaud(at)dalibo(dot)com>> a écrit :
>>
>> On 22/05/2015 11:33, Fabien COELHO wrote:
>> >
>> >>> HINT: is "foo" the right database? did you initialize first (pgbench
>> >>> -i)?
>> >>
>> >> I don't think there is precedent for spitting out "HINT:" in client
> code
>> >> (at least grep didn't find it). So this message might look like it's
>> >> coming from the server, while it doesn't.
>> >
>> > Here is a v3, which makes it clearer that the hint comes from pgbench.
>> >
>> > HINT pgbench: is "foo" the right database? did you initialize first
> (-i)?
>> >
>>
>> Hello,
>>
>> I've just reviewed this patch. First, I've attached a rebased version
>> (v4), as the v3 doesn't apply anymore.
>>
>> The patch is pretty simple, so everything is ok.
>>
>>
>> But I've noticed that if you don't specify any database to pgbench, you
>> get an empty database information:
>>
>> HINT pgbench: is "" the right database? did you initialize
>> first (-i)?
>>
>>
>> This isn't related to this patch specifically, but it may be confusing
>> for users. At least more than the other places it could already appear
>> (debug mode and failed connection attempt).
>>
>> As the database name is optional, wouldn't it be better to fix this
>> behavior?
>>
>
> I can't look at the code right now, but doesn't it use PQdb() to get the
> name of the database? If it doesn't, it probably should.
>

Well, it doesn't.

I'm attaching a new version of the patch, which retrieves the actual
database name once connected if needed. That only solves this message
though, the other ones being displayed before the connection is available.

--
Julien Rouhaud
http://dalibo.com - http://dalibo.org

Hello Guillaume & Julien,

Thanks for the comments and the improvements!

>> I can't look at the code right now, but doesn't it use PQdb() to get the
>> name of the database? If it doesn't, it probably should.
>
> I'm attaching a new version of the patch, which retrieves the actual
> database name once connected if needed. That only solves this message
> though, the other ones being displayed before the connection is available.

I'm not sure it is worth bothering with setting dbname which seems not to
be used afterwards anyway. I would suggest to coldly call PQdb on the
connection for the error message and thus simplify the code, as attached.

--
Fabien.

Le 5 juil. 2015 6:28 PM, "Fabien COELHO" <coelho(at)cri(dot)ensmp(dot)fr> a écrit :
>
>
> Hello Guillaume & Julien,
>
> Thanks for the comments and the improvements!
>
>>> I can't look at the code right now, but doesn't it use PQdb() to get the
>>> name of the database? If it doesn't, it probably should.
>>
>>
>> I'm attaching a new version of the patch, which retrieves the actual
>> database name once connected if needed. That only solves this message
>> though, the other ones being displayed before the connection is
available.
>
>
> I'm not sure it is worth bothering with setting dbname which seems not to
be used afterwards anyway. I would suggest to coldly call PQdb on the
connection for the error message and thus simplify the code, as attached.
>

That's fine with me.

--
Guillaume

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 05/07/2015 18:34, Guillaume Lelarge wrote:
> Le 5 juil. 2015 6:28 PM, "Fabien COELHO" <coelho(at)cri(dot)ensmp(dot)fr
> <mailto:coelho(at)cri(dot)ensmp(dot)fr>> a écrit :
>>
>>
>> Hello Guillaume & Julien,
>>
>> Thanks for the comments and the improvements!
>>
>>>> I can't look at the code right now, but doesn't it use PQdb()
>>>> to get the name of the database? If it doesn't, it probably
>>>> should.
>>>
>>>
>>> I'm attaching a new version of the patch, which retrieves the
>>> actual database name once connected if needed. That only solves
>>> this message though, the other ones being displayed before the
>>> connection is
> available.
>>
>>
>> I'm not sure it is worth bothering with setting dbname which
>> seems not
> to be used afterwards anyway. I would suggest to coldly call PQdb
> on the connection for the error message and thus simplify the code,
> as attached.
>>
>
> That's fine with me.
>

I totally agree. I mark the patch as ready for committer.

> -- Guillaume
>

- --
Julien Rouhaud
http://dalibo.com - http://dalibo.org
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.17 (GNU/Linux)

iQEcBAEBAgAGBQJVmV+uAAoJELGaJ8vfEpOqTRIIAIyht/HdeR3OwlUfkWK1z26c
m7a4B/wpwy3AtYmO5yBkCEbSZmXtsZOvZM9gL0rcdeP0Jwh/MT+yu+utz+j58jwV
6TcdzJP4EkKjjQXjkCwdPtXin+Qy5s+zTBJ9gTJflmIKlCTFa3dvX0/VWeElVp2L
bO4bPQMknLhqkqfAlGSgb60xaFl2OxTqvrvea+W/hj49LxtBaOhgh83Pe8G/MaSj
zKXAsa0UHfD1QF+goCzOFPRe5OaXgNBIUbNzL11DsfwepAIZ5yr4SKGLS3F7gAvV
sWKh8aEG6+Ysof509IYbtBSIMCpdyL5niYE9GTKUqBzP4SH6t+XBYVT4nd+CfpI=
=j/Uq
-----END PGP SIGNATURE-----

Fabien COELHO <coelho(at)cri(dot)ensmp(dot)fr> writes:
> I'm not sure it is worth bothering with setting dbname which seems not to
> be used afterwards anyway. I would suggest to coldly call PQdb on the
> connection for the error message and thus simplify the code, as attached.

I looked at this a bit and have a few comments:

* I think it might be best to restrict the hint to appear only in the
case that the SQLSTATE is ERRCODE_UNDEFINED_TABLE. There are a lot of
possible failures here, and I'm not sure this hint is apropos for any
but that one.

* I really don't care for "HINT pgbench: " as that's got pretty much
nothing to do with any other message formatting we have anywhere,
least of all the existing hint-like messages in pgbench. I've started
a different thread at
http://www.postgresql.org/message-id/22280.1436139973@sss.pgh.pa.us
to discuss what the general message formatting in pgbench ought to be,
and probably this needs to wait for the outcome of that discussion.

* I'd be a bit inclined to reduce the message to something like
Perhaps you need to do initialization ("pgbench -i") in database "foo"
so that we can avoid the question of formatting of multi-sentence hints.

regards, tom lane

> I looked at this a bit and have a few comments:
>
> * I think it might be best to restrict the hint to appear only in the
> case that the SQLSTATE is ERRCODE_UNDEFINED_TABLE. There are a lot of
> possible failures here, and I'm not sure this hint is apropos for any
> but that one.

Hmmm... the likelyhood of another kind of error is small on:

"select count(*) from pgbench_branches;"

I think the only other possibility would be a permission issue.

> * I really don't care for "HINT pgbench: " as that's got pretty much
> nothing to do with any other message formatting we have anywhere,
> least of all the existing hint-like messages in pgbench.

It is kind of a copy of postgres hints, but not more than that:

HINT: Consider increasing the configuration parameter "max_wal_size"

> I've started a different thread at
> http://www.postgresql.org/message-id/22280.1436139973@sss.pgh.pa.us to
> discuss what the general message formatting in pgbench ought to be, and
> probably this needs to wait for the outcome of that discussion.

Yep.

> * I'd be a bit inclined to reduce the message to something like
> Perhaps you need to do initialization ("pgbench -i") in database "foo"
> so that we can avoid the question of formatting of multi-sentence hints.

Why not. This sentence hints at one possible error instead of two, though,
but it may be enough.

--
Fabien.

On 06/07/2015 07:36, Fabien COELHO wrote:
>
>> I looked at this a bit and have a few comments:
>>
>> * I think it might be best to restrict the hint to appear only in the
>> case that the SQLSTATE is ERRCODE_UNDEFINED_TABLE. There are a lot of
>> possible failures here, and I'm not sure this hint is apropos for any
>> but that one.
>
> Hmmm... the likelyhood of another kind of error is small on:
>
> "select count(*) from pgbench_branches;"
>
> I think the only other possibility would be a permission issue.
>
>> * I really don't care for "HINT pgbench: " as that's got pretty much
>> nothing to do with any other message formatting we have anywhere,
>> least of all the existing hint-like messages in pgbench.
>
> It is kind of a copy of postgres hints, but not more than that:
>
> HINT: Consider increasing the configuration parameter "max_wal_size"
>
>> I've started a different thread at
>> http://www.postgresql.org/message-id/22280.1436139973@sss.pgh.pa.us to
>> discuss what the general message formatting in pgbench ought to be,
>> and probably this needs to wait for the outcome of that discussion.
>
> Yep.
>
>> * I'd be a bit inclined to reduce the message to something like
>> Perhaps you need to do initialization ("pgbench -i") in database "foo"
>> so that we can avoid the question of formatting of multi-sentence hints.
>
> Why not. This sentence hints at one possible error instead of two,
> though, but it may be enough.
>

Here's a v7 patch which display the hint only if SQLSTATE is
ERRCODE_UNDEFINED_TABLE and use the message Tom suggested, as there
wasn't any better suggestion or strong objection with it in the other
thread.
--
Julien Rouhaud
http://dalibo.com - http://dalibo.org

Hello Julien,

> Here's a v7 patch which display the hint only if SQLSTATE is
> ERRCODE_UNDEFINED_TABLE and use the message Tom suggested, as there
> wasn't any better suggestion or strong objection with it in the other
> thread.

I'm wondering why you added the define. Isn't-there some convenient
include to get errcode definitions?

--
Fabien.

On 18/07/2015 13:49, Fabien COELHO wrote:
>
> Hello Julien,
>
>> Here's a v7 patch which display the hint only if SQLSTATE is
>> ERRCODE_UNDEFINED_TABLE and use the message Tom suggested, as there
>> wasn't any better suggestion or strong objection with it in the other
>> thread.
>
> I'm wondering why you added the define. Isn't-there some convenient
> include to get errcode definitions?
>

IIRC correctly, you have to include postgres.h for that. I supposed it
was better to define it than adding such an include, since it shouldn't
change and it's already done like this in vacuumdb.c.

--
Julien Rouhaud
http://dalibo.com - http://dalibo.org

Julien Rouhaud <julien(dot)rouhaud(at)dalibo(dot)com> writes:
> On 18/07/2015 13:49, Fabien COELHO wrote:
>> I'm wondering why you added the define. Isn't-there some convenient
>> include to get errcode definitions?

> IIRC correctly, you have to include postgres.h for that. I supposed it
> was better to define it than adding such an include, since it shouldn't
> change and it's already done like this in vacuumdb.c.

Frontend code *mustn't* include postgres.h.

You could imagine including errcodes.h by itself after supplying a
suitable definition for MAKE_SQLSTATE(); but I think you'd want it to
reconstitute the five characters into a string literal, and I'm not
sure that there's any easy way to do that in C. If we wanted to
go in this direction, it might be easier to create another Perl
script that puts out an errcodes-fe.h with the errcode symbols
#defined as string literals.

The larger reason we've not done this is that, once a given errcode
is wired into some client-side code, it effectively becomes part of
the protocol and can't be reassigned. See the comments around the
places in libpq where specific errcodes are referenced. So it's
not very clear that we want to encourage frontend code to use this
technique a lot.

regards, tom lane

Tom Lane wrote:

> You could imagine including errcodes.h by itself after supplying a
> suitable definition for MAKE_SQLSTATE(); but I think you'd want it to
> reconstitute the five characters into a string literal, and I'm not
> sure that there's any easy way to do that in C. If we wanted to
> go in this direction, it might be easier to create another Perl
> script that puts out an errcodes-fe.h with the errcode symbols
> #defined as string literals.
>
> The larger reason we've not done this is that, once a given errcode
> is wired into some client-side code, it effectively becomes part of
> the protocol and can't be reassigned. See the comments around the
> places in libpq where specific errcodes are referenced. So it's
> not very clear that we want to encourage frontend code to use this
> technique a lot.

Maybe we could mark individual errcodes in errcodes.txt as "exported for
clients" with some new token, and the new errcodes-fe.h list would only
include those. That way, we only fix a limited number of codes instead
of all of them.

We could add such marks to all codes defined by the standard, since we
can't change those anyway.

--
Álvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On 07/18/2015 05:16 PM, Julien Rouhaud wrote:
> On 18/07/2015 13:49, Fabien COELHO wrote:
>>
>> Hello Julien,
>>
>>> Here's a v7 patch which display the hint only if SQLSTATE is
>>> ERRCODE_UNDEFINED_TABLE and use the message Tom suggested, as there
>>> wasn't any better suggestion or strong objection with it in the other
>>> thread.
>>
>> I'm wondering why you added the define. Isn't-there some convenient
>> include to get errcode definitions?
>
> IIRC correctly, you have to include postgres.h for that. I supposed it
> was better to define it than adding such an include, since it shouldn't
> change and it's already done like this in vacuumdb.c.

Ok, committed.

- Heikki

Hello Heikki,

>>>> Here's a v7 patch which display the hint only if SQLSTATE is
>>>> ERRCODE_UNDEFINED_TABLE and use the message Tom suggested, as there
>>>> wasn't any better suggestion or strong objection with it in the other
>>>> thread.
>>>
>>> I'm wondering why you added the define. Isn't-there some convenient
>>> include to get errcode definitions?
>>
>> IIRC correctly, you have to include postgres.h for that. I supposed it
>> was better to define it than adding such an include, since it shouldn't
>> change and it's already done like this in vacuumdb.c.
>
> Ok, committed.

Thanks. Note that the author of the final version of the patch is mostly
Julien, who implemented Tom's suggested improvement, rather than myself.

--
Fabien.