On Mon, 19 Jul 2021 10:51:36 +0900
Yugo NAGATA <[email protected]> wrote:
> Hello Fabien,
>
> On Sat, 17 Jul 2021 07:03:01 +0200 (CEST)
> Fabien COELHO <[email protected]> wrote:
>
> >
> > Hello Yugo-san,
> >
> > > [...] One way to avoid these errors is to send Parse messages before
> > > pipeline mode starts. I attached a patch to fix to prepare commands at
> > > starting of a script instead of at the first execution of the command.
> >
> > > What do you think?
> >
> > ISTM that moving prepare out of command execution is a good idea, so I'm
> > in favor of this approach: the code is simpler and cleaner.
> >
> > ISTM that a minor impact is that the preparation is not counted in the
> > command performance statistics. I do not think that it is a problem, even
> > if it would change detailed results under -C -r -M prepared.
>
> I agree with you. Currently, whether prepares are sent in pipeline mode or
> not depends on whether the first SQL command is placed between \startpipeline
> and \endpipeline regardless whether other commands are executed in pipeline
> or not. ISTM, this behavior would be not intuitive for users. Therefore,
> I think preparing all statements not using pipeline mode is not problem for
> now.
>
> If some users would like to send prepares in pipeline, I think it would be
> better to provide more simple and direct way. For example, we prepare
> statements
> in pipeline if the user use an option, or if the script has at least one
> \startpipeline in their pipeline. Maybe, --pipeline option is useful for
> users
> who want to use pipeline mode for all queries in scirpts including built-in
> ones.
> However, these features seems to be out of the patch proposed in this thread.
>
> > Patch applies & compiles cleanly, global & local make check ok. However
> > the issue is not tested. I think that the patch should add a tap test case
> > for the problem being addressed.
>
> Ok. I'll add a tap test to confirm the error I found is avoidable.
>
> > I'd suggest to move the statement preparation call in the
> > CSTATE_CHOOSE_SCRIPT case.
>
> I thought so at first, but I noticed we cannot do it at least if we are
> using -C because the connection may not be established in the
> CSTATE_CHOOSE_SCRIPT state.
>
> > In comments: not yet -> needed.
>
> Thanks. I'll fix it.
I attached the updated patch v2, which includes a comment fix and a TAP test.
Regards,
Yugo Nagata
--
Yugo NAGATA <[email protected]>
diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c
index 364b5a2e47..4e6db32fc9 100644
--- a/src/bin/pgbench/pgbench.c
+++ b/src/bin/pgbench/pgbench.c
@@ -2858,6 +2858,30 @@ chooseScript(TState *thread)
return i - 1;
}
+/* Prepare SQL commands in the chosen script */
+static void
+prepareCommands(CState *st)
+{
+ int j;
+ Command **commands = sql_script[st->use_file].commands;
+
+ for (j = 0; commands[j] != NULL; j++)
+ {
+ PGresult *res;
+ char name[MAX_PREPARE_NAME];
+
+ 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)
+ pg_log_error("%s", PQerrorMessage(st->con));
+ PQclear(res);
+ }
+ st->prepared[st->use_file] = true;
+}
+
/* Send a SQL command, using the chosen querymode */
static bool
sendCommand(CState *st, Command *command)
@@ -2891,42 +2915,6 @@ sendCommand(CState *st, Command *command)
char name[MAX_PREPARE_NAME];
const char *params[MAX_ARGS];
- if (!st->prepared[st->use_file])
- {
- int j;
- Command **commands = sql_script[st->use_file].commands;
-
- for (j = 0; commands[j] != NULL; j++)
- {
- PGresult *res;
- char name[MAX_PREPARE_NAME];
-
- if (commands[j]->type != SQL_COMMAND)
- continue;
- preparedStatementName(name, st->use_file, j);
- if (PQpipelineStatus(st->con) == PQ_PIPELINE_OFF)
- {
- res = PQprepare(st->con, name,
- commands[j]->argv[0], commands[j]->argc - 1, NULL);
- if (PQresultStatus(res) != PGRES_COMMAND_OK)
- pg_log_error("%s", PQerrorMessage(st->con));
- PQclear(res);
- }
- else
- {
- /*
- * In pipeline 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))
- pg_log_error("%s", PQerrorMessage(st->con));
- }
- }
- st->prepared[st->use_file] = true;
- }
-
getQueryParams(st, command, params);
preparedStatementName(name, st->use_file, st->command);
@@ -3194,6 +3182,11 @@ advanceConnectionState(TState *thread, CState *st, StatsData *agg)
memset(st->prepared, 0, sizeof(st->prepared));
}
+
+ /* Prepare SQL commands if needed */
+ if (querymode == QUERY_PREPARED && !st->prepared[st->use_file])
+ prepareCommands(st);
+
/* record transaction start time */
st->txn_begin = now;
diff --git a/src/bin/pgbench/t/001_pgbench_with_server.pl b/src/bin/pgbench/t/001_pgbench_with_server.pl
index 3aa9d5d753..48d1e29f16 100644
--- a/src/bin/pgbench/t/001_pgbench_with_server.pl
+++ b/src/bin/pgbench/t/001_pgbench_with_server.pl
@@ -878,6 +878,23 @@ select 1 \gset f
}
});
+# Working \startpipeline in prepared query mode with serializable
+pgbench(
+ '-t 1 -n -M prepared',
+ 0,
+ [ qr{type: .*/001_pgbench_pipeline_serializable}, qr{actually processed: 1/1} ],
+ [],
+ 'working \startpipeline with serializable',
+ {
+ '001_pgbench_pipeline_serializable' => q{
+-- test startpipeline with serializable
+\startpipeline
+BEGIN ISOLATION LEVEL SERIALIZABLE;
+} . "select 1;\n" x 10 . q{
+END;
+\endpipeline
+}
+ });
# trigger many expression errors
my @errors = (
