pgbench -f and vacuum

Tatsuo Ishii <ishii@postgresql.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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 14 December 2014 at 04:39, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Tatsuo Ishii <ishii@postgresql.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@sss.pgh.pa.us> wrote:

Tatsuo Ishii <ishii@postgresql.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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 14 December 2014 at 13:50, Jim Nasby <Jim.Nasby@bluetreble.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@sss.pgh.pa.us> wrote:

Tatsuo Ishii <ishii@postgresql.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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Sat, Dec 13, 2014 at 7:39 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Tatsuo Ishii <ishii@postgresql.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@sss.pgh.pa.us> wrote:

Tatsuo Ishii <ishii@postgresql.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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Sun, Dec 14, 2014 at 11:43 AM, Tatsuo Ishii <ishii@postgresql.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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Sun, Dec 14, 2014 at 11:43 AM, Tatsuo Ishii <ishii@postgresql.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@postgresql.org> wrote:

On Sun, Dec 14, 2014 at 11:43 AM, Tatsuo Ishii <ishii@postgresql.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

Show quoted text

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Hi,

On 21.12.2014 15:58, Tatsuo Ishii wrote:

On Sun, Dec 14, 2014 at 11:43 AM, Tatsuo Ishii <ishii@postgresql.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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Hi,

On 21.12.2014 15:58, Tatsuo Ishii wrote:

On Sun, Dec 14, 2014 at 11:43 AM, Tatsuo Ishii <ishii@postgresql.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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers