Hello Jeevan,
I haven't read the complete patch. But, I have noticed that many
places you changed the variable declaration from c to c++ style (i.e
moved the declaration in the for loop). IMHO, generally in PG, we
don't follow this convention. Is there any specific reason to do
this?
+1.
As I said, this C99 feature is already used extensively in pg sources, so
it makes sense to use it when refactoring something and if appropriate,
which IMO is the case here.
The patch does not apply on master, needs rebase.
Hmmm. "git apply pgbench-buffer-1.patch" works for me on current master.
Also, I got some whitespace errors.
It possible, but I cannot see any. Could you be more specific?
Many mailers do not conform to MIME and mess-up newlines when attachements
are typed text/*, because MIME requires the mailer to convert those to
crnl eol when sending and back to system eol when receiving, but few
actually do it. Maybe the issue is really there.
I think you can also refactor the function tryExecuteStatement(), and
call your newly added function executeStatementExpect() by passing
an additional flag something like "errorOK".
Indeed, good point.
--
Fabien.
diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c
index e72ad0036e..ef99016c23 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);
@@ -1137,34 +1137,36 @@ accumStats(StatsData *stats, bool skipped, double lat, double lag)
}
}
-/* call PQexec() and exit() on failure */
+/* call PQexec() and possibly exit() on failure */
+static void
+executeStatementExpect(PGconn *con, const char *sql, const ExecStatusType expected, bool errorOK)
+{
+ PGresult *res;
+
+ res = PQexec(con, sql);
+ if (PQresultStatus(res) != expected)
+ {
+ fprintf(stderr, "%s", PQerrorMessage(con));
+ if (errorOK)
+ fprintf(stderr, "(ignoring this error and continuing anyway)\n");
+ else
+ 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, 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)
- {
- fprintf(stderr, "%s", PQerrorMessage(con));
- fprintf(stderr, "(ignoring this error and continuing anyway)\n");
- }
- PQclear(res);
+ executeStatementExpect(con, sql, PGRES_COMMAND_OK, true);
}
/* set up a connection to the backend */
@@ -3631,30 +3633,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 +3661,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 +3746,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 +3777,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 +3796,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 +3807,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 +3834,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, false);
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 +3930,8 @@ initGenerateData(PGconn *con)
exit(1);
}
+ termPQExpBuffer(&sql);
+
executeStatement(con, "commit");
}
@@ -3963,14 +3959,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 +3975,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);
}
/*
