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

diff --git a/contrib/pgbench/pgbench.c b/contrib/pgbench/pgbench.c
index d69036a..6b07932 100644
--- a/contrib/pgbench/pgbench.c
+++ b/contrib/pgbench/pgbench.c
@@ -88,6 +88,8 @@ static int	pthread_create(pthread_t *thread, pthread_attr_t *attr, void *(*start
 static int	pthread_join(pthread_t th, void **thread_return);
 #endif
 
+static void executeStatement2(PGconn *con, const char *sql, const char *table);
+static bool is_table_exists(PGconn *con, const char *table);
 
 /********************************************************************
  * some configurable parameters */
@@ -605,6 +607,54 @@ executeStatement(PGconn *con, const char *sql)
 	PQclear(res);
 }
 
+/* call executeStatement() if table exists */
+static void
+executeStatement2(PGconn *con, const char *sql, const char *table)
+{
+	char		buf[1024];
+
+	snprintf(buf, sizeof(buf)-1, "%s %s", sql, table);
+
+	if (is_table_exists(con, table))
+			executeStatement(con, buf);
+}
+
+/*
+ * Return true if the table exists
+ */
+static bool
+is_table_exists(PGconn *con, const char *table)
+{
+	PGresult	*res;
+	char		buf[1024];
+	char		*result;
+
+	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);
+
+	if (result == NULL)
+	{
+		PQclear(res);
+		return false;
+	}
+
+	if (*result == 't')
+	{
+		PQclear(res);
+		return false;	/* table does not exist */
+	}
+
+	PQclear(res);
+	return true;
+}
+
 /* set up a connection to the backend */
 static PGconn *
 doConnect(void)
@@ -3197,17 +3247,34 @@ main(int argc, char **argv)
 
 	if (!is_no_vacuum)
 	{
-		fprintf(stderr, "starting vacuum...");
-		executeStatement(con, "vacuum pgbench_branches");
-		executeStatement(con, "vacuum pgbench_tellers");
-		executeStatement(con, "truncate pgbench_history");
-		fprintf(stderr, "end.\n");
+		bool msg1 = false;
+		bool msg2 = false;
+
+		if (is_table_exists(con, "pgbench_branches"))
+			msg1 = true;
+
+		if (is_table_exists(con, "pgbench_accounts"))
+			msg2 = true;
+
+		if (msg1)
+			fprintf(stderr, "starting vacuum...");
+
+		executeStatement2(con, "vacuum", "pgbench_branches");
+		executeStatement2(con, "vacuum", "pgbench_tellers");
+		executeStatement2(con, "truncate", "pgbench_history");
+
+		if (msg1)
+			fprintf(stderr, "end.\n");
 
 		if (do_vacuum_accounts)
 		{
-			fprintf(stderr, "starting vacuum pgbench_accounts...");
-			executeStatement(con, "vacuum analyze pgbench_accounts");
-			fprintf(stderr, "end.\n");
+			if (msg2)
+				fprintf(stderr, "starting vacuum pgbench_accounts...");
+
+			executeStatement2(con, "vacuum analyze", "pgbench_accounts");
+
+			if (msg2)
+				fprintf(stderr, "end.\n");
 		}
 	}
 	PQfinish(con);

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

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

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

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

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

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

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

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

On Mon, Dec 22, 2014 at 6:55 PM, Alvaro Herrera
<alvherre@2ndquadrant.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

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

Robert Haas wrote:

On Mon, Dec 22, 2014 at 6:55 PM, Alvaro Herrera
<alvherre@2ndquadrant.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

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

On Wed, Dec 24, 2014 at 12:42 AM, Alvaro Herrera
<alvherre@2ndquadrant.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

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

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

diff --git a/contrib/pgbench/pgbench.c b/contrib/pgbench/pgbench.c
index ddd11a0..f3d7e90 100644
--- a/contrib/pgbench/pgbench.c
+++ b/contrib/pgbench/pgbench.c
@@ -2872,6 +2872,7 @@ main(int argc, char **argv)
 			case 'f':
 				benchmarking_option_set = true;
 				ttype = 3;
+				is_no_vacuum = true;	/* "-f" implies "-n" (no vacuum) */
 				filename = pg_strdup(optarg);
 				if (process_file(filename) == false || *sql_files[num_files - 1] == NULL)
 					exit(1);
diff --git a/doc/src/sgml/pgbench.sgml b/doc/src/sgml/pgbench.sgml
index 7d203cd..b545ea3 100644
--- a/doc/src/sgml/pgbench.sgml
+++ b/doc/src/sgml/pgbench.sgml
@@ -316,6 +316,10 @@ pgbench <optional> <replaceable>options</> </optional> <replaceable>dbname</>
         <option>-N</option>, <option>-S</option>, and <option>-f</option>
         are mutually exclusive.
        </para>
+       <para>
+        Please note that <option>-f</option> implies <option>-n</option>, which means that VACUUM for the standard pgbench tables will not be performed before the bench marking.
+You should run the VACUUM command beforehand if necessary.
+       </para>
       </listitem>
      </varlistentry>
 

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

From 3553151908a1be40b67703fcc27b696bad546b96 Mon Sep 17 00:00:00 2001
From: Michael Paquier <michael@otacoo.com>
Date: Tue, 10 Feb 2015 03:59:19 +0900
Subject: [PATCH] Imply -n when using -f in pgbench

At the same time make the flags is_init_mode and is_no_vaccuum booleans.
This is a purely cosmetic change but those flags have been always used
as such, even if they were defined as integers.
---
 contrib/pgbench/pgbench.c | 9 +++++----
 doc/src/sgml/pgbench.sgml | 7 +++++++
 2 files changed, 12 insertions(+), 4 deletions(-)

diff --git a/contrib/pgbench/pgbench.c b/contrib/pgbench/pgbench.c
index ddd11a0..eed9324 100644
--- a/contrib/pgbench/pgbench.c
+++ b/contrib/pgbench/pgbench.c
@@ -2679,8 +2679,8 @@ main(int argc, char **argv)
 	int			c;
 	int			nclients = 1;	/* default number of simulated clients */
 	int			nthreads = 1;	/* default number of threads */
-	int			is_init_mode = 0;		/* initialize mode? */
-	int			is_no_vacuum = 0;		/* no vacuum at all before testing? */
+	bool		is_init_mode = false;	/* initialize mode? */
+	bool		is_no_vacuum = false;	/* no vacuum at all before testing? */
 	int			do_vacuum_accounts = 0; /* do vacuum accounts before testing? */
 	int			ttype = 0;		/* transaction type. 0: TPC-B, 1: SELECT only,
 								 * 2: skip update of branches and tellers */
@@ -2753,13 +2753,13 @@ main(int argc, char **argv)
 		switch (c)
 		{
 			case 'i':
-				is_init_mode++;
+				is_init_mode = true;
 				break;
 			case 'h':
 				pghost = pg_strdup(optarg);
 				break;
 			case 'n':
-				is_no_vacuum++;
+				is_no_vacuum = true;
 				break;
 			case 'v':
 				do_vacuum_accounts++;
@@ -2872,6 +2872,7 @@ main(int argc, char **argv)
 			case 'f':
 				benchmarking_option_set = true;
 				ttype = 3;
+				is_no_vacuum = true;	/* "-f" implies "-n" (no vacuum) */
 				filename = pg_strdup(optarg);
 				if (process_file(filename) == false || *sql_files[num_files - 1] == NULL)
 					exit(1);
diff --git a/doc/src/sgml/pgbench.sgml b/doc/src/sgml/pgbench.sgml
index 7d203cd..0070882 100644
--- a/doc/src/sgml/pgbench.sgml
+++ b/doc/src/sgml/pgbench.sgml
@@ -316,6 +316,13 @@ pgbench <optional> <replaceable>options</> </optional> <replaceable>dbname</>
         <option>-N</option>, <option>-S</option>, and <option>-f</option>
         are mutually exclusive.
        </para>
+       <para>
+        Please note that <option>-f</option> implies <option>-n</option>,
+        which means that <command>VACUUM</command> for the standard
+        <application>pgbench</application> tables will not be performed before
+        running the benchmarking. <command>VACUUM</command> command should
+        be run beforehand if needed.
+       </para>
       </listitem>
      </varlistentry>
 
-- 
2.3.0

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

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

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

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

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

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

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.

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

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

Robert Haas wrote:

On Mon, Dec 22, 2014 at 6:55 PM, Alvaro Herrera
<alvherre@2ndquadrant.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 <
/messages/by-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.janes@gmail.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

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

On Wed, Feb 11, 2015 at 2:00 PM, Jeff Janes <jeff.janes@gmail.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

diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c
index 06a4dfd..714d822 100644
--- a/src/bin/pgbench/pgbench.c
+++ b/src/bin/pgbench/pgbench.c
@@ -605,6 +605,18 @@ executeStatement(PGconn *con, const char *sql)
 	PQclear(res);
 }
 
+/* call PQexec() and complain, but without exiting, on failure */
+static void
+tryExecuteStatement(PGconn *con, const char *sql)
+{
+	PGresult   *res;
+
+	res = PQexec(con, sql);
+	if (PQresultStatus(res) != PGRES_COMMAND_OK)
+		fprintf(stderr, "%s", PQerrorMessage(con));
+	PQclear(res);
+}
+
 /* set up a connection to the backend */
 static PGconn *
 doConnect(void)
@@ -3283,15 +3295,15 @@ main(int argc, char **argv)
 	if (!is_no_vacuum)
 	{
 		fprintf(stderr, "starting vacuum...");
-		executeStatement(con, "vacuum pgbench_branches");
-		executeStatement(con, "vacuum pgbench_tellers");
-		executeStatement(con, "truncate pgbench_history");
+		tryExecuteStatement(con, "vacuum pgbench_branches");
+		tryExecuteStatement(con, "vacuum pgbench_tellers");
+		tryExecuteStatement(con, "truncate pgbench_history");
 		fprintf(stderr, "end.\n");
 
 		if (do_vacuum_accounts)
 		{
 			fprintf(stderr, "starting vacuum pgbench_accounts...");
-			executeStatement(con, "vacuum analyze pgbench_accounts");
+			tryExecuteStatement(con, "vacuum analyze pgbench_accounts");
 			fprintf(stderr, "end.\n");
 		}
 	}

Robert Haas <robertmhaas@gmail.com> writes:

On Wed, Feb 11, 2015 at 2:00 PM, Jeff Janes <jeff.janes@gmail.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

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

On Thu, Apr 30, 2015 at 4:17 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Robert Haas <robertmhaas@gmail.com> writes:

On Wed, Feb 11, 2015 at 2:00 PM, Jeff Janes <jeff.janes@gmail.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

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

* Robert Haas (robertmhaas@gmail.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@snowman.net> wrote:

* Robert Haas (robertmhaas@gmail.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

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

* Robert Haas (robertmhaas@gmail.com) wrote:

On Tue, May 12, 2015 at 12:08 PM, Stephen Frost <sfrost@snowman.net> wrote:

* Robert Haas (robertmhaas@gmail.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@gmail.com) wrote:

On Tue, May 12, 2015 at 12:08 PM, Stephen Frost <sfrost@snowman.net> wrote:

* Robert Haas (robertmhaas@gmail.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