Skip to content

Commit 8928817

Browse files
nathan-bossartnathan-bossart
committed
Remove volatile qualifiers from pg_stat_statements.c.
Prior to commit 0709b7e, which changed the spinlock primitives to function as compiler barriers, access to variables within a spinlock-protected section required using a volatile pointer, but that is no longer necessary. Reviewed-by: Bertrand Drouvot, Michael Paquier Discussion: https://postgr.es/m/Zqkv9iK7MkNS0KaN%40nathan
1 parent 3dcb09d commit 8928817

File tree

1 file changed

+95
-134
lines changed

1 file changed

+95
-134
lines changed

‎contrib/pg_stat_statements/pg_stat_statements.c

Lines changed: 95 additions & 134 deletions
Original file line numberDiff line numberDiff line change
@@ -301,10 +301,9 @@ static bool pgss_save = true; /* whether to save stats across shutdown */
301301

302302
#define record_gc_qtexts() \
303303
do { \
304-
volatile pgssSharedState *s = (volatile pgssSharedState *) pgss; \
305-
SpinLockAcquire(&s->mutex); \
306-
s->gc_count++; \
307-
SpinLockRelease(&s->mutex); \
304+
SpinLockAcquire(&pgss->mutex); \
305+
pgss->gc_count++; \
306+
SpinLockRelease(&pgss->mutex); \
308307
} while(0)
309308

310309
/*---- Function declarations ----*/
@@ -1386,104 +1385,102 @@ pgss_store(const char *query, uint64 queryId,
13861385
/* Increment the counts, except when jstate is not NULL */
13871386
if (!jstate)
13881387
{
1388+
Assert(kind == PGSS_PLAN || kind == PGSS_EXEC);
1389+
13891390
/*
13901391
* Grab the spinlock while updating the counters (see comment about
13911392
* locking rules at the head of the file)
13921393
*/
1393-
volatile pgssEntry *e = (volatile pgssEntry *) entry;
1394-
1395-
Assert(kind == PGSS_PLAN || kind == PGSS_EXEC);
1396-
1397-
SpinLockAcquire(&e->mutex);
1394+
SpinLockAcquire(&entry->mutex);
13981395

13991396
/* "Unstick" entry if it was previously sticky */
1400-
if (IS_STICKY(e->counters))
1401-
e->counters.usage = USAGE_INIT;
1397+
if (IS_STICKY(entry->counters))
1398+
entry->counters.usage = USAGE_INIT;
14021399

1403-
e->counters.calls[kind] += 1;
1404-
e->counters.total_time[kind] += total_time;
1400+
entry->counters.calls[kind] += 1;
1401+
entry->counters.total_time[kind] += total_time;
14051402

1406-
if (e->counters.calls[kind] == 1)
1403+
if (entry->counters.calls[kind] == 1)
14071404
{
1408-
e->counters.min_time[kind] = total_time;
1409-
e->counters.max_time[kind] = total_time;
1410-
e->counters.mean_time[kind] = total_time;
1405+
entry->counters.min_time[kind] = total_time;
1406+
entry->counters.max_time[kind] = total_time;
1407+
entry->counters.mean_time[kind] = total_time;
14111408
}
14121409
else
14131410
{
14141411
/*
14151412
* Welford's method for accurately computing variance. See
14161413
* <http://www.johndcook.com/blog/standard_deviation/>
14171414
*/
1418-
double old_mean = e->counters.mean_time[kind];
1415+
double old_mean = entry->counters.mean_time[kind];
14191416

1420-
e->counters.mean_time[kind] +=
1421-
(total_time - old_mean) / e->counters.calls[kind];
1422-
e->counters.sum_var_time[kind] +=
1423-
(total_time - old_mean) * (total_time - e->counters.mean_time[kind]);
1417+
entry->counters.mean_time[kind] +=
1418+
(total_time - old_mean) / entry->counters.calls[kind];
1419+
entry->counters.sum_var_time[kind] +=
1420+
(total_time - old_mean) * (total_time - entry->counters.mean_time[kind]);
14241421

14251422
/*
14261423
* Calculate min and max time. min = 0 and max = 0 means that the
14271424
* min/max statistics were reset
14281425
*/
1429-
if (e->counters.min_time[kind] == 0
1430-
&& e->counters.max_time[kind] == 0)
1426+
if (entry->counters.min_time[kind] == 0
1427+
&& entry->counters.max_time[kind] == 0)
14311428
{
1432-
e->counters.min_time[kind] = total_time;
1433-
e->counters.max_time[kind] = total_time;
1429+
entry->counters.min_time[kind] = total_time;
1430+
entry->counters.max_time[kind] = total_time;
14341431
}
14351432
else
14361433
{
1437-
if (e->counters.min_time[kind] > total_time)
1438-
e->counters.min_time[kind] = total_time;
1439-
if (e->counters.max_time[kind] < total_time)
1440-
e->counters.max_time[kind] = total_time;
1434+
if (entry->counters.min_time[kind] > total_time)
1435+
entry->counters.min_time[kind] = total_time;
1436+
if (entry->counters.max_time[kind] < total_time)
1437+
entry->counters.max_time[kind] = total_time;
14411438
}
14421439
}
1443-
e->counters.rows += rows;
1444-
e->counters.shared_blks_hit += bufusage->shared_blks_hit;
1445-
e->counters.shared_blks_read += bufusage->shared_blks_read;
1446-
e->counters.shared_blks_dirtied += bufusage->shared_blks_dirtied;
1447-
e->counters.shared_blks_written += bufusage->shared_blks_written;
1448-
e->counters.local_blks_hit += bufusage->local_blks_hit;
1449-
e->counters.local_blks_read += bufusage->local_blks_read;
1450-
e->counters.local_blks_dirtied += bufusage->local_blks_dirtied;
1451-
e->counters.local_blks_written += bufusage->local_blks_written;
1452-
e->counters.temp_blks_read += bufusage->temp_blks_read;
1453-
e->counters.temp_blks_written += bufusage->temp_blks_written;
1454-
e->counters.shared_blk_read_time += INSTR_TIME_GET_MILLISEC(bufusage->shared_blk_read_time);
1455-
e->counters.shared_blk_write_time += INSTR_TIME_GET_MILLISEC(bufusage->shared_blk_write_time);
1456-
e->counters.local_blk_read_time += INSTR_TIME_GET_MILLISEC(bufusage->local_blk_read_time);
1457-
e->counters.local_blk_write_time += INSTR_TIME_GET_MILLISEC(bufusage->local_blk_write_time);
1458-
e->counters.temp_blk_read_time += INSTR_TIME_GET_MILLISEC(bufusage->temp_blk_read_time);
1459-
e->counters.temp_blk_write_time += INSTR_TIME_GET_MILLISEC(bufusage->temp_blk_write_time);
1460-
e->counters.usage += USAGE_EXEC(total_time);
1461-
e->counters.wal_records += walusage->wal_records;
1462-
e->counters.wal_fpi += walusage->wal_fpi;
1463-
e->counters.wal_bytes += walusage->wal_bytes;
1440+
entry->counters.rows += rows;
1441+
entry->counters.shared_blks_hit += bufusage->shared_blks_hit;
1442+
entry->counters.shared_blks_read += bufusage->shared_blks_read;
1443+
entry->counters.shared_blks_dirtied += bufusage->shared_blks_dirtied;
1444+
entry->counters.shared_blks_written += bufusage->shared_blks_written;
1445+
entry->counters.local_blks_hit += bufusage->local_blks_hit;
1446+
entry->counters.local_blks_read += bufusage->local_blks_read;
1447+
entry->counters.local_blks_dirtied += bufusage->local_blks_dirtied;
1448+
entry->counters.local_blks_written += bufusage->local_blks_written;
1449+
entry->counters.temp_blks_read += bufusage->temp_blks_read;
1450+
entry->counters.temp_blks_written += bufusage->temp_blks_written;
1451+
entry->counters.shared_blk_read_time += INSTR_TIME_GET_MILLISEC(bufusage->shared_blk_read_time);
1452+
entry->counters.shared_blk_write_time += INSTR_TIME_GET_MILLISEC(bufusage->shared_blk_write_time);
1453+
entry->counters.local_blk_read_time += INSTR_TIME_GET_MILLISEC(bufusage->local_blk_read_time);
1454+
entry->counters.local_blk_write_time += INSTR_TIME_GET_MILLISEC(bufusage->local_blk_write_time);
1455+
entry->counters.temp_blk_read_time += INSTR_TIME_GET_MILLISEC(bufusage->temp_blk_read_time);
1456+
entry->counters.temp_blk_write_time += INSTR_TIME_GET_MILLISEC(bufusage->temp_blk_write_time);
1457+
entry->counters.usage += USAGE_EXEC(total_time);
1458+
entry->counters.wal_records += walusage->wal_records;
1459+
entry->counters.wal_fpi += walusage->wal_fpi;
1460+
entry->counters.wal_bytes += walusage->wal_bytes;
14641461
if (jitusage)
14651462
{
1466-
e->counters.jit_functions += jitusage->created_functions;
1467-
e->counters.jit_generation_time += INSTR_TIME_GET_MILLISEC(jitusage->generation_counter);
1463+
entry->counters.jit_functions += jitusage->created_functions;
1464+
entry->counters.jit_generation_time += INSTR_TIME_GET_MILLISEC(jitusage->generation_counter);
14681465

14691466
if (INSTR_TIME_GET_MILLISEC(jitusage->deform_counter))
1470-
e->counters.jit_deform_count++;
1471-
e->counters.jit_deform_time += INSTR_TIME_GET_MILLISEC(jitusage->deform_counter);
1467+
entry->counters.jit_deform_count++;
1468+
entry->counters.jit_deform_time += INSTR_TIME_GET_MILLISEC(jitusage->deform_counter);
14721469

14731470
if (INSTR_TIME_GET_MILLISEC(jitusage->inlining_counter))
1474-
e->counters.jit_inlining_count++;
1475-
e->counters.jit_inlining_time += INSTR_TIME_GET_MILLISEC(jitusage->inlining_counter);
1471+
entry->counters.jit_inlining_count++;
1472+
entry->counters.jit_inlining_time += INSTR_TIME_GET_MILLISEC(jitusage->inlining_counter);
14761473

14771474
if (INSTR_TIME_GET_MILLISEC(jitusage->optimization_counter))
1478-
e->counters.jit_optimization_count++;
1479-
e->counters.jit_optimization_time += INSTR_TIME_GET_MILLISEC(jitusage->optimization_counter);
1475+
entry->counters.jit_optimization_count++;
1476+
entry->counters.jit_optimization_time += INSTR_TIME_GET_MILLISEC(jitusage->optimization_counter);
14801477

14811478
if (INSTR_TIME_GET_MILLISEC(jitusage->emission_counter))
1482-
e->counters.jit_emission_count++;
1483-
e->counters.jit_emission_time += INSTR_TIME_GET_MILLISEC(jitusage->emission_counter);
1479+
entry->counters.jit_emission_count++;
1480+
entry->counters.jit_emission_time += INSTR_TIME_GET_MILLISEC(jitusage->emission_counter);
14841481
}
14851482

1486-
SpinLockRelease(&e->mutex);
1483+
SpinLockRelease(&entry->mutex);
14871484
}
14881485

14891486
done:
@@ -1724,15 +1721,11 @@ pg_stat_statements_internal(FunctionCallInfo fcinfo,
17241721
int n_writers;
17251722

17261723
/* Take the mutex so we can examine variables */
1727-
{
1728-
volatile pgssSharedState *s = (volatile pgssSharedState *) pgss;
1729-
1730-
SpinLockAcquire(&s->mutex);
1731-
extent = s->extent;
1732-
n_writers = s->n_writers;
1733-
gc_count = s->gc_count;
1734-
SpinLockRelease(&s->mutex);
1735-
}
1724+
SpinLockAcquire(&pgss->mutex);
1725+
extent = pgss->extent;
1726+
n_writers = pgss->n_writers;
1727+
gc_count = pgss->gc_count;
1728+
SpinLockRelease(&pgss->mutex);
17361729

17371730
/* No point in loading file now if there are active writers */
17381731
if (n_writers == 0)
@@ -1847,15 +1840,11 @@ pg_stat_statements_internal(FunctionCallInfo fcinfo,
18471840
}
18481841

18491842
/* copy counters to a local variable to keep locking time short */
1850-
{
1851-
volatile pgssEntry *e = (volatile pgssEntry *) entry;
1852-
1853-
SpinLockAcquire(&e->mutex);
1854-
tmp = e->counters;
1855-
stats_since = e->stats_since;
1856-
minmax_stats_since = e->minmax_stats_since;
1857-
SpinLockRelease(&e->mutex);
1858-
}
1843+
SpinLockAcquire(&entry->mutex);
1844+
tmp = entry->counters;
1845+
stats_since = entry->stats_since;
1846+
minmax_stats_since = entry->minmax_stats_since;
1847+
SpinLockRelease(&entry->mutex);
18591848

18601849
/* Skip entry if unexecuted (ie, it's a pending "sticky" entry) */
18611850
if (IS_STICKY(tmp))
@@ -1996,13 +1985,9 @@ pg_stat_statements_info(PG_FUNCTION_ARGS)
19961985
elog(ERROR, "return type must be a row type");
19971986

19981987
/* Read global statistics for pg_stat_statements */
1999-
{
2000-
volatile pgssSharedState *s = (volatile pgssSharedState *) pgss;
2001-
2002-
SpinLockAcquire(&s->mutex);
2003-
stats = s->stats;
2004-
SpinLockRelease(&s->mutex);
2005-
}
1988+
SpinLockAcquire(&pgss->mutex);
1989+
stats = pgss->stats;
1990+
SpinLockRelease(&pgss->mutex);
20061991

20071992
values[0] = Int64GetDatum(stats.dealloc);
20081993
values[1] = TimestampTzGetDatum(stats.stats_reset);
@@ -2169,13 +2154,9 @@ entry_dealloc(void)
21692154
pfree(entries);
21702155

21712156
/* Increment the number of times entries are deallocated */
2172-
{
2173-
volatile pgssSharedState *s = (volatile pgssSharedState *) pgss;
2174-
2175-
SpinLockAcquire(&s->mutex);
2176-
s->stats.dealloc += 1;
2177-
SpinLockRelease(&s->mutex);
2178-
}
2157+
SpinLockAcquire(&pgss->mutex);
2158+
pgss->stats.dealloc += 1;
2159+
SpinLockRelease(&pgss->mutex);
21792160
}
21802161

21812162
/*
@@ -2205,17 +2186,13 @@ qtext_store(const char *query, int query_len,
22052186
* We use a spinlock to protect extent/n_writers/gc_count, so that
22062187
* multiple processes may execute this function concurrently.
22072188
*/
2208-
{
2209-
volatile pgssSharedState *s = (volatile pgssSharedState *) pgss;
2210-
2211-
SpinLockAcquire(&s->mutex);
2212-
off = s->extent;
2213-
s->extent += query_len + 1;
2214-
s->n_writers++;
2215-
if (gc_count)
2216-
*gc_count = s->gc_count;
2217-
SpinLockRelease(&s->mutex);
2218-
}
2189+
SpinLockAcquire(&pgss->mutex);
2190+
off = pgss->extent;
2191+
pgss->extent += query_len + 1;
2192+
pgss->n_writers++;
2193+
if (gc_count)
2194+
*gc_count = pgss->gc_count;
2195+
SpinLockRelease(&pgss->mutex);
22192196

22202197
*query_offset = off;
22212198

@@ -2244,13 +2221,9 @@ qtext_store(const char *query, int query_len,
22442221
CloseTransientFile(fd);
22452222

22462223
/* Mark our write complete */
2247-
{
2248-
volatile pgssSharedState *s = (volatile pgssSharedState *) pgss;
2249-
2250-
SpinLockAcquire(&s->mutex);
2251-
s->n_writers--;
2252-
SpinLockRelease(&s->mutex);
2253-
}
2224+
SpinLockAcquire(&pgss->mutex);
2225+
pgss->n_writers--;
2226+
SpinLockRelease(&pgss->mutex);
22542227

22552228
return true;
22562229

@@ -2264,13 +2237,9 @@ qtext_store(const char *query, int query_len,
22642237
CloseTransientFile(fd);
22652238

22662239
/* Mark our write complete */
2267-
{
2268-
volatile pgssSharedState *s = (volatile pgssSharedState *) pgss;
2269-
2270-
SpinLockAcquire(&s->mutex);
2271-
s->n_writers--;
2272-
SpinLockRelease(&s->mutex);
2273-
}
2240+
SpinLockAcquire(&pgss->mutex);
2241+
pgss->n_writers--;
2242+
SpinLockRelease(&pgss->mutex);
22742243

22752244
return false;
22762245
}
@@ -2408,13 +2377,9 @@ need_gc_qtexts(void)
24082377
Size extent;
24092378

24102379
/* Read shared extent pointer */
2411-
{
2412-
volatile pgssSharedState *s = (volatile pgssSharedState *) pgss;
2413-
2414-
SpinLockAcquire(&s->mutex);
2415-
extent = s->extent;
2416-
SpinLockRelease(&s->mutex);
2417-
}
2380+
SpinLockAcquire(&pgss->mutex);
2381+
extent = pgss->extent;
2382+
SpinLockRelease(&pgss->mutex);
24182383

24192384
/*
24202385
* Don't proceed if file does not exceed 512 bytes per possible entry.
@@ -2733,14 +2698,10 @@ entry_reset(Oid userid, Oid dbid, uint64 queryid, bool minmax_only)
27332698
* Reset global statistics for pg_stat_statements since all entries are
27342699
* removed.
27352700
*/
2736-
{
2737-
volatile pgssSharedState *s = (volatile pgssSharedState *) pgss;
2738-
2739-
SpinLockAcquire(&s->mutex);
2740-
s->stats.dealloc = 0;
2741-
s->stats.stats_reset = stats_reset;
2742-
SpinLockRelease(&s->mutex);
2743-
}
2701+
SpinLockAcquire(&pgss->mutex);
2702+
pgss->stats.dealloc = 0;
2703+
pgss->stats.stats_reset = stats_reset;
2704+
SpinLockRelease(&pgss->mutex);
27442705

27452706
/*
27462707
* Write new empty query file, perhaps even creating a new one to recover

0 commit comments

Comments
 (0)