Vaishnavi Prabakaran wrote:
> if (PQbatchStatus(st->con) != PQBATCH_MODE_ON)
>
> But, it is better to use if (PQbatchStatus(st->con) ==
> PQBATCH_MODE_OFF) for this verification. Reason is there is one more state
> in batch mode - PQBATCH_MODE_ABORTED. So, if the batch mode is in aborted
> status, this check will still assume that the connection is not in batch
> mode.
> In a same way, "if(PQbatchStatus(st->con) == PQBATCH_MODE_ON)" check is
> better to be modified as "PQbatchStatus(st->con) != PQBATCH_MODE_OFF".
Agreed, these two tests need to be changed to account for the
PQBATCH_MODE_ABORTED case. Fixed in new patch.
> 2. @@ -2207,7 +2227,47 @@ doCustom(TState *thread, CState *st, StatsData
> *agg)
> + if (PQbatchStatus(st->con) != PQBATCH_MODE_OFF)
> + {
> + commandFailed(st, "already in batch mode");
> + break;
> + }
>
> This is not required as below PQbatchBegin() internally does this
> verification.
>
> + PQbatchBegin(st->con);
The point of this test is to specifically forbid a sequence like this
\beginbatch
...(no \endbatch)
\beginbatch
...
and according to the doc for PQbatchBegin, it looks like the return
code won't tell us:
"Causes a connection to enter batch mode if it is currently idle or
already in batch mode.
int PQbatchBegin(PGconn *conn);
Returns 1 for success"
My understanding is that "already in batch mode" is not an error.
"Returns 0 and has no effect if the connection is not currently
idle, i.e. it has a result ready, is waiting for more input from
the server, etc. This function does not actually send anything to
the server, it just changes the libpq connection state"
> 3. It is better to check the return value of PQbatchBegin() rather than
> assuming success. E.g: PQbatchBegin() will return false if the connection
> is in copy in/out/both states.
Given point #2 above, I think both tests are needed:
if (PQbatchStatus(st->con) != PQBATCH_MODE_OFF)
{
commandFailed(st, "already in batch mode");
break;
}
if (PQbatchBegin(st->con) == 0)
{
commandFailed(st, "failed to start a batch");
break;
}
> > 3. In relation to #2, PQsendQuery() is not forbidden in batch mode
> > although I don't think it can work with it, as it's based on the old
> > protocol.
> >
> It is actually forbidden and you can see the error message "cannot
> PQsendQuery in batch mode, use PQsendQueryParams" when you use
> PQsendQuery().
Sorry for blaming the innocent piece of code, looking closer
it's pgbench that does not display the message.
With the simple query protocol, sendCommand() does essentially:
r = PQsendQuery(st->con, sql);
... other stuff...
if (r == 0)
{
if (debug)
fprintf(stderr, "client %d could not send %s\n",
st->id, command->argv[0]);
st->ecnt++;
return false;
}
So only in debug mode does it inform the user that it failed, and
even then, it does not display PQerrorMessage(st->con).
In non-debug mode it's opaque to the user. If it always fail, it appears
to loop forever. The caller has this comment that is relevant to the problem:
if (!sendCommand(st, command))
{
/*
* Failed. Stay in CSTATE_START_COMMAND state, to
* retry. ??? What the point or retrying? Should
* rather abort?
*/
return;
}
I think it doesn't bother anyone up to now because the immediate
failure modes of PQsendQuery() are resource allocation or protocol
failures that tend to never happen.
Anyway currently \beginbatch avoids the problem by checking
whether querymode == QUERY_SIMPLE, to fail gracefully
instead of letting the endless loop happen.
Best regards,
--
Daniel Vérité
PostgreSQL-powered mailer: http://www.manitou-mail.org
Twitter: @DanielVerite
diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c
index f6cb5d4..f93673e 100644
--- a/src/bin/pgbench/pgbench.c
+++ b/src/bin/pgbench/pgbench.c
@@ -269,7 +269,8 @@ typedef enum
*
* CSTATE_START_COMMAND starts the execution of a command. On a SQL
* command, the command is sent to the server, and we move to
- * CSTATE_WAIT_RESULT state. On a \sleep meta-command, the timer is
set,
+ * CSTATE_WAIT_RESULT state unless in batch mode.
+ * On a \sleep meta-command, the timer is set,
* and we enter the CSTATE_SLEEP state to wait for it to expire. Other
* meta-commands are executed immediately.
*
@@ -1882,11 +1883,24 @@ sendCommand(CState *st, Command *command)
if (commands[j]->type != SQL_COMMAND)
continue;
preparedStatementName(name, st->use_file, j);
- res = PQprepare(st->con, name,
- commands[j]->argv[0],
commands[j]->argc - 1, NULL);
- if (PQresultStatus(res) != PGRES_COMMAND_OK)
- fprintf(stderr, "%s",
PQerrorMessage(st->con));
- PQclear(res);
+ if (PQbatchStatus(st->con) == PQBATCH_MODE_OFF)
+ {
+ res = PQprepare(st->con, name,
+
commands[j]->argv[0], commands[j]->argc - 1, NULL);
+ if (PQresultStatus(res) !=
PGRES_COMMAND_OK)
+ fprintf(stderr, "%s",
PQerrorMessage(st->con));
+ PQclear(res);
+ }
+ else
+ {
+ /*
+ * In batch mode, we use asynchronous
functions. If a server-side
+ * error occurs, it will be processed
later among the other results.
+ */
+ if (!PQsendPrepare(st->con, name,
+
commands[j]->argv[0], commands[j]->argc - 1, NULL))
+ fprintf(stderr, "%s",
PQerrorMessage(st->con));
+ }
}
st->prepared[st->use_file] = true;
}
@@ -2165,7 +2179,13 @@ doCustom(TState *thread, CState *st, StatsData *agg)
return;
}
else
- st->state = CSTATE_WAIT_RESULT;
+ {
+ /* Wait for results, unless in
batch mode */
+ if (PQbatchStatus(st->con) ==
PQBATCH_MODE_OFF)
+ st->state =
CSTATE_WAIT_RESULT;
+ else
+ st->state =
CSTATE_END_COMMAND;
+ }
}
else if (command->type == META_COMMAND)
{
@@ -2207,7 +2227,51 @@ doCustom(TState *thread, CState *st, StatsData *agg)
}
else
{
- if (pg_strcasecmp(argv[0],
"set") == 0)
+ if (pg_strcasecmp(argv[0],
"beginbatch") == 0)
+ {
+ /*
+ * In batch mode, we
use a workflow based on libpq batch
+ * functions.
+ */
+ if (querymode ==
QUERY_SIMPLE)
+ {
+
commandFailed(st, "cannot use batch mode with the simple query protocol");
+ st->state =
CSTATE_ABORTED;
+ break;
+ }
+
+ if
(PQbatchStatus(st->con) != PQBATCH_MODE_OFF)
+ {
+
commandFailed(st, "already in batch mode");
+ break;
+ }
+ if
(PQbatchBegin(st->con) == 0)
+ {
+
commandFailed(st, "failed to start a batch");
+ break;
+ }
+ }
+ else if (pg_strcasecmp(argv[0],
"endbatch") == 0)
+ {
+ if
(PQbatchStatus(st->con) != PQBATCH_MODE_ON)
+ {
+
commandFailed(st, "not in batch mode");
+ break;
+ }
+ if
(!PQbatchQueueSync(st->con))
+ {
+
commandFailed(st, "failed to end the batch");
+ st->state =
CSTATE_ABORTED;
+ break;
+ }
+ if (PQbatchEnd(st->con)
== 0)
+ {
+ /* collect
pending results before getting out of batch mode */
+ st->state =
CSTATE_WAIT_RESULT;
+ break;
+ }
+ }
+ else if (pg_strcasecmp(argv[0],
"set") == 0)
{
PgBenchExpr *expr =
command->expr;
PgBenchValue result;
@@ -2279,6 +2343,7 @@ doCustom(TState *thread, CState *st, StatsData *agg)
}
break;
+
/*
* Wait for the current SQL command to complete
*/
@@ -2295,6 +2360,13 @@ doCustom(TState *thread, CState *st, StatsData *agg)
if (PQisBusy(st->con))
return; /* don't have the whole
result yet */
+ if (PQbatchStatus(st->con) == PQBATCH_MODE_ON &&
+ !PQbatchQueueProcess(st->con))
+ {
+ /* no complete result yet in batch
mode*/
+ return;
+ }
+
/*
* Read and discard the query result;
*/
@@ -2307,7 +2379,22 @@ doCustom(TState *thread, CState *st, StatsData *agg)
/* OK */
PQclear(res);
discard_response(st);
- st->state = CSTATE_END_COMMAND;
+ /*
+ * In non-batch mode, only one
result per command is expected.
+ * In batch mode, keep waiting
for results until getting
+ * PGRES_BATCH_END.
+ */
+ if (PQbatchStatus(st->con) !=
PQBATCH_MODE_ON)
+ st->state =
CSTATE_END_COMMAND;
+ break;
+ case PGRES_BATCH_END:
+ if (PQbatchEnd(st->con) == 1)
+ {
+ /* all results
collected, exit out of command and batch mode */
+ st->state =
CSTATE_END_COMMAND;
+ }
+ else
+ fprintf(stderr, "client
%d to exit batch mode", st->id);
break;
default:
commandFailed(st,
PQerrorMessage(st->con));
@@ -3173,6 +3260,13 @@ process_backslash_command(PsqlScanState sstate, const
char *source)
syntax_error(source, lineno, my_command->line,
my_command->argv[0],
"missing command", NULL, -1);
}
+ else if (pg_strcasecmp(my_command->argv[0], "beginbatch") == 0 ||
+ pg_strcasecmp(my_command->argv[0], "endbatch") == 0 )
+ {
+ if (my_command->argc > 1)
+ syntax_error(source, lineno, my_command->line,
my_command->argv[0],
+ "unexpected argument", NULL,
-1);
+ }
else
{
syntax_error(source, lineno, my_command->line,
my_command->argv[0],
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
