public inbox for [email protected]help / color / mirror / Atom feed
[PATCH v3 1/2] Improve functionality, docs and tests of -R, --no-ensure-shutdown and --dry-run options 14+ messages / 8 participants [nested] [flat]
* [PATCH v3 1/2] Improve functionality, docs and tests of -R, --no-ensure-shutdown and --dry-run options @ 2019-10-04 14:14 Alexey Kondratov <[email protected]> 0 siblings, 0 replies; 14+ messages in thread From: Alexey Kondratov @ 2019-10-04 14:14 UTC (permalink / raw) Branch: pg-rewind-fixes --- doc/src/sgml/ref/pg_rewind.sgml | 10 +-- src/bin/pg_rewind/pg_rewind.c | 19 +++--- src/bin/pg_rewind/t/001_basic.pl | 2 +- src/bin/pg_rewind/t/002_databases.pl | 2 +- src/bin/pg_rewind/t/003_extrafiles.pl | 2 +- src/bin/pg_rewind/t/004_pg_xlog_symlink.pl | 2 +- src/bin/pg_rewind/t/005_same_timeline.pl | 32 +++++++--- src/bin/pg_rewind/t/RewindTest.pm | 71 +++++++++++++++++----- 8 files changed, 103 insertions(+), 37 deletions(-) diff --git a/doc/src/sgml/ref/pg_rewind.sgml b/doc/src/sgml/ref/pg_rewind.sgml index fbf454803b..42d29edd4e 100644 --- a/doc/src/sgml/ref/pg_rewind.sgml +++ b/doc/src/sgml/ref/pg_rewind.sgml @@ -169,12 +169,14 @@ PostgreSQL documentation <term><option>--no-ensure-shutdown</option></term> <listitem> <para> - <application>pg_rewind</application> verifies that the target server - is cleanly shutdown before rewinding; by default, if it isn't, it - starts the server in single-user mode to complete crash recovery. + <application>pg_rewind</application> requires that the target server + is cleanly shut down before rewinding. By default, if the target server + is not shut down cleanly, <application>pg_rewind</application> starts + the target server in single-user mode to complete crash recovery first, + and stops it. By passing this option, <application>pg_rewind</application> skips this and errors out immediately if the server is not cleanly shut - down. Users are expected to handle the situation themselves in that + down. Users are expected to handle the situation themselves in that case. </para> </listitem> diff --git a/src/bin/pg_rewind/pg_rewind.c b/src/bin/pg_rewind/pg_rewind.c index fe1468b771..875a43b219 100644 --- a/src/bin/pg_rewind/pg_rewind.c +++ b/src/bin/pg_rewind/pg_rewind.c @@ -270,11 +270,12 @@ main(int argc, char **argv) pg_free(buffer); /* - * If the target instance was not cleanly shut down, run a single-user - * postgres session really quickly and reload the control file to get the - * new state. Note if no_ensure_shutdown is specified, pg_rewind won't do - * that automatically. That means users need to do themselves in advance, - * else pg_rewind will soon quit, see sanityChecks(). + * If the target instance was not cleanly shut down, start and stop the + * target cluster once in single-user mode to enforce recovery to finish, + * ensuring that the cluster can be used by pg_rewind. Note that if + * no_ensure_shutdown is specified, pg_rewind ignores this step, and users + * need to make sure by themselves that the target cluster is in a clean + * state. */ if (!no_ensure_shutdown && ControlFile_target.state != DB_SHUTDOWNED && @@ -847,8 +848,12 @@ ensureCleanShutdown(const char *argv0) if (dry_run) return; - /* finally run postgres in single-user mode */ - snprintf(cmd, MAXCMDLEN, "\"%s\" --single -D \"%s\" template1 < \"%s\"", + /* + * Finally run postgres in single-user mode. There is no need to use + * fsync here. This makes the recovery faster, and the target data folder + * is synced at the end anyway. + */ + snprintf(cmd, MAXCMDLEN, "\"%s\" --single -F -D \"%s\" template1 < \"%s\"", exec_path, datadir_target, DEVNULL); if (system(cmd) != 0) diff --git a/src/bin/pg_rewind/t/001_basic.pl b/src/bin/pg_rewind/t/001_basic.pl index c3293e93df..a1659460ec 100644 --- a/src/bin/pg_rewind/t/001_basic.pl +++ b/src/bin/pg_rewind/t/001_basic.pl @@ -1,7 +1,7 @@ use strict; use warnings; use TestLib; -use Test::More tests => 11; +use Test::More tests => 14; use FindBin; use lib $FindBin::RealBin; diff --git a/src/bin/pg_rewind/t/002_databases.pl b/src/bin/pg_rewind/t/002_databases.pl index 1db534c0dc..921c4434f5 100644 --- a/src/bin/pg_rewind/t/002_databases.pl +++ b/src/bin/pg_rewind/t/002_databases.pl @@ -1,7 +1,7 @@ use strict; use warnings; use TestLib; -use Test::More tests => 7; +use Test::More tests => 10; use FindBin; use lib $FindBin::RealBin; diff --git a/src/bin/pg_rewind/t/003_extrafiles.pl b/src/bin/pg_rewind/t/003_extrafiles.pl index f4710440fc..bce5b47148 100644 --- a/src/bin/pg_rewind/t/003_extrafiles.pl +++ b/src/bin/pg_rewind/t/003_extrafiles.pl @@ -3,7 +3,7 @@ use strict; use warnings; use TestLib; -use Test::More tests => 5; +use Test::More tests => 8; use File::Find; diff --git a/src/bin/pg_rewind/t/004_pg_xlog_symlink.pl b/src/bin/pg_rewind/t/004_pg_xlog_symlink.pl index 639eeb9c91..a501be8f78 100644 --- a/src/bin/pg_rewind/t/004_pg_xlog_symlink.pl +++ b/src/bin/pg_rewind/t/004_pg_xlog_symlink.pl @@ -14,7 +14,7 @@ if ($windows_os) } else { - plan tests => 5; + plan tests => 8; } use FindBin; diff --git a/src/bin/pg_rewind/t/005_same_timeline.pl b/src/bin/pg_rewind/t/005_same_timeline.pl index 40dbc44caa..1d96740a55 100644 --- a/src/bin/pg_rewind/t/005_same_timeline.pl +++ b/src/bin/pg_rewind/t/005_same_timeline.pl @@ -1,19 +1,35 @@ +# +# Test that running pg_rewind with the source and target clusters +# on the same timeline runs successfully. +# use strict; use warnings; use TestLib; -use Test::More tests => 1; +use Test::More tests => 6; use FindBin; use lib $FindBin::RealBin; use RewindTest; -# Test that running pg_rewind if the two clusters are on the same -# timeline runs successfully. +sub run_test +{ + my $test_mode = shift; + + RewindTest::setup_cluster($test_mode, ['-g']); + RewindTest::start_master(); + RewindTest::create_standby($test_mode); + + RewindTest::promote_standby(); + + RewindTest::run_pg_rewind($test_mode); + + RewindTest::clean_rewind_test(); + return; +} + +# Run the test in both modes. +run_test('local'); +run_test('remote'); -RewindTest::setup_cluster(); -RewindTest::start_master(); -RewindTest::create_standby(); -RewindTest::run_pg_rewind('local'); -RewindTest::clean_rewind_test(); exit(0); diff --git a/src/bin/pg_rewind/t/RewindTest.pm b/src/bin/pg_rewind/t/RewindTest.pm index c540722420..9292092b1a 100644 --- a/src/bin/pg_rewind/t/RewindTest.pm +++ b/src/bin/pg_rewind/t/RewindTest.pm @@ -227,9 +227,57 @@ sub run_pg_rewind # Append the rewind-specific role to the connection string. $standby_connstr = "$standby_connstr user=rewind_user"; - # Stop the master and be ready to perform the rewind + # Stop the master cleanly to check dry-run mode. $node_master->stop; + # First run pg_rewind in dry-run mode + if ($test_mode eq "local") + { + + # Stop the new master and be ready to perform the rewind + # Do rewind in dry-run mode using a local pgdata as source + $node_standby->stop; + command_ok( + [ + 'pg_rewind', + "--debug", "--no-sync", "--dry-run", + "--source-pgdata=$standby_pgdata", + "--target-pgdata=$master_pgdata", + "--no-ensure-shutdown" + ], + 'pg_rewind local with --dry-run'); + } + elsif ($test_mode eq "remote") + { + + # Do rewind in dry-run mode using a remote connection as source + command_ok( + [ + 'pg_rewind', + "--debug", "--no-sync", "--dry-run", + "--source-server", $standby_connstr, + "--target-pgdata=$master_pgdata", + "--write-recovery-conf", "--progress" + ], + 'pg_rewind remote with --dry-run'); + + # Check that standby.signal hasn't been created. + ok(! -e "$master_pgdata/standby.signal", + 'standby.signal is absent'); + } + else + { + + # Cannot come here normally + croak("Incorrect test mode specified"); + } + + # Start and stop the master and be ready to perform the rewind. + # The cluster needs recovery to finish once, and pg_rewind makes + # sure that it happens automatically. + $node_master->start; + $node_master->stop('immediate'); + # At this point, the rewind processing is ready to run. # We now have a very simple scenario with a few diverged WAL record. # The real testing begins really now with a bifurcation of the possible @@ -246,8 +294,6 @@ sub run_pg_rewind { # Do rewind using a local pgdata as source - # Stop the master and be ready to perform the rewind - $node_standby->stop; command_ok( [ 'pg_rewind', @@ -261,18 +307,21 @@ sub run_pg_rewind elsif ($test_mode eq "remote") { - # Do rewind using a remote connection as source + # Do rewind using a remote connection as source, generating + # recovery configuration automatically. command_ok( [ 'pg_rewind', "--debug", "--source-server", $standby_connstr, - "--target-pgdata=$master_pgdata", "-R", - "--no-sync" + "--target-pgdata=$master_pgdata", "--no-sync", + "--write-recovery-conf" ], 'pg_rewind remote'); - # Check that standby.signal has been created. - ok(-e "$master_pgdata/standby.signal"); + # Check that standby.signal is here as recovery configuration + # was requested. + ok( -e "$master_pgdata/standby.signal", + 'standby.signal created after pg_rewind'); # Now, when pg_rewind apparently succeeded with minimal permissions, # add REPLICATION privilege. So we could test that new standby @@ -280,12 +329,6 @@ sub run_pg_rewind $node_standby->safe_psql('postgres', "ALTER ROLE rewind_user WITH REPLICATION;"); } - else - { - - # Cannot come here normally - croak("Incorrect test mode specified"); - } # Now move back postgresql.conf with old settings move( base-commit: 6837632b758e0470a2692d9f8303e8aebd6fbd8f -- 2.17.1 --------------4FB4DA99007DA85F944316F9 Content-Type: text/x-patch; name="v3-0002-WIP-increase-pg_rewind-test-coverage.patch" Content-Transfer-Encoding: 7bit Content-Disposition: attachment; filename*0="v3-0002-WIP-increase-pg_rewind-test-coverage.patch" ^ permalink raw reply [nested|flat] 14+ messages in thread
* pgbench: using prepared BEGIN statement in a pipeline could cause an error @ 2021-07-16 06:30 Yugo NAGATA <[email protected]> 0 siblings, 1 reply; 14+ messages in thread From: Yugo NAGATA @ 2021-07-16 06:30 UTC (permalink / raw) To: pgsql-hackers Hello, I found that using "BEGIN ISOLATINO LEVEL SERIALIZABLE" in a pipline with prepared statement makes pgbench abort. $ cat pipeline.sql \startpipeline begin isolation level repeatable read; select 1; end; \endpipeline $ pgbench -f pipeline.sql -M prepared -t 1 pgbench (15devel) starting vacuum...end. pgbench: error: client 0 script 0 aborted in command 4 query 0: transaction type: pipeline.sql scaling factor: 1 query mode: prepared number of clients: 1 number of threads: 1 number of transactions per client: 1 number of transactions actually processed: 0/1 pgbench: fatal: Run was aborted; the above results are incomplete. The error that occured in the backend was "ERROR: SET TRANSACTION ISOLATION LEVEL must be called before any query". After investigating this, now I've got the cause as below. 1. The commands in the script are executed in the order. First, pipeline mode starts at \startpipeline. 2. Parse messages for all SQL commands in the script are sent to the backend because it is first time to execute them. 3. An implicit transaction starts, and this is not committed yet because Sync message is not sent at that time in pipeline mode. 4. All prepared statements are sent to the backend. 5. After processing \endpipeline, Sync is issued and all sent commands are executed. 6. However, the BEGIN doesn't start new transaction because the implicit transaction has already started. The error above occurs because the snapshot was already created before the BEGIN command. We can also see the similar error when using "BEGIN DEFERRABLE". 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. Or, we can also avoid these errors by placing \startpipeline after the BEGIN, so it might be enogh just to note in the documentation. Actually, we also get an error just when there is another SQL command before the BEGIN in a pipelne, as below, regardless to using prepared statement or not, because this command cause an implicit transaction. \startpipeline select 0; begin isolation level repeatable read; select 1; end; \endpipeline I think it is hard to prevent this error from pgbench without analysing command strings. Therefore, noting in the documentation that the first command in a pipeline starts an implicit transaction might be useful for users. What do you think? Regards, Yugo Nagata -- Yugo NAGATA <[email protected]> Attachments: [text/x-diff] pgbench-prepare.patch (2.5K, ../../[email protected]/2-pgbench-prepare.patch) download | inline diff: diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c index 364b5a2e47..56e790fa33 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 not yet */ + if (querymode == QUERY_PREPARED && !st->prepared[st->use_file]) + prepareCommands(st); + /* record transaction start time */ st->txn_begin = now; ^ permalink raw reply [nested|flat] 14+ messages in thread
* Re: pgbench: using prepared BEGIN statement in a pipeline could cause an error @ 2021-07-17 05:03 Fabien COELHO <[email protected]> parent: Yugo NAGATA <[email protected]> 0 siblings, 2 replies; 14+ messages in thread From: Fabien COELHO @ 2021-07-17 05:03 UTC (permalink / raw) To: Yugo NAGATA <[email protected]>; +Cc: pgsql-hackers 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. 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. I'd suggest to move the statement preparation call in the CSTATE_CHOOSE_SCRIPT case. In comments: not yet -> needed. -- Fabien. ^ permalink raw reply [nested|flat] 14+ messages in thread
* Re: pgbench: using prepared BEGIN statement in a pipeline could cause an error @ 2021-07-19 01:51 Yugo NAGATA <[email protected]> parent: Fabien COELHO <[email protected]> 1 sibling, 1 reply; 14+ messages in thread From: Yugo NAGATA @ 2021-07-19 01:51 UTC (permalink / raw) To: Fabien COELHO <[email protected]>; +Cc: pgsql-hackers 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. Regards, Yugo Nagata -- Yugo NAGATA <[email protected]> ^ permalink raw reply [nested|flat] 14+ messages in thread
* Re: pgbench: using prepared BEGIN statement in a pipeline could cause an error @ 2021-07-21 01:49 Yugo NAGATA <[email protected]> parent: Yugo NAGATA <[email protected]> 0 siblings, 1 reply; 14+ messages in thread From: Yugo NAGATA @ 2021-07-21 01:49 UTC (permalink / raw) To: Yugo NAGATA <[email protected]>; +Cc: Fabien COELHO <[email protected]>; pgsql-hackers 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]> Attachments: [text/x-diff] v2-pgbench-prepare.patch (3.3K, ../../[email protected]/2-v2-pgbench-prepare.patch) download | inline diff: 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 = ( ^ permalink raw reply [nested|flat] 14+ messages in thread
* Re: pgbench: using prepared BEGIN statement in a pipeline could cause an error @ 2021-11-15 13:13 Daniel Gustafsson <[email protected]> parent: Yugo NAGATA <[email protected]> 0 siblings, 1 reply; 14+ messages in thread From: Daniel Gustafsson @ 2021-11-15 13:13 UTC (permalink / raw) To: Yugo NAGATA <[email protected]>; +Cc: Fabien COELHO <[email protected]>; pgsql-hackers > On 21 Jul 2021, at 03:49, Yugo NAGATA <[email protected]> wrote: > I attached the updated patch v2, which includes a comment fix and a TAP test. This patch fails the TAP test for pgbench: # Tests were run but no plan was declared and done_testing() was not seen. # Looks like your test exited with 25 just after 224. t/001_pgbench_with_server.pl .. Dubious, test returned 25 (wstat 6400, 0x1900) All 224 subtests passed t/002_pgbench_no_server.pl .... ok Test Summary Report ------------------- t/001_pgbench_with_server.pl (Wstat: 6400 Tests: 224 Failed: 0) Non-zero exit status: 25 Parse errors: No plan found in TAP output Files=2, Tests=426, 3 wallclock secs ( 0.04 usr 0.00 sys + 1.20 cusr 0.36 csys = 1.60 CPU) Result: FAIL -- Daniel Gustafsson https://vmware.com/ ^ permalink raw reply [nested|flat] 14+ messages in thread
* Re: pgbench: using prepared BEGIN statement in a pipeline could cause an error @ 2021-11-15 17:26 Yugo NAGATA <[email protected]> parent: Daniel Gustafsson <[email protected]> 0 siblings, 0 replies; 14+ messages in thread From: Yugo NAGATA @ 2021-11-15 17:26 UTC (permalink / raw) To: Daniel Gustafsson <[email protected]>; +Cc: Fabien COELHO <[email protected]>; pgsql-hackers Hello Daniel Gustafsson, On Mon, 15 Nov 2021 14:13:32 +0100 Daniel Gustafsson <[email protected]> wrote: > > On 21 Jul 2021, at 03:49, Yugo NAGATA <[email protected]> wrote: > > > I attached the updated patch v2, which includes a comment fix and a TAP test. > > This patch fails the TAP test for pgbench: Thank you for pointing it out! I attached the updated patch. Regards, Yugo Nagata > # Tests were run but no plan was declared and done_testing() was not seen. > # Looks like your test exited with 25 just after 224. > t/001_pgbench_with_server.pl .. > Dubious, test returned 25 (wstat 6400, 0x1900) > All 224 subtests passed > t/002_pgbench_no_server.pl .... ok > Test Summary Report > ------------------- > t/001_pgbench_with_server.pl (Wstat: 6400 Tests: 224 Failed: 0) > Non-zero exit status: 25 > Parse errors: No plan found in TAP output > Files=2, Tests=426, 3 wallclock secs ( 0.04 usr 0.00 sys + 1.20 cusr 0.36 csys = 1.60 CPU) > Result: FAIL > > -- > Daniel Gustafsson https://vmware.com/ > -- Yugo NAGATA <[email protected]> Attachments: [text/x-diff] v3-pgbench-prepare.patch (3.3K, ../../[email protected]/2-v3-pgbench-prepare.patch) download | inline diff: diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c index c12b6f0615..c7c3963600 100644 --- a/src/bin/pgbench/pgbench.c +++ b/src/bin/pgbench/pgbench.c @@ -2859,6 +2859,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) @@ -2892,42 +2916,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); @@ -3199,6 +3187,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 69ffa595dd..7b7a8518d1 100644 --- a/src/bin/pgbench/t/001_pgbench_with_server.pl +++ b/src/bin/pgbench/t/001_pgbench_with_server.pl @@ -830,6 +830,23 @@ select 1 \gset f } }); +# Working \startpipeline in prepared query mode with serializable +$node->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 = ( ^ permalink raw reply [nested|flat] 14+ messages in thread
* Re: pgbench: using prepared BEGIN statement in a pipeline could cause an error @ 2022-03-25 20:19 Tom Lane <[email protected]> parent: Fabien COELHO <[email protected]> 1 sibling, 1 reply; 14+ messages in thread From: Tom Lane @ 2022-03-25 20:19 UTC (permalink / raw) To: Fabien COELHO <[email protected]>; +Cc: Yugo NAGATA <[email protected]>; pgsql-hackers Fabien COELHO <[email protected]> writes: >> [...] 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. > 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 am not convinced this is a great idea. The current behavior is that a statement is not prepared until it's about to be executed, and I think we chose that deliberately to avoid semantic differences between prepared and not-prepared mode. For example, if a script looks like CREATE FUNCTION foo(...) ...; SELECT foo(...); DROP FUNCTION foo; trying to prepare the SELECT in advance would lead to failure. We could perhaps get away with preparing the commands within a pipeline just before we start to execute the pipeline, but it looks to me like this patch tries to prepare the entire script in advance. BTW, the cfbot says the patch is failing to apply anyway ... I think it was sideswiped by 4a39f87ac. regards, tom lane ^ permalink raw reply [nested|flat] 14+ messages in thread
* Re: pgbench: using prepared BEGIN statement in a pipeline could cause an error @ 2022-03-28 03:33 Yugo NAGATA <[email protected]> parent: Tom Lane <[email protected]> 0 siblings, 2 replies; 14+ messages in thread From: Yugo NAGATA @ 2022-03-28 03:33 UTC (permalink / raw) To: Tom Lane <[email protected]>; +Cc: Fabien COELHO <[email protected]>; pgsql-hackers On Fri, 25 Mar 2022 16:19:54 -0400 Tom Lane <[email protected]> wrote: > Fabien COELHO <[email protected]> writes: > >> [...] 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. > > > 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 am not convinced this is a great idea. The current behavior is that > a statement is not prepared until it's about to be executed, and I think > we chose that deliberately to avoid semantic differences between prepared > and not-prepared mode. For example, if a script looks like > > CREATE FUNCTION foo(...) ...; > SELECT foo(...); > DROP FUNCTION foo; > > trying to prepare the SELECT in advance would lead to failure. > > We could perhaps get away with preparing the commands within a pipeline > just before we start to execute the pipeline, but it looks to me like > this patch tries to prepare the entire script in advance. > Well, the semantic differences is already in the current behavior. Currently, pgbench fails to execute the above script in prepared mode because it prepares the entire script in advance just before the first command execution. This patch does not change the semantic. > BTW, the cfbot says the patch is failing to apply anyway ... > I think it was sideswiped by 4a39f87ac. I attached the rebased patch. Regards, Yugo Nagata -- Yugo NAGATA <[email protected]> Attachments: [text/x-diff] v5-pgbench-prepare.patch (3.9K, ../../[email protected]/2-v5-pgbench-prepare.patch) download | inline diff: diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c index acf3e56413..bf8fecf219 100644 --- a/src/bin/pgbench/pgbench.c +++ b/src/bin/pgbench/pgbench.c @@ -3074,6 +3074,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) @@ -3107,42 +3131,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->variables, command, params); preparedStatementName(name, st->use_file, st->command); @@ -3619,6 +3607,20 @@ advanceConnectionState(TState *thread, CState *st, StatsData *agg) memset(st->prepared, 0, sizeof(st->prepared)); } + /* + * Prepare SQL commands if needed. + * + * We should send Parse messages before executing meta commands + * especially /startpipeline. If a Parse message is sent in + * pipeline mode, a transaction starts before BEGIN is sent, and + * it could be a problem. For example, "BEGIN ISOLATION LEVEL + * SERIALIZABLE" is sent after a transaction starts, the error + * "ERROR: SET TRANSACTION ISOLATION LEVEL must be called before any query" + * occurs. + */ + if (querymode == QUERY_PREPARED && !st->prepared[st->use_file]) + prepareCommands(st); + /* * It is the first try to run this transaction. Remember the * random state: maybe it will get an error and we will need to diff --git a/src/bin/pgbench/t/001_pgbench_with_server.pl b/src/bin/pgbench/t/001_pgbench_with_server.pl index ca71f968dc..bcd8abe739 100644 --- a/src/bin/pgbench/t/001_pgbench_with_server.pl +++ b/src/bin/pgbench/t/001_pgbench_with_server.pl @@ -839,6 +839,23 @@ select 1 \gset f } }); +# Working \startpipeline in prepared query mode with serializable +$node->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 = ( ^ permalink raw reply [nested|flat] 14+ messages in thread
* Re: pgbench: using prepared BEGIN statement in a pipeline could cause an error @ 2022-09-03 05:36 Ibrar Ahmed <[email protected]> parent: Yugo NAGATA <[email protected]> 1 sibling, 1 reply; 14+ messages in thread From: Ibrar Ahmed @ 2022-09-03 05:36 UTC (permalink / raw) To: Kyotaro Horiguchi <[email protected]>; Fabien COELHO <[email protected]>; Daniel Gustafsson <[email protected]>; +Cc: pgsql-hackers; Yugo NAGATA <[email protected]> On Mon, Mar 28, 2022 at 8:35 AM Yugo NAGATA <[email protected]> wrote: > On Fri, 25 Mar 2022 16:19:54 -0400 > Tom Lane <[email protected]> wrote: > > > Fabien COELHO <[email protected]> writes: > > >> [...] 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. > > > > > 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 am not convinced this is a great idea. The current behavior is that > > a statement is not prepared until it's about to be executed, and I think > > we chose that deliberately to avoid semantic differences between prepared > > and not-prepared mode. For example, if a script looks like > > > > CREATE FUNCTION foo(...) ...; > > SELECT foo(...); > > DROP FUNCTION foo; > > > > trying to prepare the SELECT in advance would lead to failure. > > > > We could perhaps get away with preparing the commands within a pipeline > > just before we start to execute the pipeline, but it looks to me like > > this patch tries to prepare the entire script in advance. > > > Well, the semantic differences is already in the current behavior. > Currently, pgbench fails to execute the above script in prepared mode > because it prepares the entire script in advance just before the first > command execution. This patch does not change the semantic. > > > BTW, the cfbot says the patch is failing to apply anyway ... > > I think it was sideswiped by 4a39f87ac. > > I attached the rebased patch. > > Regards, > Yugo Nagata > > -- > Yugo NAGATA <[email protected]> > Hi Kyotaro Horiguchi, Fabien Coelho, Daniel Gustafsson, Since you haven't had time to write a review the last many days, the author replied with a rebased patch for a long time and never heard. We've taken your name off the reviewer list for this patch. Of course, you are still welcome to review it if you can find the time. We're removing your name so that other reviewers know the patch still needs attention. We understand that day jobs and other things get in the way of doing patch reviews when you want to, so please come back and review a patch or two later when you have more time. -- Ibrar Ahmed ^ permalink raw reply [nested|flat] 14+ messages in thread
* Re: pgbench: using prepared BEGIN statement in a pipeline could cause an error @ 2022-09-03 07:09 Julien Rouhaud <[email protected]> parent: Ibrar Ahmed <[email protected]> 0 siblings, 1 reply; 14+ messages in thread From: Julien Rouhaud @ 2022-09-03 07:09 UTC (permalink / raw) To: Ibrar Ahmed <[email protected]>; +Cc: Kyotaro Horiguchi <[email protected]>; Fabien COELHO <[email protected]>; Daniel Gustafsson <[email protected]>; pgsql-hackers; Yugo NAGATA <[email protected]> Hi, On Sat, Sep 03, 2022 at 10:36:37AM +0500, Ibrar Ahmed wrote: > > Hi Kyotaro Horiguchi, Fabien Coelho, Daniel Gustafsson, > > Since you haven't had time to write a review the last many days, the author > replied > with a rebased patch for a long time and never heard. We've taken your name > off the reviewer list for this patch. Of course, you are still welcome to > review it if you can > find the time. We're removing your name so that other reviewers know the > patch still needs > attention. We understand that day jobs and other things get in the way of > doing patch > reviews when you want to, so please come back and review a patch or two > later when you > have more time. I thought that we decided not to remove assigned reviewers from a CF entry, even if they didn't reply recently? See the discussion around https://www.postgresql.org/message-id/CA%2BTgmoZSBNhX0zCkG5T5KiQize9Aq4%2Bec%2BuqLcfBhm_%2B12MbQA%40... ^ permalink raw reply [nested|flat] 14+ messages in thread
* Re: pgbench: using prepared BEGIN statement in a pipeline could cause an error @ 2022-09-04 07:56 Ibrar Ahmed <[email protected]> parent: Julien Rouhaud <[email protected]> 0 siblings, 0 replies; 14+ messages in thread From: Ibrar Ahmed @ 2022-09-04 07:56 UTC (permalink / raw) To: Julien Rouhaud <[email protected]>; +Cc: Kyotaro Horiguchi <[email protected]>; Fabien COELHO <[email protected]>; Daniel Gustafsson <[email protected]>; pgsql-hackers; Yugo NAGATA <[email protected]> On Sat, Sep 3, 2022 at 12:09 PM Julien Rouhaud <[email protected]> wrote: > Hi, > > On Sat, Sep 03, 2022 at 10:36:37AM +0500, Ibrar Ahmed wrote: > > > > Hi Kyotaro Horiguchi, Fabien Coelho, Daniel Gustafsson, > > > > Since you haven't had time to write a review the last many days, the > author > > replied > > with a rebased patch for a long time and never heard. We've taken your > name > > off the reviewer list for this patch. Of course, you are still welcome to > > review it if you can > > find the time. We're removing your name so that other reviewers know the > > patch still needs > > attention. We understand that day jobs and other things get in the way of > > doing patch > > reviews when you want to, so please come back and review a patch or two > > later when you > > have more time. > > I thought that we decided not to remove assigned reviewers from a CF entry, > even if they didn't reply recently? See the discussion around > > https://www.postgresql.org/message-id/CA%2BTgmoZSBNhX0zCkG5T5KiQize9Aq4%2Bec%2BuqLcfBhm_%2B12MbQA%40... > Ah, ok, thanks for the clarification. I will add them back. @Jacob Champion, we need to update the CommitFest Checklist [1] document accordingly. *"Reviewer Clear [reviewer name]:* * Since you haven't had time to write a review of [patch] in the last 5 days, we've taken your name off the reviewer list for this patch."* [1] https://wiki.postgresql.org/wiki/CommitFest_Checklist -- Ibrar Ahmed ^ permalink raw reply [nested|flat] 14+ messages in thread
* Re: pgbench: using prepared BEGIN statement in a pipeline could cause an error @ 2022-09-12 15:03 Alvaro Herrera <[email protected]> parent: Yugo NAGATA <[email protected]> 1 sibling, 1 reply; 14+ messages in thread From: Alvaro Herrera @ 2022-09-12 15:03 UTC (permalink / raw) To: Yugo NAGATA <[email protected]>; +Cc: Tom Lane <[email protected]>; Fabien COELHO <[email protected]>; pgsql-hackers On 2022-Mar-28, Yugo NAGATA wrote: > On Fri, 25 Mar 2022 16:19:54 -0400 > Tom Lane <[email protected]> wrote: > > I am not convinced this is a great idea. The current behavior is that > > a statement is not prepared until it's about to be executed, and I think > > we chose that deliberately to avoid semantic differences between prepared > > and not-prepared mode. For example, if a script looks like > > > > CREATE FUNCTION foo(...) ...; > > SELECT foo(...); > > DROP FUNCTION foo; > > > > trying to prepare the SELECT in advance would lead to failure. > > > > We could perhaps get away with preparing the commands within a pipeline > > just before we start to execute the pipeline, but it looks to me like > > this patch tries to prepare the entire script in advance. Maybe it would work to have one extra boolean in struct Command, indicating that the i-th command in the script is inside a pipeline; in -M prepared, issue PREPARE for each command marked with that flag ahead of time, and for all other commands, do as today. That way, we don't change behavior for anything except those commands that need the change. -- Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/ "Digital and video cameras have this adjustment and film cameras don't for the same reason dogs and cats lick themselves: because they can." (Ken Rockwell) ^ permalink raw reply [nested|flat] 14+ messages in thread
* Re: pgbench: using prepared BEGIN statement in a pipeline could cause an error @ 2022-09-30 01:07 Yugo NAGATA <[email protected]> parent: Alvaro Herrera <[email protected]> 0 siblings, 0 replies; 14+ messages in thread From: Yugo NAGATA @ 2022-09-30 01:07 UTC (permalink / raw) To: Alvaro Herrera <[email protected]>; +Cc: Tom Lane <[email protected]>; Fabien COELHO <[email protected]>; pgsql-hackers Hi, On Mon, 12 Sep 2022 17:03:43 +0200 Alvaro Herrera <[email protected]> wrote: > On 2022-Mar-28, Yugo NAGATA wrote: > > > On Fri, 25 Mar 2022 16:19:54 -0400 > > Tom Lane <[email protected]> wrote: > > > > I am not convinced this is a great idea. The current behavior is that > > > a statement is not prepared until it's about to be executed, and I think > > > we chose that deliberately to avoid semantic differences between prepared > > > and not-prepared mode. For example, if a script looks like > > > > > > CREATE FUNCTION foo(...) ...; > > > SELECT foo(...); > > > DROP FUNCTION foo; > > > > > > trying to prepare the SELECT in advance would lead to failure. > > > > > > We could perhaps get away with preparing the commands within a pipeline > > > just before we start to execute the pipeline, but it looks to me like > > > this patch tries to prepare the entire script in advance. > > Maybe it would work to have one extra boolean in struct Command, indicating > that the i-th command in the script is inside a pipeline; in -M > prepared, issue PREPARE for each command marked with that flag ahead of > time, and for all other commands, do as today. That way, we don't > change behavior for anything except those commands that need the change. Well, I still don't understand why we need to prepare only "the commands within a pipeline" before starting pipeline. In the current behavior, the entire script is prepared in advance just before executing the first SQL command in the script, instead of preparing each command one by one. This patch also prepare the entire script in advance, so there is no behavioural change in this sense. However, there are a few behavioural changes. One is that the preparation is not counted in the command performance statistics as Fabien mentioned. Another is that all meta-commands including \shell and \sleep etc. are executed before the preparation. To reduce impact of these changes, I updated the patch to prepare the commands just before executing the first SQL command or \startpipeline meta-command instead of at the beginning of the script. Regards, Yugo Nagata > > -- > Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/ > "Digital and video cameras have this adjustment and film cameras don't for the > same reason dogs and cats lick themselves: because they can." (Ken Rockwell) -- Yugo NAGATA <[email protected]> Attachments: [text/x-diff] v6-pgbench-prepare.patch (3.7K, ../../[email protected]/2-v6-pgbench-prepare.patch) download | inline diff: diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c index aa1a3541fe..12a268fe84 100644 --- a/src/bin/pgbench/pgbench.c +++ b/src/bin/pgbench/pgbench.c @@ -3070,6 +3070,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) @@ -3104,39 +3128,7 @@ sendCommand(CState *st, Command *command) 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; - - 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; - } + prepareCommands(st); getQueryParams(&st->variables, command, params); preparedStatementName(name, st->use_file, st->command); @@ -4376,6 +4368,19 @@ executeMetaCommand(CState *st, pg_time_usec_t *now) commandFailed(st, "startpipeline", "cannot use pipeline mode with the simple query protocol"); return CSTATE_ABORTED; } + /* + * Prepare SQL commands if needed. + * + * We should send Parse messages before executing /startpipeline. + * If a Parse message is sent in pipeline mode, a transaction starts + * before BEGIN is sent, and it could be a problem. For example, if + * "BEGIN ISOLATION LEVEL SERIALIZABLE" is sent after a transaction + * starts, the error + * "ERROR: SET TRANSACTION ISOLATION LEVEL must be called before any query" + * occurs. + */ + else if (querymode == QUERY_PREPARED && !st->prepared[st->use_file]) + prepareCommands(st); if (PQpipelineStatus(st->con) != PQ_PIPELINE_OFF) { diff --git a/src/bin/pgbench/t/001_pgbench_with_server.pl b/src/bin/pgbench/t/001_pgbench_with_server.pl index 2c0dc36965..e70a7d966c 100644 --- a/src/bin/pgbench/t/001_pgbench_with_server.pl +++ b/src/bin/pgbench/t/001_pgbench_with_server.pl @@ -839,6 +839,23 @@ select 1 \gset f } }); +# Working \startpipeline in prepared query mode with serializable +$node->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 = ( ^ permalink raw reply [nested|flat] 14+ messages in thread
end of thread, other threads:[~2022-09-30 01:07 UTC | newest] Thread overview: 14+ messages (download: mbox mbox.gz follow: Atom feed) -- links below jump to the message on this page -- 2019-10-04 14:14 [PATCH v3 1/2] Improve functionality, docs and tests of -R, --no-ensure-shutdown and --dry-run options Alexey Kondratov <[email protected]> 2021-07-16 06:30 pgbench: using prepared BEGIN statement in a pipeline could cause an error Yugo NAGATA <[email protected]> 2021-07-17 05:03 ` Re: pgbench: using prepared BEGIN statement in a pipeline could cause an error Fabien COELHO <[email protected]> 2021-07-19 01:51 ` Re: pgbench: using prepared BEGIN statement in a pipeline could cause an error Yugo NAGATA <[email protected]> 2021-07-21 01:49 ` Re: pgbench: using prepared BEGIN statement in a pipeline could cause an error Yugo NAGATA <[email protected]> 2021-11-15 13:13 ` Re: pgbench: using prepared BEGIN statement in a pipeline could cause an error Daniel Gustafsson <[email protected]> 2021-11-15 17:26 ` Re: pgbench: using prepared BEGIN statement in a pipeline could cause an error Yugo NAGATA <[email protected]> 2022-03-25 20:19 ` Re: pgbench: using prepared BEGIN statement in a pipeline could cause an error Tom Lane <[email protected]> 2022-03-28 03:33 ` Re: pgbench: using prepared BEGIN statement in a pipeline could cause an error Yugo NAGATA <[email protected]> 2022-09-03 05:36 ` Re: pgbench: using prepared BEGIN statement in a pipeline could cause an error Ibrar Ahmed <[email protected]> 2022-09-03 07:09 ` Re: pgbench: using prepared BEGIN statement in a pipeline could cause an error Julien Rouhaud <[email protected]> 2022-09-04 07:56 ` Re: pgbench: using prepared BEGIN statement in a pipeline could cause an error Ibrar Ahmed <[email protected]> 2022-09-12 15:03 ` Re: pgbench: using prepared BEGIN statement in a pipeline could cause an error Alvaro Herrera <[email protected]> 2022-09-30 01:07 ` Re: pgbench: using prepared BEGIN statement in a pipeline could cause an error Yugo NAGATA <[email protected]>
This inbox is served by agora; see mirroring instructions for how to clone and mirror all data and code used for this inbox