Re: [HACKERS] WIP Patch: Pgbench Serialization and deadlock errors

On 11-09-2018 18:29, Fabien COELHO wrote:
> Hello Marina,
> 
>> Hmm, but we can say the same for serialization or deadlock errors that 
>> were not retried (the client test code itself could not run correctly 
>> or the SQL sent was somehow wrong, which is also the client's fault), 
>> can't we?
> 
> I think not.
> 
> If a client asks for something "legal", but some other client in
> parallel happens to make an incompatible change which result in a
> serialization or deadlock error, the clients are not responsible for
> the raised errors, it is just that they happen to ask for something
> incompatible at the same time. So there is no user error per se, but
> the server is reporting its (temporary) inability to process what was
> asked for. For these errors, retrying is fine. If the client was
> alone, there would be no such errors, you cannot deadlock with
> yourself. This is really an isolation issue linked to parallel
> execution.

You can get other errors that cannot happen for only one client if you 
use shell commands in meta commands:

starting vacuum...end.
transaction type: pgbench_meta_concurrent_error.sql
scaling factor: 1
query mode: simple
number of clients: 2
number of threads: 1
number of transactions per client: 10
number of transactions actually processed: 20/20
maximum number of tries: 1
latency average = 6.953 ms
tps = 287.630161 (including connections establishing)
tps = 303.232242 (excluding connections establishing)
statement latencies in milliseconds and failures:
          1.636           0  BEGIN;
          1.497           0  \setshell var mkdir my_directory && echo 1
          0.007           0  \sleep 1 us
          1.465           0  \setshell var rmdir my_directory && echo 1
          1.622           0  END;

starting vacuum...end.
mkdir: cannot create directory ‘my_directory’: File exists
mkdir: could not read result of shell command
client 1 got an error in command 1 (setshell) of script 0; execution of 
meta-command failed
transaction type: pgbench_meta_concurrent_error.sql
scaling factor: 1
query mode: simple
number of clients: 2
number of threads: 1
number of transactions per client: 10
number of transactions actually processed: 19/20
number of failures: 1 (5.000%)
number of meta-command failures: 1 (5.000%)
maximum number of tries: 1
latency average = 11.782 ms (including failures)
tps = 161.269033 (including connections establishing)
tps = 167.733278 (excluding connections establishing)
statement latencies in milliseconds and failures:
          2.731           0  BEGIN;
          2.909           1  \setshell var mkdir my_directory && echo 1
          0.231           0  \sleep 1 us
          2.366           0  \setshell var rmdir my_directory && echo 1
          2.664           0  END;

Or if you use untrusted procedural languages in SQL expressions (see the 
used file in the attachments):

starting vacuum...ERROR:  relation "pgbench_branches" does not exist
(ignoring this error and continuing anyway)
ERROR:  relation "pgbench_tellers" does not exist
(ignoring this error and continuing anyway)
ERROR:  relation "pgbench_history" does not exist
(ignoring this error and continuing anyway)
end.
client 1 got an error in command 0 (SQL) of script 0; ERROR:  could not 
create the directory "my_directory": File exists at line 3.
CONTEXT:  PL/Perl anonymous code block

client 1 got an error in command 0 (SQL) of script 0; ERROR:  could not 
create the directory "my_directory": File exists at line 3.
CONTEXT:  PL/Perl anonymous code block

transaction type: pgbench_concurrent_error.sql
scaling factor: 1
query mode: simple
number of clients: 2
number of threads: 1
number of transactions per client: 10
number of transactions actually processed: 18/20
number of failures: 2 (10.000%)
number of serialization failures: 0 (0.000%)
number of deadlock failures: 0 (0.000%)
number of other SQL failures: 2 (10.000%)
maximum number of tries: 1
latency average = 3.282 ms (including failures)
tps = 548.437196 (including connections establishing)
tps = 637.662753 (excluding connections establishing)
statement latencies in milliseconds and failures:
          1.566           2  DO $$

starting vacuum...ERROR:  relation "pgbench_branches" does not exist
(ignoring this error and continuing anyway)
ERROR:  relation "pgbench_tellers" does not exist
(ignoring this error and continuing anyway)
ERROR:  relation "pgbench_history" does not exist
(ignoring this error and continuing anyway)
end.
transaction type: pgbench_concurrent_error.sql
scaling factor: 1
query mode: simple
number of clients: 2
number of threads: 1
number of transactions per client: 10
number of transactions actually processed: 20/20
maximum number of tries: 1
latency average = 2.760 ms
tps = 724.746078 (including connections establishing)
tps = 853.131985 (excluding connections establishing)
statement latencies in milliseconds and failures:
          1.893           0  DO $$

Or if you try to create a function and perhaps replace an existing one:

starting vacuum...end.
client 0 got an error in command 0 (SQL) of script 0; ERROR:  duplicate 
key value violates unique constraint "pg_proc_proname_args_nsp_index"
DETAIL:  Key (proname, proargtypes, pronamespace)=(my_function, , 2200) 
already exists.

client 0 got an error in command 0 (SQL) of script 0; ERROR:  tuple 
concurrently updated

client 1 got an error in command 0 (SQL) of script 0; ERROR:  tuple 
concurrently updated

client 1 got an error in command 0 (SQL) of script 0; ERROR:  tuple 
concurrently updated

client 1 got an error in command 0 (SQL) of script 0; ERROR:  tuple 
concurrently updated

client 1 got an error in command 0 (SQL) of script 0; ERROR:  tuple 
concurrently updated

client 0 got an error in command 0 (SQL) of script 0; ERROR:  tuple 
concurrently updated

client 1 got an error in command 0 (SQL) of script 0; ERROR:  tuple 
concurrently updated

client 1 got an error in command 0 (SQL) of script 0; ERROR:  tuple 
concurrently updated

client 0 got an error in command 0 (SQL) of script 0; ERROR:  tuple 
concurrently updated

transaction type: pgbench_create_function.sql
scaling factor: 1
query mode: simple
number of clients: 2
number of threads: 1
number of transactions per client: 10
number of transactions actually processed: 10/20
number of failures: 10 (50.000%)
number of serialization failures: 0 (0.000%)
number of deadlock failures: 0 (0.000%)
number of other SQL failures: 10 (50.000%)
maximum number of tries: 1
latency average = 82.881 ms (including failures)
tps = 12.065492 (including connections establishing)
tps = 12.092216 (excluding connections establishing)
statement latencies in milliseconds and failures:
         82.549          10  CREATE OR REPLACE FUNCTION my_function() 
RETURNS integer AS 'select 1;' LANGUAGE SQL;

>> Why not handle client errors that can occur (but they may also not 
>> occur) the same way? (For example, always abort the client, or 
>> conversely do not make aborts in these cases.) Here's an example of 
>> such error:
> 
>> client 5 got an error in command 1 (SQL) of script 0; ERROR:  division 
>> by zero
> 
> This is an interesting case. For me we must stop the script because
> the client is asking for something "stupid", and retrying the same
> won't change the outcome, the division will still be by zero. It is
> the client responsability not to ask for something stupid, the bench
> script is buggy, it should not submit illegal SQL queries. This is
> quite different from submitting something legal which happens to fail.
> ...
>>> I'm not sure that having "--debug" implying this option
>>> is useful: As there are two distinct options, the user may be allowed
>>> to trigger one or the other as they wish?
>> 
>> I'm not sure that the main debugging output will give a good clue of 
>> what's happened without full messages about errors, retries and 
>> failures...
> 
> I'm more argumenting about letting the user decide what they want.
> 
>> These lines are quite long - do you suggest to wrap them this way?
> 
> Sure, if it is too long, then wrap.

Ok!

>>> Function getTransactionStatus name does not seem to correspond fully 
>>> to what the function does. There is a passthru case which should be 
>>> either avoided or clearly commented.
>> 
>> I don't quite understand you - do you mean that in fact this function 
>> finds out whether we are in a (failed) transaction block or not? Or do 
>> you mean that the case of PQTRANS_INTRANS is also ok?...
> 
> The former: although the function is named "getTransactionStatus", it
> does not really return the "status" of the transaction (aka
> PQstatus()?).

Thank you, I'll think how to improve it. Perhaps the name 
checkTransactionStatus will be better...

>>> I'd insist in a comment that "cnt" does not include "skipped" 
>>> transactions
>>> (anymore).
>> 
>> If you mean CState.cnt I'm not sure if this is practically useful 
>> because the code uses only the sum of all client transactions 
>> including skipped and failed... Maybe we can rename this field to 
>> nxacts or total_cnt?
> 
> I'm fine with renaming the field if it makes thinks clearer. They are
> all counters, so naming them "cnt" or "total_cnt" does not help much.
> Maybe "succeeded" or "success" to show what is really counted?

Perhaps renaming of StatsData.cnt is better than just adding a comment 
to this field. But IMO we have the same problem (They are all counters, 
so naming them "cnt" or "total_cnt" does not help much.) for CState.cnt 
which cannot be named in the same way because it also includes skipped 
and failed transactions.

-- 
Marina Polyakova
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company