Hello,
While developing pgbench to allow partitioned tabled, I reproduced the
string management style used in the corresponding functions, but was
pretty unhappy with that kind of pattern:
snprintf(buf + strlen(buf), sizeof(buf) - strlen(buf), ...)
However adding a feature is not the place for refactoring.
This patch refactors initialization functions so as to use PQExpBuffer
where appropriate to simplify and clarify the code. SQL commands are
generated by accumulating parts into a buffer in order, before executing
it. I also added a more generic function to execute a statement and fail
if the result is unexpected.
--
Fabien.
diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c
index e72ad0036e..5450eba4ac 100644
--- a/src/bin/pgbench/pgbench.c
+++ b/src/bin/pgbench/pgbench.c
@@ -599,7 +599,7 @@ 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(PQExpBuffer query);
static void addScript(ParsedScript script);
static void *threadRun(void *arg);
static void finishCon(CState *st);
@@ -1139,17 +1139,24 @@ accumStats(StatsData *stats, bool skipped, double lat, double lag)
/* call PQexec() and exit() on failure */
static void
+executeStatementExpect(PGconn *con, const char *sql, const ExecStatusType expected)
+{
+ PGresult *res;
+
+ res = PQexec(con, sql);
+ if (PQresultStatus(res) != expected)
+ {
+ fprintf(stderr, "%s", PQerrorMessage(con));
+ exit(1);
+ }
+ PQclear(res);
+}
+
+/* execute sql command, which must work, or die if not */
+static void
executeStatement(PGconn *con, const char *sql)
{
- PGresult *res;
-
- res = PQexec(con, sql);
- if (PQresultStatus(res) != PGRES_COMMAND_OK)
- {
- fprintf(stderr, "%s", PQerrorMessage(con));
- exit(1);
- }
- PQclear(res);
+ executeStatementExpect(con, sql, PGRES_COMMAND_OK);
}
/* call PQexec() and complain, but without exiting, on failure */
@@ -3631,30 +3638,26 @@ 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));
+ PQExpBufferData query;
/* we must have to create some partitions */
Assert(partitions > 0);
fprintf(stderr, "creating %d partitions...\n", partitions);
+ initPQExpBuffer(&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];
+
+ printfPQExpBuffer(&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
@@ -3663,34 +3666,39 @@ createPartitions(PGconn *con)
* scale, it is more generic and the performance is better.
*/
if (p == 1)
- sprintf(minvalue, "minvalue");
+ appendPQExpBufferStr(&query, "minvalue");
else
- sprintf(minvalue, INT64_FORMAT, (p - 1) * part_size + 1);
+ appendPQExpBuffer(&query, INT64_FORMAT, (p - 1) * part_size + 1);
+
+ appendPQExpBufferStr(&query, ") to (");
if (p < partitions)
- sprintf(maxvalue, INT64_FORMAT, p * part_size + 1);
+ appendPQExpBuffer(&query, INT64_FORMAT, p * part_size + 1);
else
- sprintf(maxvalue, "maxvalue");
+ appendPQExpBufferStr(&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);
+ appendPQExpBufferChar(&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);
+ printfPQExpBuffer(&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);
}
+
+ termPQExpBuffer(&query);
}
/*
@@ -3743,27 +3751,30 @@ initCreateTables(PGconn *con)
1
}
};
- int i;
+
+ PQExpBufferData query;
fprintf(stderr, "creating tables...\n");
- for (i = 0; i < lengthof(DDLs); i++)
+ initPQExpBuffer(&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';
+ printfPQExpBuffer(&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]);
+ appendPQExpBuffer(&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)
{
@@ -3771,20 +3782,15 @@ initCreateTables(PGconn *con)
escape_tablespace = PQescapeIdentifier(con, tablespace,
strlen(tablespace));
- snprintf(opts + strlen(opts), sizeof(opts) - strlen(opts),
- " tablespace %s", escape_tablespace);
+ appendPQExpBuffer(&query, " 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);
}
+ termPQExpBuffer(&query);
+
if (partition_method != PART_NONE)
createPartitions(con);
}
@@ -3795,10 +3801,9 @@ 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(PQExpBuffer query)
{
- snprintf(opts + strlen(opts), len - strlen(opts),
- " with (fillfactor=%d)", fillfactor);
+ appendPQExpBuffer(query, " with (fillfactor=%d)", fillfactor);
}
/*
@@ -3807,10 +3812,7 @@ append_fillfactor(char *opts, int len)
static void
initGenerateData(PGconn *con)
{
- char sql[256];
- PGresult *res;
- int i;
- int64 k;
+ PQExpBufferData sql;
/* used to track elapsed time and estimate of the remaining time */
instr_time start,
@@ -3837,50 +3839,47 @@ initGenerateData(PGconn *con)
"pgbench_history, "
"pgbench_tellers");
+ initPQExpBuffer(&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);
+ printfPQExpBuffer(&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);
+ printfPQExpBuffer(&sql,
+ "insert into pgbench_tellers(tid,bid,tbalance) values (%d,%d,0)",
+ i + 1, i / ntellers + 1);
+ executeStatement(con, sql.data);
}
/*
* 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)
- {
- fprintf(stderr, "%s", PQerrorMessage(con));
- exit(1);
- }
- PQclear(res);
+ executeStatementExpect(con, "copy pgbench_accounts from stdin", PGRES_COPY_IN);
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))
+ printfPQExpBuffer(&sql,
+ INT64_FORMAT "\t" INT64_FORMAT "\t%d\t\n",
+ j, k / naccounts + 1, 0);
+ if (PQputline(con, sql.data))
{
fprintf(stderr, "PQputline failed\n");
exit(1);
@@ -3936,6 +3935,8 @@ initGenerateData(PGconn *con)
exit(1);
}
+ termPQExpBuffer(&sql);
+
executeStatement(con, "commit");
}
@@ -3963,14 +3964,15 @@ initCreatePKeys(PGconn *con)
"alter table pgbench_tellers add primary key (tid)",
"alter table pgbench_accounts add primary key (aid)"
};
- int i;
+ PQExpBufferData query;
fprintf(stderr, "creating primary keys...\n");
- for (i = 0; i < lengthof(DDLINDEXes); i++)
- {
- char buffer[256];
+ initPQExpBuffer(&query);
- strlcpy(buffer, DDLINDEXes[i], sizeof(buffer));
+ for (int i = 0; i < lengthof(DDLINDEXes); i++)
+ {
+ resetPQExpBuffer(&query);
+ appendPQExpBufferStr(&query, DDLINDEXes[i]);
if (index_tablespace != NULL)
{
@@ -3978,13 +3980,15 @@ initCreatePKeys(PGconn *con)
escape_tablespace = PQescapeIdentifier(con, index_tablespace,
strlen(index_tablespace));
- snprintf(buffer + strlen(buffer), sizeof(buffer) - strlen(buffer),
- " using index tablespace %s", escape_tablespace);
+ appendPQExpBuffer(&query,
+ " using index tablespace %s", escape_tablespace);
PQfreemem(escape_tablespace);
}
- executeStatement(con, buffer);
+ executeStatement(con, query.data);
}
+
+ termPQExpBuffer(&query);
}
/*
