2013-03-18 06:47 keltezéssel, Tom Lane írta:
Boszormenyi Zoltan <z...@cybertec.at> writes:
The volatile marking shouldn't even be necessary there.
The signal handler doesn't writes to it, only the main code.
Well, (a) that's not the case in the patch as committed, and (b) even if
it were true, the volatile marking is still *necessary*, because that's
what tells the compiler it can't optimize away changes to the variable,
say on the grounds of there being another store with no intervening
fetches in the main-line code. Without that, a compiler that had
aggressively inlined the smaller functions could well deduce that the
disable_alarm() assignment was useless.
Also, since the the variable assignment doesn't depend on other code
in the same function (or vice-versa) the compiler is still free to
reorder it.
Volatile is about not caching the variable in a CPU register since
it's "volatile"...
I don't believe you understand what volatile is about at all. Go read
the C standard: it's about requiring objects' values to actually match
the nominal program-specified values at sequence points. A more
accurate heuristic is that volatile tells the compiler there may be
other forces reading or writing the variable besides the ones it can see
in the current function's source code, and so it can't drop or reorder
accesses to the variable.
regards, tom lane
After reading up on a lot of volatile and memory barriers,
I am still not totally convinced.
For the record, sig_atomit_t is a plain int without any special
treatment from the compiler. It's atomic by nature on every 32-bit
and 64-bit CPU.
How about the attached patch over current GIT? In other words,
why I am wrong with this idea?
The problem that may arise if it's wrong is that transactions are
left waiting for the lock when the interrupt triggers and the variable
is still seen as false from the interrupt handler. But this doesn't happen.
FWIW, this small patch seems to give a 0,7 percent performance
boost for my settings:
shared_buffers = 256MB
work_mem = 8MB
effective_io_concurrency = 8
wal_level = hot_standby
checkpoint_segments = 64
random_page_cost = 1.8
Everything else is the default. I tested the patch on a 8-core
AMD FX-8120 CPU with this pgbench script below. Basically, it's
the default transaction prepended with "SET lock_timeout = 1;"
I have used the attached quick-and-dirty patch to pgbench to
ignore SQL errors coming from statements. "-s 100" was used
to initialize the test database.
\set nbranches 1 * :scale
\set ntellers 10 * :scale
\set naccounts 100000 * :scale
\setrandom aid 1 :naccounts
\setrandom bid 1 :nbranches
\setrandom tid 1 :ntellers
\setrandom delta -5000 5000
BEGIN;
SET lock_timeout = 1;
UPDATE pgbench_accounts SET abalance = abalance + :delta WHERE aid = :aid;
SELECT abalance FROM pgbench_accounts WHERE aid = :aid;
UPDATE pgbench_tellers SET tbalance = tbalance + :delta WHERE tid = :tid;
UPDATE pgbench_branches SET bbalance = bbalance + :delta WHERE bid = :bid;
INSERT INTO pgbench_history (tid, bid, aid, delta, mtime) VALUES (:tid, :bid, :aid,
:delta, CURRENT_TIMESTAMP);
END;
Results of "pgbench -c 32 -j 32 -t 10000 -e -f script.sql" are
for the GIT version:
tps = 3366.844023 (including connections establishing)
tps = 3367.645454 (excluding connections establishing)
tps = 3379.784707 (including connections establishing)
tps = 3380.622317 (excluding connections establishing)
tps = 3385.198238 (including connections establishing)
tps = 3386.132433 (excluding connections establishing)
and with the barrier patch:
tps = 3412.799044 (including connections establishing)
tps = 3413.670832 (excluding connections establishing)
tps = 3389.796287 (including connections establishing)
tps = 3390.602187 (excluding connections establishing)
tps = 3405.924548 (including connections establishing)
tps = 3406.726997 (excluding connections establishing)
Best regards,
Zoltán Böszörményi
--
----------------------------------
Zoltán Böszörményi
Cybertec Schönig & Schönig GmbH
Gröhrmühlgasse 26
A-2700 Wiener Neustadt, Austria
Web: http://www.postgresql-support.de
http://www.postgresql.at/
diff --git a/src/backend/utils/misc/timeout.c b/src/backend/utils/misc/timeout.c
index 3c3e41e..8737a86 100644
--- a/src/backend/utils/misc/timeout.c
+++ b/src/backend/utils/misc/timeout.c
@@ -16,6 +16,7 @@
#include <sys/time.h>
+#include "storage/barrier.h"
#include "storage/proc.h"
#include "utils/timeout.h"
#include "utils/timestamp.h"
@@ -58,10 +59,10 @@ static timeout_params *volatile active_timeouts[MAX_TIMEOUTS];
* since the probability of the interrupt actually occurring while we have
* it disabled is low. See comments in schedule_alarm() about that.
*/
-static volatile sig_atomic_t alarm_enabled = false;
+static sig_atomic_t alarm_enabled = false;
-#define disable_alarm() (alarm_enabled = false)
-#define enable_alarm() (alarm_enabled = true)
+#define disable_alarm() do { alarm_enabled = false; pg_write_barrier(); } while (0)
+#define enable_alarm() do { alarm_enabled = true; pg_write_barrier(); } while (0)
/*****************************************************************************
diff --git a/contrib/pgbench/pgbench.c b/contrib/pgbench/pgbench.c
index bc01f07..dc8e592 100644
--- a/contrib/pgbench/pgbench.c
+++ b/contrib/pgbench/pgbench.c
@@ -162,6 +162,7 @@ char *index_tablespace = NULL;
bool use_log; /* log transaction latencies to a file */
bool use_quiet; /* quiet logging onto stderr */
+bool ignore_errors; /* ignore SQL errors from transactions */
int agg_interval; /* log aggregates instead of individual transactions */
bool is_connect; /* establish connection for each transaction */
bool is_latencies; /* report per-command latencies */
@@ -345,6 +346,7 @@ usage(void)
" -C establish new connection for each transaction\n"
" -D VARNAME=VALUE\n"
" define variable for use by custom script\n"
+ " -e ignore errors from transactions\n"
" -f FILENAME read transaction script from FILENAME\n"
" -j NUM number of threads (default: 1)\n"
" -l write transaction times to log file\n"
@@ -1040,6 +1042,8 @@ top:
case PGRES_TUPLES_OK:
break; /* OK */
default:
+ if (ignore_errors)
+ break;
fprintf(stderr, "Client %d aborted in state %d: %s",
st->id, st->state, PQerrorMessage(st->con));
PQclear(res);
@@ -2152,7 +2156,7 @@ main(int argc, char **argv)
state = (CState *) pg_malloc(sizeof(CState));
memset(state, 0, sizeof(CState));
- while ((c = getopt_long(argc, argv, "ih:nvp:dqSNc:j:Crs:t:T:U:lf:D:F:M:", long_options, &optindex)) != -1)
+ while ((c = getopt_long(argc, argv, "ih:nvp:dqSNc:j:Crs:t:T:U:lef:D:F:M:", long_options, &optindex)) != -1)
{
switch (c)
{
@@ -2263,6 +2267,9 @@ main(int argc, char **argv)
case 'q':
use_quiet = true;
break;
+ case 'e':
+ ignore_errors = true;
+ break;
case 'f':
ttype = 3;
filename = pg_strdup(optarg);
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
