Hello David,
I'd prefer not to expand the use of pqexpbuffer in more places, and
instead rather see this use StringInfo, now that's also available to
frontend programs.
Franckly, one or the other does not matter much to me.
FWIW, I agree with Andres with regard to using StringInfo.
Ok. I find it strange to mix PQExpBuffer & StringInfo in the same file.
Also, the changes to executeStatementExpect() and adding executeStatement()
do not seem to fit in with the purpose of this patch.
Yep, that was in passing.
Attached a v6 which uses StringInfo, and the small refactoring as a
separate patch.
--
Fabien.
diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c
index b8864c6ae5..40cae7b121 100644
--- a/src/bin/pgbench/pgbench.c
+++ b/src/bin/pgbench/pgbench.c
@@ -62,6 +62,7 @@
#include "fe_utils/cancel.h"
#include "fe_utils/conditional.h"
#include "getopt_long.h"
+#include "lib/stringinfo.h"
#include "libpq-fe.h"
#include "pgbench.h"
#include "portability/instr_time.h"
@@ -599,7 +600,9 @@ static void doLog(TState *thread, CState *st,
StatsData *agg, bool skipped, double latency, double lag);
static void processXactStats(TState *thread, CState *st, instr_time *now,
bool skipped, StatsData *agg);
-static void append_fillfactor(char *opts, int len);
+static void append_fillfactor(StringInfo query);
+static void append_tablespace(PGconn *con, StringInfo query,
+ const char *kind, const char *tablespace);
static void addScript(ParsedScript script);
static void *threadRun(void *arg);
static void finishCon(CState *st);
@@ -3608,30 +3611,27 @@ initDropTables(PGconn *con)
static void
createPartitions(PGconn *con)
{
- char ff[64];
-
- ff[0] = '\0';
-
- /*
- * Per ddlinfo in initCreateTables, fillfactor is needed on table
- * pgbench_accounts.
- */
- append_fillfactor(ff, sizeof(ff));
+ StringInfoData query;
/* we must have to create some partitions */
Assert(partitions > 0);
fprintf(stderr, "creating %d partitions...\n", partitions);
+ initStringInfo(&query);
+
for (int p = 1; p <= partitions; p++)
{
- char query[256];
-
if (partition_method == PART_RANGE)
{
int64 part_size = (naccounts * (int64) scale + partitions - 1) / partitions;
- char minvalue[32],
- maxvalue[32];
+
+ resetStringInfo(&query);
+ appendStringInfo(&query,
+ "create%s table pgbench_accounts_%d\n"
+ " partition of pgbench_accounts\n"
+ " for values from (",
+ unlogged_tables ? " unlogged" : "", p);
/*
* For RANGE, we use open-ended partitions at the beginning and
@@ -3640,34 +3640,42 @@ createPartitions(PGconn *con)
* scale, it is more generic and the performance is better.
*/
if (p == 1)
- sprintf(minvalue, "minvalue");
+ appendStringInfoString(&query, "minvalue");
else
- sprintf(minvalue, INT64_FORMAT, (p - 1) * part_size + 1);
+ appendStringInfo(&query, INT64_FORMAT, (p - 1) * part_size + 1);
+
+ appendStringInfoString(&query, ") to (");
if (p < partitions)
- sprintf(maxvalue, INT64_FORMAT, p * part_size + 1);
+ appendStringInfo(&query, INT64_FORMAT, p * part_size + 1);
else
- sprintf(maxvalue, "maxvalue");
+ appendStringInfoString(&query, "maxvalue");
- snprintf(query, sizeof(query),
- "create%s table pgbench_accounts_%d\n"
- " partition of pgbench_accounts\n"
- " for values from (%s) to (%s)%s\n",
- unlogged_tables ? " unlogged" : "", p,
- minvalue, maxvalue, ff);
+ appendStringInfoChar(&query, ')');
}
else if (partition_method == PART_HASH)
- snprintf(query, sizeof(query),
- "create%s table pgbench_accounts_%d\n"
- " partition of pgbench_accounts\n"
- " for values with (modulus %d, remainder %d)%s\n",
- unlogged_tables ? " unlogged" : "", p,
- partitions, p - 1, ff);
+ {
+ resetStringInfo(&query);
+ appendStringInfo(&query,
+ "create%s table pgbench_accounts_%d\n"
+ " partition of pgbench_accounts\n"
+ " for values with (modulus %d, remainder %d)",
+ unlogged_tables ? " unlogged" : "", p,
+ partitions, p - 1);
+ }
else /* cannot get there */
Assert(0);
- executeStatement(con, query);
+ /*
+ * Per ddlinfo in initCreateTables, fillfactor is needed on table
+ * pgbench_accounts.
+ */
+ append_fillfactor(&query);
+
+ executeStatement(con, query.data);
}
+
+ pfree(query.data);
}
/*
@@ -3720,48 +3728,40 @@ initCreateTables(PGconn *con)
1
}
};
- int i;
+
+ StringInfoData query;
fprintf(stderr, "creating tables...\n");
- for (i = 0; i < lengthof(DDLs); i++)
+ initStringInfo(&query);
+
+ for (int i = 0; i < lengthof(DDLs); i++)
{
- char opts[256];
- char buffer[256];
const struct ddlinfo *ddl = &DDLs[i];
- const char *cols;
/* Construct new create table statement. */
- opts[0] = '\0';
+ resetStringInfo(&query);
+ appendStringInfo(&query, "create%s table %s(%s)",
+ unlogged_tables ? " unlogged" : "",
+ ddl->table,
+ (scale >= SCALE_32BIT_THRESHOLD) ? ddl->bigcols : ddl->smcols);
/* Partition pgbench_accounts table */
if (partition_method != PART_NONE && strcmp(ddl->table, "pgbench_accounts") == 0)
- snprintf(opts + strlen(opts), sizeof(opts) - strlen(opts),
- " partition by %s (aid)", PARTITION_METHOD[partition_method]);
+ appendStringInfo(&query,
+ " partition by %s (aid)", PARTITION_METHOD[partition_method]);
else if (ddl->declare_fillfactor)
/* fillfactor is only expected on actual tables */
- append_fillfactor(opts, sizeof(opts));
+ append_fillfactor(&query);
if (tablespace != NULL)
- {
- char *escape_tablespace;
+ append_tablespace(con, &query, "", tablespace);
- escape_tablespace = PQescapeIdentifier(con, tablespace,
- strlen(tablespace));
- snprintf(opts + strlen(opts), sizeof(opts) - strlen(opts),
- " tablespace %s", escape_tablespace);
- PQfreemem(escape_tablespace);
- }
-
- cols = (scale >= SCALE_32BIT_THRESHOLD) ? ddl->bigcols : ddl->smcols;
-
- snprintf(buffer, sizeof(buffer), "create%s table %s(%s)%s",
- unlogged_tables ? " unlogged" : "",
- ddl->table, cols, opts);
-
- executeStatement(con, buffer);
+ executeStatement(con, query.data);
}
+ pfree(query.data);
+
if (partition_method != PART_NONE)
createPartitions(con);
}
@@ -3772,10 +3772,23 @@ initCreateTables(PGconn *con)
* XXX - As default is 100, it could be removed in this case.
*/
static void
-append_fillfactor(char *opts, int len)
+append_fillfactor(StringInfo query)
{
- snprintf(opts + strlen(opts), len - strlen(opts),
- " with (fillfactor=%d)", fillfactor);
+ appendStringInfo(query, " with (fillfactor=%d)", fillfactor);
+}
+
+/*
+ * add tablespace option.
+ */
+static void
+append_tablespace(PGconn *con, StringInfo query, const char *kind,
+ const char *tablespace)
+{
+ char *escape_tablespace;
+
+ escape_tablespace = PQescapeIdentifier(con, tablespace, strlen(tablespace));
+ appendStringInfo(query, "%s tablespace %s", kind, escape_tablespace);
+ PQfreemem(escape_tablespace);
}
/*
@@ -3797,10 +3810,8 @@ initTruncateTables(PGconn *con)
static void
initGenerateDataClientSide(PGconn *con)
{
- char sql[256];
- PGresult *res;
- int i;
- int64 k;
+ PGresult *res;
+ StringInfoData sql;
/* used to track elapsed time and estimate of the remaining time */
instr_time start,
@@ -3823,26 +3834,30 @@ initGenerateDataClientSide(PGconn *con)
/* truncate away any old data */
initTruncateTables(con);
+ initStringInfo(&sql);
+
/*
* fill branches, tellers, accounts in that order in case foreign keys
* already exist
*/
- for (i = 0; i < nbranches * scale; i++)
+ for (int i = 0; i < nbranches * scale; i++)
{
/* "filler" column defaults to NULL */
- snprintf(sql, sizeof(sql),
- "insert into pgbench_branches(bid,bbalance) values(%d,0)",
- i + 1);
- executeStatement(con, sql);
+ resetStringInfo(&sql);
+ appendStringInfo(&sql,
+ "insert into pgbench_branches(bid,bbalance) values(%d,0)",
+ i + 1);
+ executeStatement(con, sql.data);
}
- for (i = 0; i < ntellers * scale; i++)
+ for (int i = 0; i < ntellers * scale; i++)
{
/* "filler" column defaults to NULL */
- snprintf(sql, sizeof(sql),
- "insert into pgbench_tellers(tid,bid,tbalance) values (%d,%d,0)",
- i + 1, i / ntellers + 1);
- executeStatement(con, sql);
+ resetStringInfo(&sql);
+ appendStringInfo(&sql,
+ "insert into pgbench_tellers(tid,bid,tbalance) values (%d,%d,0)",
+ i + 1, i / ntellers + 1);
+ executeStatement(con, sql.data);
}
/*
@@ -3858,15 +3873,17 @@ initGenerateDataClientSide(PGconn *con)
INSTR_TIME_SET_CURRENT(start);
- for (k = 0; k < (int64) naccounts * scale; k++)
+ /* printf overheads should probably be avoided... */
+ for (int64 k = 0; k < (int64) naccounts * scale; k++)
{
int64 j = k + 1;
/* "filler" column defaults to blank padded empty string */
- snprintf(sql, sizeof(sql),
- INT64_FORMAT "\t" INT64_FORMAT "\t%d\t\n",
- j, k / naccounts + 1, 0);
- if (PQputline(con, sql))
+ resetStringInfo(&sql);
+ appendStringInfo(&sql,
+ INT64_FORMAT "\t" INT64_FORMAT "\t%d\t\n",
+ j, k / naccounts + 1, 0);
+ if (PQputline(con, sql.data))
{
pg_log_fatal("PQputline failed");
exit(1);
@@ -3928,6 +3945,8 @@ initGenerateDataClientSide(PGconn *con)
exit(1);
}
+ pfree(sql.data);
+
executeStatement(con, "commit");
}
@@ -3941,7 +3960,7 @@ initGenerateDataClientSide(PGconn *con)
static void
initGenerateDataServerSide(PGconn *con)
{
- char sql[256];
+ StringInfoData sql;
fprintf(stderr, "generating data (server-side)...\n");
@@ -3954,24 +3973,30 @@ initGenerateDataServerSide(PGconn *con)
/* truncate away any old data */
initTruncateTables(con);
- snprintf(sql, sizeof(sql),
- "insert into pgbench_branches(bid,bbalance) "
- "select bid, 0 "
- "from generate_series(1, %d) as bid", nbranches * scale);
- executeStatement(con, sql);
+ initStringInfo(&sql);
- snprintf(sql, sizeof(sql),
- "insert into pgbench_tellers(tid,bid,tbalance) "
- "select tid, (tid - 1) / %d + 1, 0 "
- "from generate_series(1, %d) as tid", ntellers, ntellers * scale);
- executeStatement(con, sql);
+ appendStringInfo(&sql,
+ "insert into pgbench_branches(bid,bbalance) "
+ "select bid, 0 "
+ "from generate_series(1, %d) as bid", nbranches * scale);
+ executeStatement(con, sql.data);
- snprintf(sql, sizeof(sql),
- "insert into pgbench_accounts(aid,bid,abalance,filler) "
- "select aid, (aid - 1) / %d + 1, 0, '' "
- "from generate_series(1, "INT64_FORMAT") as aid",
- naccounts, (int64) naccounts * scale);
- executeStatement(con, sql);
+ resetStringInfo(&sql);
+ appendStringInfo(&sql,
+ "insert into pgbench_tellers(tid,bid,tbalance) "
+ "select tid, (tid - 1) / %d + 1, 0 "
+ "from generate_series(1, %d) as tid", ntellers, ntellers * scale);
+ executeStatement(con, sql.data);
+
+ resetStringInfo(&sql);
+ appendStringInfo(&sql,
+ "insert into pgbench_accounts(aid,bid,abalance,filler) "
+ "select aid, (aid - 1) / %d + 1, 0, '' "
+ "from generate_series(1, "INT64_FORMAT") as aid",
+ naccounts, (int64) naccounts * scale);
+ executeStatement(con, sql.data);
+
+ pfree(sql.data);
executeStatement(con, "commit");
}
@@ -4000,28 +4025,23 @@ initCreatePKeys(PGconn *con)
"alter table pgbench_tellers add primary key (tid)",
"alter table pgbench_accounts add primary key (aid)"
};
- int i;
+ StringInfoData query;
fprintf(stderr, "creating primary keys...\n");
- for (i = 0; i < lengthof(DDLINDEXes); i++)
- {
- char buffer[256];
+ initStringInfo(&query);
- strlcpy(buffer, DDLINDEXes[i], sizeof(buffer));
+ for (int i = 0; i < lengthof(DDLINDEXes); i++)
+ {
+ resetStringInfo(&query);
+ appendStringInfoString(&query, DDLINDEXes[i]);
if (index_tablespace != NULL)
- {
- char *escape_tablespace;
+ append_tablespace(con, &query, " using index", index_tablespace);
- escape_tablespace = PQescapeIdentifier(con, index_tablespace,
- strlen(index_tablespace));
- snprintf(buffer + strlen(buffer), sizeof(buffer) - strlen(buffer),
- " using index tablespace %s", escape_tablespace);
- PQfreemem(escape_tablespace);
- }
-
- executeStatement(con, buffer);
+ executeStatement(con, query.data);
}
+
+ pfree(query.data);
}
/*
@@ -4079,13 +4099,13 @@ checkInitSteps(const char *initialize_steps)
static void
runInitSteps(const char *initialize_steps)
{
- PQExpBufferData stats;
+ StringInfoData stats;
PGconn *con;
const char *step;
double run_time = 0.0;
bool first = true;
- initPQExpBuffer(&stats);
+ initStringInfo(&stats);
if ((con = doConnect()) == NULL)
exit(1);
@@ -4148,11 +4168,11 @@ runInitSteps(const char *initialize_steps)
elapsed_sec = INSTR_TIME_GET_DOUBLE(diff);
if (!first)
- appendPQExpBufferStr(&stats, ", ");
+ appendStringInfoString(&stats, ", ");
else
first = false;
- appendPQExpBuffer(&stats, "%s %.2f s", op, elapsed_sec);
+ appendStringInfo(&stats, "%s %.2f s", op, elapsed_sec);
run_time += elapsed_sec;
}
@@ -4161,7 +4181,7 @@ runInitSteps(const char *initialize_steps)
fprintf(stderr, "done in %.2f s (%s).\n", run_time, stats.data);
ResetCancelConn();
PQfinish(con);
- termPQExpBuffer(&stats);
+ pfree(stats.data);
}
/*
diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c
index 40cae7b121..b773f14f7e 100644
--- a/src/bin/pgbench/pgbench.c
+++ b/src/bin/pgbench/pgbench.c
@@ -1137,35 +1137,38 @@ accumStats(StatsData *stats, bool skipped, double lat, double lag)
}
}
-/* call PQexec() and exit() on failure */
+/* call PQexec() and possibly exit() on failure */
static void
-executeStatement(PGconn *con, const char *sql)
+executeStatementExpect(PGconn *con, const char *sql,
+ const ExecStatusType expected, bool errorOK)
{
PGresult *res;
res = PQexec(con, sql);
- if (PQresultStatus(res) != PGRES_COMMAND_OK)
+ if (PQresultStatus(res) != expected)
{
pg_log_fatal("query failed: %s", PQerrorMessage(con));
pg_log_info("query was: %s", sql);
- exit(1);
+ if (errorOK)
+ pg_log_info("(ignoring this error and continuing anyway)");
+ else
+ exit(1);
}
PQclear(res);
}
+/* execute sql command, which must work, or die if not */
+static void
+executeStatement(PGconn *con, const char *sql)
+{
+ executeStatementExpect(con, sql, PGRES_COMMAND_OK, false);
+}
+
/* 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)
- {
- pg_log_error("%s", PQerrorMessage(con));
- pg_log_info("(ignoring this error and continuing anyway)");
- }
- PQclear(res);
+ executeStatementExpect(con, sql, PGRES_COMMAND_OK, true);
}
/* set up a connection to the backend */
@@ -3810,7 +3813,6 @@ initTruncateTables(PGconn *con)
static void
initGenerateDataClientSide(PGconn *con)
{
- PGresult *res;
StringInfoData sql;
/* used to track elapsed time and estimate of the remaining time */
@@ -3863,13 +3865,7 @@ initGenerateDataClientSide(PGconn *con)
/*
* accounts is big enough to be worth using COPY and tracking runtime
*/
- res = PQexec(con, "copy pgbench_accounts from stdin");
- if (PQresultStatus(res) != PGRES_COPY_IN)
- {
- pg_log_fatal("unexpected copy in result: %s", PQerrorMessage(con));
- exit(1);
- }
- PQclear(res);
+ executeStatementExpect(con, "copy pgbench_accounts from stdin", PGRES_COPY_IN, false);
INSTR_TIME_SET_CURRENT(start);
