Re: pgbench -f and vacuum

Currently pgbench -f (run custom script) executes vacuum against
pgbench_* tables before stating bench marking if -n (or --no-vacuum)
is not specified. If those tables do not exist, pgbench fails. To
prevent this, -n must be specified. For me this behavior seems insane
because "-f" does not necessarily suppose the existence of the
pgbench_* tables. Attached patch prevents pgbench from exiting even
if those tables do not exist. Here is the sample session:

./pgbench -f /tmp/a.sql test2
starting vacuum...ERROR: relation "pgbench_branches" does not exist
ERROR: relation "pgbench_tellers" does not exist
ERROR: relation "pgbench_history" does not exist
end.
transaction type: Custom query
scaling factor: 1
query mode: simple
number of clients: 1
number of threads: 1
number of transactions per client: 10
number of transactions actually processed: 10/10
latency average: 0.000 ms
tps = 5977.286312 (including connections establishing)
tps = 15822.784810 (excluding connections establishing)

Best regards,
--
Tatsuo Ishii
SRA OSS, Inc. Japan
English: http://www.sraoss.co.jp/index_en.php
Japanese:http://www.sraoss.co.jp

Tatsuo Ishii <ishii(at)postgresql(dot)org> writes:
> Currently pgbench -f (run custom script) executes vacuum against
> pgbench_* tables before stating bench marking if -n (or --no-vacuum)
> is not specified. If those tables do not exist, pgbench fails. To
> prevent this, -n must be specified. For me this behavior seems insane
> because "-f" does not necessarily suppose the existence of the
> pgbench_* tables. Attached patch prevents pgbench from exiting even
> if those tables do not exist.

I don't particularly care for this approach. I think if we want to
do something about this, we should just make -f imply -n. Although
really, given the lack of complaints so far, it seems like people
manage to deal with this state of affairs just fine. Do we really
need to do anything?

regards, tom lane

On 14 December 2014 at 04:39, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>
> Tatsuo Ishii <ishii(at)postgresql(dot)org> writes:
> > Currently pgbench -f (run custom script) executes vacuum against
> > pgbench_* tables before stating bench marking if -n (or --no-vacuum)
> > is not specified. If those tables do not exist, pgbench fails. To
> > prevent this, -n must be specified. For me this behavior seems insane
> > because "-f" does not necessarily suppose the existence of the
> > pgbench_* tables. Attached patch prevents pgbench from exiting even
> > if those tables do not exist.
>
> I don't particularly care for this approach. I think if we want to
> do something about this, we should just make -f imply -n. Although
> really, given the lack of complaints so far, it seems like people
> manage to deal with this state of affairs just fine. Do we really
> need to do anything?
>
>
>
I also find this weird vacuum non existing tables rather bizarre. I think
the first time I ever used pgbench I was confronted by the pgbench* tables
not existing, despite the fact that I was trying to run my own script.
Looking at --help it mentioned nothing about the pgbench_* tables, so I was
pretty confused until I opened up the online docs.

I'm not really a fan of how this is done in the proposed patch, I'd vote
for either skipping vacuum if -f is specified, or just doing a database
wide vacuum in that case. Though, that might surprise a few people, so
maybe the first option is better.

Regards

David Rowley

> On 14 December 2014 at 04:39, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>>
>> Tatsuo Ishii <ishii(at)postgresql(dot)org> writes:
>> > Currently pgbench -f (run custom script) executes vacuum against
>> > pgbench_* tables before stating bench marking if -n (or --no-vacuum)
>> > is not specified. If those tables do not exist, pgbench fails. To
>> > prevent this, -n must be specified. For me this behavior seems insane
>> > because "-f" does not necessarily suppose the existence of the
>> > pgbench_* tables. Attached patch prevents pgbench from exiting even
>> > if those tables do not exist.
>>
>> I don't particularly care for this approach. I think if we want to
>> do something about this, we should just make -f imply -n. Although
>> really, given the lack of complaints so far, it seems like people
>> manage to deal with this state of affairs just fine. Do we really
>> need to do anything?
>>
>>
>>
> I also find this weird vacuum non existing tables rather bizarre. I think
> the first time I ever used pgbench I was confronted by the pgbench* tables
> not existing, despite the fact that I was trying to run my own script.
> Looking at --help it mentioned nothing about the pgbench_* tables, so I was
> pretty confused until I opened up the online docs.
>
> I'm not really a fan of how this is done in the proposed patch, I'd vote
> for either skipping vacuum if -f is specified, or just doing a database
> wide vacuum in that case. Though, that might surprise a few people, so
> maybe the first option is better.

Problem with "-f implies -n" approach is, it breaks backward
compatibility. There are use cases using custom script *and* pgbench_*
tables. For example the particular user wants to use the standard
pgbench tables and is not satisfied with the built in scenario. I know
at least one user does this way.

Best regards,
--
Tatsuo Ishii
SRA OSS, Inc. Japan
English: http://www.sraoss.co.jp/index_en.php
Japanese:http://www.sraoss.co.jp

On 12/13/14, 6:17 PM, Tatsuo Ishii wrote:
> Problem with "-f implies -n" approach is, it breaks backward
> compatibility. There are use cases using custom script*and* pgbench_*
> tables. For example the particular user wants to use the standard
> pgbench tables and is not satisfied with the built in scenario. I know
> at least one user does this way.

If we care enough about that case to attempt the vacuum anyway then we need to do something about the error message; either squelch it or check for the existence of the tables before attempting to vacuum. Since there's no way to squelch in the server logfile, I think checking for the table is the right answer.
--
Jim Nasby, Data Architect, Blue Treble Consulting
Data in Trouble? Get it in Treble! http://BlueTreble.com

> If we care enough about that case to attempt the vacuum anyway then we
> need to do something about the error message; either squelch it or
> check for the existence of the tables before attempting to
> vacuum. Since there's no way to squelch in the server logfile, I think
> checking for the table is the right answer.

Fair enough. I will come up with "checking for table before vacuum"
approach.

Best regards,
--
Tatsuo Ishii
SRA OSS, Inc. Japan
English: http://www.sraoss.co.jp/index_en.php
Japanese:http://www.sraoss.co.jp

On 14 December 2014 at 13:50, Jim Nasby <Jim(dot)Nasby(at)bluetreble(dot)com> wrote:
>
> On 12/13/14, 6:17 PM, Tatsuo Ishii wrote:
>
>> Problem with "-f implies -n" approach is, it breaks backward
>> compatibility. There are use cases using custom script*and* pgbench_*
>> tables. For example the particular user wants to use the standard
>> pgbench tables and is not satisfied with the built in scenario. I know
>> at least one user does this way.
>>
>
> If we care enough about that case to attempt the vacuum anyway then we
> need to do something about the error message; either squelch it or check
> for the existence of the tables before attempting to vacuum. Since there's
> no way to squelch in the server logfile, I think checking for the table is
> the right answer.
>
>
Maybe someone can write a patch for VACUUM IF EXISTS ... :-)

/me hides

Regards

David Rowley

On Sat, Dec 13, 2014 at 10:39 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Tatsuo Ishii <ishii(at)postgresql(dot)org> writes:
>> Currently pgbench -f (run custom script) executes vacuum against
>> pgbench_* tables before stating bench marking if -n (or --no-vacuum)
>> is not specified. If those tables do not exist, pgbench fails. To
>> prevent this, -n must be specified. For me this behavior seems insane
>> because "-f" does not necessarily suppose the existence of the
>> pgbench_* tables. Attached patch prevents pgbench from exiting even
>> if those tables do not exist.
>
> I don't particularly care for this approach. I think if we want to
> do something about this, we should just make -f imply -n. Although
> really, given the lack of complaints so far, it seems like people
> manage to deal with this state of affairs just fine. Do we really
> need to do anything?

I would vote for changing nothing. If we make -f imply -n, then what
happens if you have a script which is a slight variant of the default
script and you *don't* want -n? Then we'll have to add yet another
pgbench option to select the default behavior, and I don't know that
the marginal usability gain of getting to leave out -n sometimes would
be enough to justify having to remember another switch.

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

On Sat, Dec 13, 2014 at 7:39 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>
> Tatsuo Ishii <ishii(at)postgresql(dot)org> writes:
> > Currently pgbench -f (run custom script) executes vacuum against
> > pgbench_* tables before stating bench marking if -n (or --no-vacuum)
> > is not specified. If those tables do not exist, pgbench fails. To
> > prevent this, -n must be specified. For me this behavior seems insane
> > because "-f" does not necessarily suppose the existence of the
> > pgbench_* tables. Attached patch prevents pgbench from exiting even
> > if those tables do not exist.
>
> I don't particularly care for this approach. I think if we want to
> do something about this, we should just make -f imply -n. Although
> really, given the lack of complaints so far, it seems like people
> manage to deal with this state of affairs just fine. Do we really
> need to do anything?
>

I hereby complain about this.

It has bugged me several times, and having the errors be non-fatal when -f
was given was the best (easy) thing I could come up with as well, but I was
too lazy to actually write the code.

Cheers,

Jeff

On 2014-12-15 10:55:30 -0800, Jeff Janes wrote:
> On Sat, Dec 13, 2014 at 7:39 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> >
> > Tatsuo Ishii <ishii(at)postgresql(dot)org> writes:
> > > Currently pgbench -f (run custom script) executes vacuum against
> > > pgbench_* tables before stating bench marking if -n (or --no-vacuum)
> > > is not specified. If those tables do not exist, pgbench fails. To
> > > prevent this, -n must be specified. For me this behavior seems insane
> > > because "-f" does not necessarily suppose the existence of the
> > > pgbench_* tables. Attached patch prevents pgbench from exiting even
> > > if those tables do not exist.
> >
> > I don't particularly care for this approach. I think if we want to
> > do something about this, we should just make -f imply -n. Although
> > really, given the lack of complaints so far, it seems like people
> > manage to deal with this state of affairs just fine. Do we really
> > need to do anything?
> >
>
> I hereby complain about this.
>
> It has bugged me several times, and having the errors be non-fatal when -f
> was given was the best (easy) thing I could come up with as well, but I was
> too lazy to actually write the code.

Same here. I vote for making -f imply -n as well.

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On Sun, Dec 14, 2014 at 11:43 AM, Tatsuo Ishii <ishii(at)postgresql(dot)org> wrote:
>> If we care enough about that case to attempt the vacuum anyway then we
>> need to do something about the error message; either squelch it or
>> check for the existence of the tables before attempting to
>> vacuum. Since there's no way to squelch in the server logfile, I think
>> checking for the table is the right answer.
>
> Fair enough. I will come up with "checking for table before vacuum"
> approach.

+1 for this approach.

Regards,

--
Fujii Masao

> On Sun, Dec 14, 2014 at 11:43 AM, Tatsuo Ishii <ishii(at)postgresql(dot)org> wrote:
>>> If we care enough about that case to attempt the vacuum anyway then we
>>> need to do something about the error message; either squelch it or
>>> check for the existence of the tables before attempting to
>>> vacuum. Since there's no way to squelch in the server logfile, I think
>>> checking for the table is the right answer.
>>
>> Fair enough. I will come up with "checking for table before vacuum"
>> approach.
>
> +1 for this approach.

Here is the patch I promised.

Best regards,
--
Tatsuo Ishii
SRA OSS, Inc. Japan
English: http://www.sraoss.co.jp/index_en.php
Japanese:http://www.sraoss.co.jp

On Sun, Dec 21, 2014 at 12:58 PM, Tatsuo Ishii <ishii(at)postgresql(dot)org> wrote:
>
> > On Sun, Dec 14, 2014 at 11:43 AM, Tatsuo Ishii <ishii(at)postgresql(dot)org>
wrote:
> >>> If we care enough about that case to attempt the vacuum anyway then we
> >>> need to do something about the error message; either squelch it or
> >>> check for the existence of the tables before attempting to
> >>> vacuum. Since there's no way to squelch in the server logfile, I think
> >>> checking for the table is the right answer.
> >>
> >> Fair enough. I will come up with "checking for table before vacuum"
> >> approach.
> >
> > +1 for this approach.
>
> Here is the patch I promised.
>

Some comments:

- Error to apply to the current master:

$ git apply /home/fabrizio/Downloads/pgbench-f-noexit-v2.patch
/home/fabrizio/Downloads/pgbench-f-noexit-v2.patch:9: trailing whitespace.
static void executeStatement2(PGconn *con, const char *sql, const char
*table);
/home/fabrizio/Downloads/pgbench-f-noexit-v2.patch:10: trailing whitespace.
static bool is_table_exists(PGconn *con, const char *table);
/home/fabrizio/Downloads/pgbench-f-noexit-v2.patch:18: trailing whitespace.
/* call executeStatement() if table exists */
/home/fabrizio/Downloads/pgbench-f-noexit-v2.patch:19: trailing whitespace.
static void
/home/fabrizio/Downloads/pgbench-f-noexit-v2.patch:20: trailing whitespace.
executeStatement2(PGconn *con, const char *sql, const char *table)
error: patch failed: contrib/pgbench/pgbench.c:88
error: contrib/pgbench/pgbench.c: patch does not apply

+static void executeStatement2(PGconn *con, const char *sql, const char
*table);

I think we can use a better name like "executeStatementIfTableExists".

+ if (result == NULL)
+ {
+ PQclear(res);
+ return false;
+ }
+
+ if (*result == 't')
+ {
+ PQclear(res);
+ return false; /* table does not exist */
+ }

To simplify isn't better this way?

if (result == NULL || *result == 't')
{
PQclear(res);
return false; /* table does not exists */
}

Regards,

--
Fabrízio de Royes Mello
Consultoria/Coaching PostgreSQL
>> Timbira: http://www.timbira.com.br
>> Blog: http://fabriziomello.github.io
>> Linkedin: http://br.linkedin.com/in/fabriziomello
>> Twitter: http://twitter.com/fabriziomello
>> Github: http://github.com/fabriziomello

> - Error to apply to the current master:

Works for me.

$ git apply ~/pgbench-f-noexit-v2.patch
$

Maybe git version difference or the patch file was malformed by mail
client?

> +static void executeStatement2(PGconn *con, const char *sql, const char
> *table);
>
> I think we can use a better name like "executeStatementIfTableExists".

Point taken.

> + if (result == NULL)
> + {
> + PQclear(res);
> + return false;
> + }
> +
> + if (*result == 't')
> + {
> + PQclear(res);
> + return false; /* table does not exist */
> + }
>
> To simplify isn't better this way?
>
> if (result == NULL || *result == 't')
> {
> PQclear(res);
> return false; /* table does not exists */
> }

Thanks for pointing it out.

Best regards,
--
Tatsuo Ishii
SRA OSS, Inc. Japan
English: http://www.sraoss.co.jp/index_en.php
Japanese:http://www.sraoss.co.jp

Hi,

On 21.12.2014 15:58, Tatsuo Ishii wrote:
>> On Sun, Dec 14, 2014 at 11:43 AM, Tatsuo Ishii <ishii(at)postgresql(dot)org> wrote:
>>>> If we care enough about that case to attempt the vacuum anyway
>>>> then we need to do something about the error message; either
>>>> squelch it or check for the existence of the tables before
>>>> attempting to vacuum. Since there's no way to squelch in the
>>>> server logfile, I think checking for the table is the right
>>>> answer.
>>>
>>> Fair enough. I will come up with "checking for table before
>>> vacuum" approach.
>>
>> +1 for this approach.
>
> Here is the patch I promised.

First of all - I'm not entirely convinced the "IF EXISTS" approach is
somehow better than "-f implies -n" suggested before, but I don't have a
strong preference either.

Regarding the patch:

(1) I agree with Fabrizio that the 'executeStatement2' is not the best
naming as it does not show the 'if exists' intent.

(2) The 'executeStatement2' API is a bit awkward as the signarure

executeStatement2(PGconn *con, const char *sql, const char *table);

suggests that the 'sql' command is executed when 'table' exists.
But that's not the case, because what actually gets executed is
'sql table'.

(3) The 'is_table_exists' should be probably just 'table_exists'.

(4) The SQL used in is_table_exists to check table existence seems
slightly wrong, because 'to_regclass' works for other relation
kinds, not just regular tables - it will match views for example.
While a conflict like that (having an object with the right name
but not a regular table) is rather unlikely I guess, a more correct
query would be this:

SELECT oid FROM pg_class WHERE relname = '%s' AND relkind = 'r';

(5) I'm not a libpq expert, but I don't see how the PQgetvalue() could
return anything except true/false, so the

if (result == NULL)
{
PQclear(res);
return false;
}

seems a bit pointless to me. But maybe it's necessary?

(6) The is_table_exists might be further simplified along these lines:

static bool
is_table_exists(PGconn *con, const char *table)
{
PGresult *res;
char buf[1024];
char *result;
bool retval;

snprintf(buf, sizeof(buf)-1,
"SELECT to_regclass('%s') IS NULL", table);

res = PQexec(con, buf);
if (PQresultStatus(res) != PGRES_TUPLES_OK)
{
return false;
}

result = PQgetvalue(res, 0, 0);

retval = (*result == 't');

PQclear(res);

return retval;
}

(7) I also find the coding in main() around line 3250 a bit clumsy. The
first reason is that it only checks existence of 'pgbench_branches'
and then vacuums three pgbench_tellers and pgbench_history in the
same block. If pgbench_branches does not exist, there will be no
message printed (but the tables will be vacuumed).

The second reason is that the msg1, msg2 variables seem unnecessary.
IMHO this is a bit better:

if (!is_no_vacuum)
{
if (is_table_exists(con, "pgbench_branches"))
{
fprintf(stderr, "starting vacuum...");

executeStatement2(con, "vacuum", "pgbench_branches");
executeStatement2(con, "vacuum", "pgbench_tellers");
executeStatement2(con, "truncate", "pgbench_history");

fprintf(stderr, "end.\n");
}

if (do_vacuum_accounts)
{
if (is_table_exists(con, "pgbench_accounts"))
{
fprintf(stderr, "starting vacuum pgbench_accounts...");

executeStatement(con,
"vacuum analyze pgbench_accounts");

fprintf(stderr, "end.\n");
}
}
}

(8) Also, I think it's not necessary to define function prototypes for
executeStatement2 and is_table_exists. It certainly is not
consistent with the other functions defined in pgbench.c (e.g.
there's no prototype for executeStatement). Just delete the two
prototypes and move is_table_exists before executeStatement2.

regards
Tomas

> Hi,
>
> On 21.12.2014 15:58, Tatsuo Ishii wrote:
>>> On Sun, Dec 14, 2014 at 11:43 AM, Tatsuo Ishii <ishii(at)postgresql(dot)org> wrote:
>>>>> If we care enough about that case to attempt the vacuum anyway
>>>>> then we need to do something about the error message; either
>>>>> squelch it or check for the existence of the tables before
>>>>> attempting to vacuum. Since there's no way to squelch in the
>>>>> server logfile, I think checking for the table is the right
>>>>> answer.
>>>>
>>>> Fair enough. I will come up with "checking for table before
>>>> vacuum" approach.
>>>
>>> +1 for this approach.
>>
>> Here is the patch I promised.
>
> First of all - I'm not entirely convinced the "IF EXISTS" approach is
> somehow better than "-f implies -n" suggested before, but I don't have a
> strong preference either.
>
> Regarding the patch:
>
> (1) I agree with Fabrizio that the 'executeStatement2' is not the best
> naming as it does not show the 'if exists' intent.
>
>
> (2) The 'executeStatement2' API is a bit awkward as the signarure
>
> executeStatement2(PGconn *con, const char *sql, const char *table);
>
> suggests that the 'sql' command is executed when 'table' exists.
> But that's not the case, because what actually gets executed is
> 'sql table'.

Any replacement idea for "sql" and "table"?

> (3) The 'is_table_exists' should be probably just 'table_exists'.
>
>
> (4) The SQL used in is_table_exists to check table existence seems
> slightly wrong, because 'to_regclass' works for other relation
> kinds, not just regular tables - it will match views for example.
> While a conflict like that (having an object with the right name
> but not a regular table) is rather unlikely I guess, a more correct
> query would be this:
>
> SELECT oid FROM pg_class WHERE relname = '%s' AND relkind = 'r';

This doesn't always work if schema search path is set.

> (5) I'm not a libpq expert, but I don't see how the PQgetvalue() could
> return anything except true/false, so the
>
> if (result == NULL)
> {
> PQclear(res);
> return false;
> }
>
> seems a bit pointless to me. But maybe it's necessary?

PQgetvalue could return NULL, if the parameter is wrong. I don't want
to raise segfault in any case.

> (6) The is_table_exists might be further simplified along these lines:
>
> static bool
> is_table_exists(PGconn *con, const char *table)
> {
> PGresult *res;
> char buf[1024];
> char *result;
> bool retval;
>
> snprintf(buf, sizeof(buf)-1,
> "SELECT to_regclass('%s') IS NULL", table);
>
> res = PQexec(con, buf);
> if (PQresultStatus(res) != PGRES_TUPLES_OK)
> {
> return false;
> }
>
> result = PQgetvalue(res, 0, 0);
>
> retval = (*result == 't');
>
> PQclear(res);
>
> return retval;
> }
>
>
> (7) I also find the coding in main() around line 3250 a bit clumsy. The
> first reason is that it only checks existence of 'pgbench_branches'
> and then vacuums three pgbench_tellers and pgbench_history in the
> same block. If pgbench_branches does not exist, there will be no
> message printed (but the tables will be vacuumed).

So we should check the existence of all pgbench_branches,
pgbench_tellers, pgbench_history and pgbench_accounts? Not sure if
it's worth the trouble.

> The second reason is that the msg1, msg2 variables seem unnecessary.
> IMHO this is a bit better:

This will behave differently from what pgbench it is now. If -f is not
specified and pgbench_* does not exist, then pgbech silently skipps
vacuum. Today pgbench raises error in this case.

> if (!is_no_vacuum)
> {
> if (is_table_exists(con, "pgbench_branches"))
> {
> fprintf(stderr, "starting vacuum...");
>
> executeStatement2(con, "vacuum", "pgbench_branches");
> executeStatement2(con, "vacuum", "pgbench_tellers");
> executeStatement2(con, "truncate", "pgbench_history");
>
> fprintf(stderr, "end.\n");
> }
>
> if (do_vacuum_accounts)
> {
> if (is_table_exists(con, "pgbench_accounts"))
> {
> fprintf(stderr, "starting vacuum pgbench_accounts...");
>
> executeStatement(con,
> "vacuum analyze pgbench_accounts");
>
> fprintf(stderr, "end.\n");
> }
> }
> }
>
> (8) Also, I think it's not necessary to define function prototypes for
> executeStatement2 and is_table_exists. It certainly is not
> consistent with the other functions defined in pgbench.c (e.g.
> there's no prototype for executeStatement). Just delete the two
> prototypes and move is_table_exists before executeStatement2.

I think not having static function prototypes is not a good
custom. See other source code in PostgreSQL.

Best regards,
--
Tatsuo Ishii
SRA OSS, Inc. Japan
English: http://www.sraoss.co.jp/index_en.php
Japanese:http://www.sraoss.co.jp

On 22.12.2014 07:36, Tatsuo Ishii wrote:
> On 22.12.2014 00:28, Tomas Vondra wrote:
>>
>> (2) The 'executeStatement2' API is a bit awkward as the signarure
>>
>> executeStatement2(PGconn *con, const char *sql, const char *table);
>>
>> suggests that the 'sql' command is executed when 'table' exists.
>> But that's not the case, because what actually gets executed is
>> 'sql table'.
>
> Any replacement idea for "sql" and "table"?

What about removing the concatenation logic, and just passing the whole
query to executeStatement2()? The queries are reasonably short, IMHO.

>> (3) The 'is_table_exists' should be probably just 'table_exists'.
>>
>>
>> (4) The SQL used in is_table_exists to check table existence seems
>> slightly wrong, because 'to_regclass' works for other relation
>> kinds, not just regular tables - it will match views for example.
>> While a conflict like that (having an object with the right name
>> but not a regular table) is rather unlikely I guess, a more correct
>> query would be this:
>>
>> SELECT oid FROM pg_class WHERE relname = '%s' AND relkind = 'r';
>
> This doesn't always work if schema search path is set.

True. My point was that the to_regclass() is not exactly sufficient.
Maybe that's acceptable, maybe not.

>> (5) I'm not a libpq expert, but I don't see how the PQgetvalue() could
>> return anything except true/false, so the
>>
>> if (result == NULL)
>> {
>> PQclear(res);
>> return false;
>> }
>>
>> seems a bit pointless to me. But maybe it's necessary?
>
> PQgetvalue could return NULL, if the parameter is wrong. I don't want
> to raise segfault in any case.

So, how could the parameter be wrong? Or what about using PQgetisnull()?

>> (7) I also find the coding in main() around line 3250 a bit clumsy. The
>> first reason is that it only checks existence of 'pgbench_branches'
>> and then vacuums three pgbench_tellers and pgbench_history in the
>> same block. If pgbench_branches does not exist, there will be no
>> message printed (but the tables will be vacuumed).
>
> So we should check the existence of all pgbench_branches,
> pgbench_tellers, pgbench_history and pgbench_accounts? Not sure if
> it's worth the trouble.

I'm not sure. But if we use 'at least one of the tables exists' logic,
then I don't see a reason for msg1 and msg2. A single flag would be
sufficient.

>> The second reason is that the msg1, msg2 variables seem unnecessary.
>> IMHO this is a bit better:
>
> This will behave differently from what pgbench it is now. If -f is not
> specified and pgbench_* does not exist, then pgbech silently skipps
> vacuum. Today pgbench raises error in this case.

I don't understand. I believe the code I proposed does exactly the same
thing as the patch, no? I certainly don't see any error messages in the
patch ...

>> (8) Also, I think it's not necessary to define function prototypes for
>> executeStatement2 and is_table_exists. It certainly is not
>> consistent with the other functions defined in pgbench.c (e.g.
>> there's no prototype for executeStatement). Just delete the two
>> prototypes and move is_table_exists before executeStatement2.
>
> I think not having static function prototypes is not a good
> custom. See other source code in PostgreSQL.

Yes, but apparently pgbench.c does not do that. It's strange to have
prototypes for just two of many functions in the file.

Tomas

Tomas Vondra wrote:
> On 22.12.2014 07:36, Tatsuo Ishii wrote:
> > On 22.12.2014 00:28, Tomas Vondra wrote:

> >> (8) Also, I think it's not necessary to define function prototypes for
> >> executeStatement2 and is_table_exists. It certainly is not
> >> consistent with the other functions defined in pgbench.c (e.g.
> >> there's no prototype for executeStatement). Just delete the two
> >> prototypes and move is_table_exists before executeStatement2.
> >
> > I think not having static function prototypes is not a good
> > custom. See other source code in PostgreSQL.
>
> Yes, but apparently pgbench.c does not do that. It's strange to have
> prototypes for just two of many functions in the file.

Whenever a function is defined before its first use, a prototype is not
mandatory, so we tend to omit them, but I'm pretty sure there are cases
where we add them anyway. I my opinion, rearranging code so that called
functions appear first just to avoid the prototype is not a very good
way to organize things, though. I haven't looked at this patch so I
don't know whether this is what's being done here.

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

On 22.12.2014 17:47, Alvaro Herrera wrote:
> Tomas Vondra wrote:
>> On 22.12.2014 07:36, Tatsuo Ishii wrote:
>>> On 22.12.2014 00:28, Tomas Vondra wrote:
>
>>>> (8) Also, I think it's not necessary to define function prototypes for
>>>> executeStatement2 and is_table_exists. It certainly is not
>>>> consistent with the other functions defined in pgbench.c (e.g.
>>>> there's no prototype for executeStatement). Just delete the two
>>>> prototypes and move is_table_exists before executeStatement2.
>>>
>>> I think not having static function prototypes is not a good
>>> custom. See other source code in PostgreSQL.
>>
>> Yes, but apparently pgbench.c does not do that. It's strange to have
>> prototypes for just two of many functions in the file.
>
> Whenever a function is defined before its first use, a prototype is
> not mandatory, so we tend to omit them, but I'm pretty sure there are
> cases where we add them anyway. I my opinion, rearranging code so
> that called functions appear first just to avoid the prototype is not
> a very good way to organize things, though. I haven't looked at this
> patch so I don't know whether this is what's being done here.

I'm not objecting to prototypes in general, but I believe the principle
is to respect how the existing code is written. There are almost no
other prototypes in pgbench.c - e.g. there are no prototypes for
executeStatement(), init() etc. so adding the prototypes in this patch
seems inconsistent. But maybe that's nitpicking.

Tomas

Tomas Vondra wrote:

> I'm not objecting to prototypes in general, but I believe the principle
> is to respect how the existing code is written. There are almost no
> other prototypes in pgbench.c - e.g. there are no prototypes for
> executeStatement(), init() etc. so adding the prototypes in this patch
> seems inconsistent. But maybe that's nitpicking.

Well, given that Tatsuo-san invented pgbench in the first place, I think
he has enough authority to decide whether to add prototypes or not.

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

On 2014-12-22 18:17:56 +0100, Tomas Vondra wrote:
> On 22.12.2014 17:47, Alvaro Herrera wrote:
> > Tomas Vondra wrote:
> >> On 22.12.2014 07:36, Tatsuo Ishii wrote:
> >>> On 22.12.2014 00:28, Tomas Vondra wrote:
> >
> >>>> (8) Also, I think it's not necessary to define function prototypes for
> >>>> executeStatement2 and is_table_exists. It certainly is not
> >>>> consistent with the other functions defined in pgbench.c (e.g.
> >>>> there's no prototype for executeStatement). Just delete the two
> >>>> prototypes and move is_table_exists before executeStatement2.
> >>>
> >>> I think not having static function prototypes is not a good
> >>> custom. See other source code in PostgreSQL.
> >>
> >> Yes, but apparently pgbench.c does not do that. It's strange to have
> >> prototypes for just two of many functions in the file.
> >
> > Whenever a function is defined before its first use, a prototype is
> > not mandatory, so we tend to omit them, but I'm pretty sure there are
> > cases where we add them anyway. I my opinion, rearranging code so
> > that called functions appear first just to avoid the prototype is not
> > a very good way to organize things, though. I haven't looked at this
> > patch so I don't know whether this is what's being done here.
>
> I'm not objecting to prototypes in general, but I believe the principle
> is to respect how the existing code is written. There are almost no
> other prototypes in pgbench.c - e.g. there are no prototypes for
> executeStatement(), init() etc. so adding the prototypes in this patch
> seems inconsistent. But maybe that's nitpicking.

I don't see a point in avoiding prototypes if the author thinks it makes
his patch clearer. And it's not like pgbench is the pinnacle of beauty
that would be greatly disturbed by any changes to its form.

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On 22.12.2014 18:41, Andres Freund wrote:
> On 2014-12-22 18:17:56 +0100, Tomas Vondra wrote:
>> On 22.12.2014 17:47, Alvaro Herrera wrote:
>>> Tomas Vondra wrote:
>>>> On 22.12.2014 07:36, Tatsuo Ishii wrote:
>>>>> On 22.12.2014 00:28, Tomas Vondra wrote:
>>>
>>>>>> (8) Also, I think it's not necessary to define function prototypes for
>>>>>> executeStatement2 and is_table_exists. It certainly is not
>>>>>> consistent with the other functions defined in pgbench.c (e.g.
>>>>>> there's no prototype for executeStatement). Just delete the two
>>>>>> prototypes and move is_table_exists before executeStatement2.
>>>>>
>>>>> I think not having static function prototypes is not a good
>>>>> custom. See other source code in PostgreSQL.
>>>>
>>>> Yes, but apparently pgbench.c does not do that. It's strange to have
>>>> prototypes for just two of many functions in the file.
>>>
>>> Whenever a function is defined before its first use, a prototype is
>>> not mandatory, so we tend to omit them, but I'm pretty sure there are
>>> cases where we add them anyway. I my opinion, rearranging code so
>>> that called functions appear first just to avoid the prototype is not
>>> a very good way to organize things, though. I haven't looked at this
>>> patch so I don't know whether this is what's being done here.
>>
>> I'm not objecting to prototypes in general, but I believe the
>> principle is to respect how the existing code is written. There are
>> almost no other prototypes in pgbench.c - e.g. there are no
>> prototypes for executeStatement(), init() etc. so adding the
>> prototypes in this patch seems inconsistent. But maybe that's
>> nitpicking.
>
> I don't see a point in avoiding prototypes if the author thinks it
> makes his patch clearer. And it's not like pgbench is the pinnacle
> of beauty that would be greatly disturbed by any changes to its form.

I'm not recommending to avoid them (although I'm not sure they make the
patch/code cleaner in this case). It was just a minor observation that
the patch introduces prototypes into code where currently are none. So
executeStatement2() has a prototype while executeStatement() doesn't.

This certainly is not a problem that would make the patch uncommitable,
and yes, it's up to the author to either add prototypes or not. I'm not
going to fight for removing or keeping them.

regards
Tomas

> First of all - I'm not entirely convinced the "IF EXISTS" approach is
> somehow better than "-f implies -n" suggested before, but I don't have a
> strong preference either.

I revisited the "-f implies -n" approach again. The main reason why I
wanted to avoid the approach was, it breaks the backward
compatibility. However if we were not going to back port the patch,
the approach is simpler and cleaner from the point of code
organization, I think (the patch I posted already make it impossible
to back port because to_regclass is used) .

However there's another problem with the approach. If we want to use
-f *and* run vacuum before testing, currently there's no way to do
it. "-v" might help, but it runs vacuum against pgbench_accounts
(without -v, pgbench runs vacuum against pgbench_* except
pgbench_accounts). To solve the problem, we would need to add opposite
option to -n, "run VACUUM before tests except pgbench_accounts"
(suppose the option be "-k"). But maybe someone said "why don't we
vacuum always pgbench_accounts? These days machines are pretty fast
and we don't need to care about it any more."

So my questions are:

1) Which approach is better "IF EXISTS" or "-f implies -n"?

2) If latter is better, do we need to add "-k" option? Or it's not
worth the trouble?
--
Tatsuo Ishii
SRA OSS, Inc. Japan
English: http://www.sraoss.co.jp/index_en.php
Japanese:http://www.sraoss.co.jp

Here's a completely different idea. How about we add an option that
means "vacuum this table before running the test" (can be given several
times); by default the set of vacuumed tables is the current pgbench_*
list, but if -f is specified then the default set is cleared. So if you
have a -f script and want to vacuum the default tables, you're forced to
give a few --vacuum-table=foo options. But this gives the option to
vacuum some other table before the test, not just the pgbench default
ones.

This is a bit more complicated, and makes life more difficult to people
using -f and the default pgbench tables than your proposed new -k
switch. But it's more general and it might have more use cases; and
people using -f with the default pgbench tables are probably rare,
anyway.

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

On Mon, Dec 22, 2014 at 6:55 PM, Alvaro Herrera
<alvherre(at)2ndquadrant(dot)com> wrote:
> Here's a completely different idea. How about we add an option that
> means "vacuum this table before running the test" (can be given several
> times); by default the set of vacuumed tables is the current pgbench_*
> list, but if -f is specified then the default set is cleared. So if you
> have a -f script and want to vacuum the default tables, you're forced to
> give a few --vacuum-table=foo options. But this gives the option to
> vacuum some other table before the test, not just the pgbench default
> ones.

Well, really, you might want arbitrary initialization steps, not just
vacuums. We could have --init-script=FILENAME. Although that might
be taking this thread rather far off-topic.

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

Robert Haas wrote:
> On Mon, Dec 22, 2014 at 6:55 PM, Alvaro Herrera
> <alvherre(at)2ndquadrant(dot)com> wrote:
> > Here's a completely different idea. How about we add an option that
> > means "vacuum this table before running the test" (can be given several
> > times); by default the set of vacuumed tables is the current pgbench_*
> > list, but if -f is specified then the default set is cleared. So if you
> > have a -f script and want to vacuum the default tables, you're forced to
> > give a few --vacuum-table=foo options. But this gives the option to
> > vacuum some other table before the test, not just the pgbench default
> > ones.
>
> Well, really, you might want arbitrary initialization steps, not just
> vacuums. We could have --init-script=FILENAME.

"Init" (pgbench -i) is the step that creates the tables and populates
them, so I think this would need a different name, maybe "startup," but
otherwise yeah.

> Although that might be taking this thread rather far off-topic.

Not really sure about that, because the only outstanding objection to
this discussion is what happens in the startup stage if you specify -f.
Right now vacuum is attempted on the standard tables, which is probably
not the right thing in the vast majority of cases. But if we turn that
off, how do we reinstate it for the rare cases that want it? Personally
I would just leave it turned off and be done with it, but if we want to
provide some way to re-enable it, this --startup-script=FILE gadget
sounds like a pretty decent idea.

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

On Wed, Dec 24, 2014 at 12:42 AM, Alvaro Herrera
<alvherre(at)2ndquadrant(dot)com> wrote:
>> Although that might be taking this thread rather far off-topic.
> Not really sure about that, because the only outstanding objection to
> this discussion is what happens in the startup stage if you specify -f.
> Right now vacuum is attempted on the standard tables, which is probably
> not the right thing in the vast majority of cases. But if we turn that
> off, how do we reinstate it for the rare cases that want it? Personally
> I would just leave it turned off and be done with it, but if we want to
> provide some way to re-enable it, this --startup-script=FILE gadget
> sounds like a pretty decent idea.
(Catching up with this thread)
Yes I think that it would be more simple to simply turn off the
internal VACUUM if -f is specified. For the cases where we still need
to vacuum the tables pgbench_*, we could simply document to run a
VACUUM on them.
--
Michael

>>> Although that might be taking this thread rather far off-topic.
>> Not really sure about that, because the only outstanding objection to
>> this discussion is what happens in the startup stage if you specify -f.
>> Right now vacuum is attempted on the standard tables, which is probably
>> not the right thing in the vast majority of cases. But if we turn that
>> off, how do we reinstate it for the rare cases that want it? Personally
>> I would just leave it turned off and be done with it, but if we want to
>> provide some way to re-enable it, this --startup-script=FILE gadget
>> sounds like a pretty decent idea.
> (Catching up with this thread)
> Yes I think that it would be more simple to simply turn off the
> internal VACUUM if -f is specified. For the cases where we still need
> to vacuum the tables pgbench_*, we could simply document to run a
> VACUUM on them.

Agreed. Here is the patch to implement the idea: -f just implies -n.

Best regards,
--
Tatsuo Ishii
SRA OSS, Inc. Japan
English: http://www.sraoss.co.jp/index_en.php
Japanese:http://www.sraoss.co.jp

On Mon, Feb 9, 2015 at 2:54 PM, Tatsuo Ishii wrote:
> Agreed. Here is the patch to implement the idea: -f just implies -n.

Some small comments:
- is_no_vacuum, as well as is_init_mode, are defined as an integers
but their use imply that they are boolean switches. This patch sets
is_no_vacuum to true, while the rest of the code actually increment
its value when -n is used. Why not simply changing both flags as
booleans? My suggestion is not really related to this patch and purely
cosmetic but we could change things at the same time, or update your
patch to increment to is_no_vacuum++ when -f is used.
- The documentation misses some markups for pgbench and VACUUM and did
not respect the 80-character limit.
Attached is an updated patch with those things changed.
Regards,
--
Michael

> On Mon, Feb 9, 2015 at 2:54 PM, Tatsuo Ishii wrote:
>> Agreed. Here is the patch to implement the idea: -f just implies -n.
>
> Some small comments:
> - is_no_vacuum, as well as is_init_mode, are defined as an integers
> but their use imply that they are boolean switches. This patch sets
> is_no_vacuum to true, while the rest of the code actually increment
> its value when -n is used. Why not simply changing both flags as
> booleans? My suggestion is not really related to this patch and purely
> cosmetic but we could change things at the same time, or update your
> patch to increment to is_no_vacuum++ when -f is used.

Yes, I have to admit that the current pgench code is quite confusing
in this regard. I think we should change is_no_vacuum and is_init_mode
to boolean.

> - The documentation misses some markups for pgbench and VACUUM and did
> not respect the 80-character limit.

I didn't realize that there's such a style guide. Although I think
it's a good thing, I just want to know where such a guide is
described.

> Attached is an updated patch with those things changed.

Looks good.

Best regards,
--
Tatsuo Ishii
SRA OSS, Inc. Japan
English: http://www.sraoss.co.jp/index_en.php
Japanese:http://www.sraoss.co.jp

On Tue, Feb 10, 2015 at 5:03 PM, Tatsuo Ishii wrote:
>> - The documentation misses some markups for pgbench and VACUUM and did
>> not respect the 80-character limit.
>
> I didn't realize that there's such a style guide. Although I think
> it's a good thing, I just want to know where such a guide is
> described.

That's self-learning based on the style of the other pages. I don't
recall if there are actually convention guidelines for the docs, the
only I know of being the coding convention here:
http://www.postgresql.org/docs/devel/static/source.html
Maybe we could have a section dedicated to that. Thoughts?
--
Michael

> On Tue, Feb 10, 2015 at 5:03 PM, Tatsuo Ishii wrote:
>>> - The documentation misses some markups for pgbench and VACUUM and did
>>> not respect the 80-character limit.
>>
>> I didn't realize that there's such a style guide. Although I think
>> it's a good thing, I just want to know where such a guide is
>> described.
>
> That's self-learning based on the style of the other pages. I don't
> recall if there are actually convention guidelines for the docs, the
> only I know of being the coding convention here:
> http://www.postgresql.org/docs/devel/static/source.html
> Maybe we could have a section dedicated to that. Thoughts?

I think you need to talk to Peter.

Best regards,
--
Tatsuo Ishii
SRA OSS, Inc. Japan
English: http://www.sraoss.co.jp/index_en.php
Japanese:http://www.sraoss.co.jp

On 2/10/15 3:12 AM, Michael Paquier wrote:
> On Tue, Feb 10, 2015 at 5:03 PM, Tatsuo Ishii wrote:
>>> - The documentation misses some markups for pgbench and VACUUM and did
>>> not respect the 80-character limit.
>>
>> I didn't realize that there's such a style guide. Although I think
>> it's a good thing, I just want to know where such a guide is
>> described.
>
> That's self-learning based on the style of the other pages. I don't
> recall if there are actually convention guidelines for the docs, the
> only I know of being the coding convention here:
> http://www.postgresql.org/docs/devel/static/source.html
> Maybe we could have a section dedicated to that. Thoughts?

We have http://www.postgresql.org/docs/devel/static/docguide-style.html,
although that doesn't cover formatting. For that we have .dir-locals.el:

(sgml-mode . ((fill-column . 78)
(indent-tabs-mode . nil))))

;-)

Feel free to improve that.

On Tue, Dec 23, 2014 at 7:42 AM, Alvaro Herrera <alvherre(at)2ndquadrant(dot)com>
wrote:

> Robert Haas wrote:
> > On Mon, Dec 22, 2014 at 6:55 PM, Alvaro Herrera
> > <alvherre(at)2ndquadrant(dot)com> wrote:
> > > Here's a completely different idea. How about we add an option that
> > > means "vacuum this table before running the test" (can be given several
> > > times); by default the set of vacuumed tables is the current pgbench_*
> > > list, but if -f is specified then the default set is cleared. So if
> you
> > > have a -f script and want to vacuum the default tables, you're forced
> to
> > > give a few --vacuum-table=foo options. But this gives the option to
> > > vacuum some other table before the test, not just the pgbench default
> > > ones.
> >
> > Well, really, you might want arbitrary initialization steps, not just
> > vacuums. We could have --init-script=FILENAME.
>
> "Init" (pgbench -i) is the step that creates the tables and populates
> them, so I think this would need a different name, maybe "startup," but
> otherwise yeah.
>
> > Although that might be taking this thread rather far off-topic.
>
> Not really sure about that, because the only outstanding objection to
> this discussion is what happens in the startup stage if you specify -f.
> Right now vacuum is attempted on the standard tables, which is probably
> not the right thing in the vast majority of cases. But if we turn that
> off, how do we reinstate it for the rare cases that want it? Personally
> I would just leave it turned off and be done with it, but if we want to
> provide some way to re-enable it, this --startup-script=FILE gadget
> sounds like a pretty decent idea.
>

There are two (or more?) possible meanings of a startup script. One would
be run a single time at the start of a pgbench run, and one would be run at
the start of each connection, in the case of -C or -c. Vacuums would
presumably go in the first category, while something like tweaking a
work_mem or enable_* setting would use the second. I'd find more use for
the second way.

I had a patch to do this on a per connection basis a while ago, but it took
the command as a string to --startup. Robert suggested it be a filename
rather than a string, and I agreed but never followed up with a different
patch, as I couldn't figure out how to refactor the code that parses -f
files so that it could be used for this without undo replication of the
code.

See <
http://www.postgresql.org/message-id/CAMkU=1xV3tYKoHD8U2mQzfC5Kbn_bdcVf8br-EnUvy-6Z=B47w@mail.gmail.com
>

I was wondering if we could't invent three new backslash commands.

One would precede an SQL command to be run during -i, and ignored any other
time (and then during -i any unbackslashed commands would be ignored)

One would precede an SQL command to be run upon starting up a pgbench run.

One would precede an SQL command to be run upon starting up a benchmarking
connection.

That way you could have a single file that would record its own
initialization requirements.

One problem is I don't know how you would merge together multiple -f
arguments. Another is I would want to be able to override the
per-connection command without having to use sed or something to
edit-in-place the SQL file.

But as far as what has been discussed on the central topic of this thread,
I think that doing the vacuum and making the failure for non-existent
tables be non-fatal when -f is provided would be an improvement. Or maybe
just making it non-fatal at all times--if the table is needed and not
present, the session will fail quite soon anyway. I don't see the other
changes as being improvements. I would rather just learn to add the -n
when I use -f and don't have the default tables in place, than have to
learn new methods for saying "no really, I left -n off on purpose" when I
have a custom file which does use the default tables and I want them
vacuumed.

Cheers,

Jeff

On Wed, Feb 11, 2015 at 2:00 PM, Jeff Janes <jeff(dot)janes(at)gmail(dot)com> wrote:
> I would rather just learn to add the -n when I use -f
> and don't have the default tables in place, than have to learn new methods
> for saying "no really, I left -n off on purpose" when I have a custom file
> which does use the default tables and I want them vacuumed.

Quite.

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

On Wed, Feb 11, 2015 at 2:00 PM, Jeff Janes <jeff(dot)janes(at)gmail(dot)com> wrote:
> But as far as what has been discussed on the central topic of this thread, I
> think that doing the vacuum and making the failure for non-existent tables
> be non-fatal when -f is provided would be an improvement. Or maybe just
> making it non-fatal at all times--if the table is needed and not present,
> the session will fail quite soon anyway. I don't see the other changes as
> being improvements. I would rather just learn to add the -n when I use -f
> and don't have the default tables in place, than have to learn new methods
> for saying "no really, I left -n off on purpose" when I have a custom file
> which does use the default tables and I want them vacuumed.

So, discussion seems to have died off here. I think what Jeff is
proposing here is a reasonable compromise. Patch for that attached.

Objections?

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

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> On Wed, Feb 11, 2015 at 2:00 PM, Jeff Janes <jeff(dot)janes(at)gmail(dot)com> wrote:
>> But as far as what has been discussed on the central topic of this thread, I
>> think that doing the vacuum and making the failure for non-existent tables
>> be non-fatal when -f is provided would be an improvement. Or maybe just
>> making it non-fatal at all times--if the table is needed and not present,
>> the session will fail quite soon anyway. I don't see the other changes as
>> being improvements. I would rather just learn to add the -n when I use -f
>> and don't have the default tables in place, than have to learn new methods
>> for saying "no really, I left -n off on purpose" when I have a custom file
>> which does use the default tables and I want them vacuumed.

> So, discussion seems to have died off here. I think what Jeff is
> proposing here is a reasonable compromise. Patch for that attached.

+1 as to the basic behavior, but I'm not convinced that this is
user-friendly reporting:

+ if (PQresultStatus(res) != PGRES_COMMAND_OK)
+ fprintf(stderr, "%s", PQerrorMessage(con));

I would be a bit surprised to see pgbench report an ERROR and then
continue on anyway; I might think that was a bug, even. I am not
sure exactly what it should print instead though. Some perhaps viable
proposals:

* don't print anything at all, just chug along.

* do something like
fprintf(stderr, "Ignoring: %s", PQerrorMessage(con));

* add something like "(Ignoring this error and continuing anyway)"
on a line after the error message.

(I realize this takes us right back into the bikeshedding game, but
I do think that what's displayed is important.)

regards, tom lane

On Thu, Apr 30, 2015 at 4:17 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>> On Wed, Feb 11, 2015 at 2:00 PM, Jeff Janes <jeff(dot)janes(at)gmail(dot)com> wrote:
>>> But as far as what has been discussed on the central topic of this thread, I
>>> think that doing the vacuum and making the failure for non-existent tables
>>> be non-fatal when -f is provided would be an improvement. Or maybe just
>>> making it non-fatal at all times--if the table is needed and not present,
>>> the session will fail quite soon anyway. I don't see the other changes as
>>> being improvements. I would rather just learn to add the -n when I use -f
>>> and don't have the default tables in place, than have to learn new methods
>>> for saying "no really, I left -n off on purpose" when I have a custom file
>>> which does use the default tables and I want them vacuumed.
>
>> So, discussion seems to have died off here. I think what Jeff is
>> proposing here is a reasonable compromise. Patch for that attached.
>
> +1 as to the basic behavior, but I'm not convinced that this is
> user-friendly reporting:
>
> + if (PQresultStatus(res) != PGRES_COMMAND_OK)
> + fprintf(stderr, "%s", PQerrorMessage(con));
>
> I would be a bit surprised to see pgbench report an ERROR and then
> continue on anyway; I might think that was a bug, even. I am not
> sure exactly what it should print instead though. Some perhaps viable
> proposals:
>
> * don't print anything at all, just chug along.
>
> * do something like
> fprintf(stderr, "Ignoring: %s", PQerrorMessage(con));
>
> * add something like "(Ignoring this error and continuing anyway)"
> on a line after the error message.
>
> (I realize this takes us right back into the bikeshedding game, but
> I do think that what's displayed is important.)

I tried it out before sending the patch and it's really not that bad.
It's says:

starting vacuum.... ERROR: blah
ERROR: blah
ERROR: blah
done

And then continues on. Sure, that's not the greatest error reporting
output ever, but what do you expect from pgbench? I think it's clear
enough what's going on there. The messages appear in quick
succession, because it doesn't take very long to notice that the table
isn't there, so it's not like you are sitting there going "wait,
what?".

If we're going to add something, I like your second suggestion
"(ignoring this error and continuing anyway)" more than the first one.
Putting "ignoring:" before the thing you plan to ignore will be
confusing, I think.

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

* Robert Haas (robertmhaas(at)gmail(dot)com) wrote:
> It's says:
>
> starting vacuum.... ERROR: blah
> ERROR: blah
> ERROR: blah
> done
>
> And then continues on. Sure, that's not the greatest error reporting
> output ever, but what do you expect from pgbench? I think it's clear
> enough what's going on there. The messages appear in quick
> succession, because it doesn't take very long to notice that the table
> isn't there, so it's not like you are sitting there going "wait,
> what?".
>
> If we're going to add something, I like your second suggestion
> "(ignoring this error and continuing anyway)" more than the first one.
> Putting "ignoring:" before the thing you plan to ignore will be
> confusing, I think.

+1 to adding "(ignoring this error and continuing anyway)" and
committing this.

Thanks!

Stephen

On Tue, May 12, 2015 at 12:08 PM, Stephen Frost <sfrost(at)snowman(dot)net> wrote:
> * Robert Haas (robertmhaas(at)gmail(dot)com) wrote:
>> It's says:
>>
>> starting vacuum.... ERROR: blah
>> ERROR: blah
>> ERROR: blah
>> done
>>
>> And then continues on. Sure, that's not the greatest error reporting
>> output ever, but what do you expect from pgbench? I think it's clear
>> enough what's going on there. The messages appear in quick
>> succession, because it doesn't take very long to notice that the table
>> isn't there, so it's not like you are sitting there going "wait,
>> what?".
>>
>> If we're going to add something, I like your second suggestion
>> "(ignoring this error and continuing anyway)" more than the first one.
>> Putting "ignoring:" before the thing you plan to ignore will be
>> confusing, I think.
>
> +1 to adding "(ignoring this error and continuing anyway)" and
> committing this.

You want to take care of that?

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

* Robert Haas (robertmhaas(at)gmail(dot)com) wrote:
> On Tue, May 12, 2015 at 12:08 PM, Stephen Frost <sfrost(at)snowman(dot)net> wrote:
> > * Robert Haas (robertmhaas(at)gmail(dot)com) wrote:
> >> It's says:
> >>
> >> starting vacuum.... ERROR: blah
> >> ERROR: blah
> >> ERROR: blah
> >> done
> >>
> >> And then continues on. Sure, that's not the greatest error reporting
> >> output ever, but what do you expect from pgbench? I think it's clear
> >> enough what's going on there. The messages appear in quick
> >> succession, because it doesn't take very long to notice that the table
> >> isn't there, so it's not like you are sitting there going "wait,
> >> what?".
> >>
> >> If we're going to add something, I like your second suggestion
> >> "(ignoring this error and continuing anyway)" more than the first one.
> >> Putting "ignoring:" before the thing you plan to ignore will be
> >> confusing, I think.
> >
> > +1 to adding "(ignoring this error and continuing anyway)" and
> > committing this.
>
> You want to take care of that?

Sure.

Thanks!

Stephen

* Robert Haas (robertmhaas(at)gmail(dot)com) wrote:
> On Tue, May 12, 2015 at 12:08 PM, Stephen Frost <sfrost(at)snowman(dot)net> wrote:
> > * Robert Haas (robertmhaas(at)gmail(dot)com) wrote:
> >> It's says:
> >>
> >> starting vacuum.... ERROR: blah
> >> ERROR: blah
> >> ERROR: blah
> >> done
> >>
> >> And then continues on. Sure, that's not the greatest error reporting
> >> output ever, but what do you expect from pgbench? I think it's clear
> >> enough what's going on there. The messages appear in quick
> >> succession, because it doesn't take very long to notice that the table
> >> isn't there, so it's not like you are sitting there going "wait,
> >> what?".
> >>
> >> If we're going to add something, I like your second suggestion
> >> "(ignoring this error and continuing anyway)" more than the first one.
> >> Putting "ignoring:" before the thing you plan to ignore will be
> >> confusing, I think.
> >
> > +1 to adding "(ignoring this error and continuing anyway)" and
> > committing this.
>
> You want to take care of that?

Done. Results looked good to me:

--> pgbench -f test.sql
starting vacuum...ERROR: relation "pgbench_branches" does not exist
(ignoring this error and continuing anyway)
ERROR: relation "pgbench_tellers" does not exist
(ignoring this error and continuing anyway)
ERROR: relation "pgbench_history" does not exist
(ignoring this error and continuing anyway)
end.
transaction type: Custom query
scaling factor: 1
query mode: simple
[...]

CF app updated.

Thanks!

Stephen