Skip to content

Commit dc5f905

Browse files
michaelpqmichaelpq
committed
Fix invalidation of local pgstats references for entry reinitialization
818119a has introduced the "generation" concept in pgstats entries, incremented a counter when a pgstats entry is reinitialized, but it did not count on the fact that backends still holding local references to such entries need to be refreshed if the cache age is outdated. The previous logic only updated local references when an entry was dropped, but it needs also to consider entries that are reinitialized. This matters for replication slot stats (as well as custom pgstats kinds in 18~), where concurrent drops and creates of a slot could cause incorrect stats to be locally referenced. This would lead to an assertion failure at shutdown when writing out the stats file, as the backend holding an outdated local reference would not be able to drop during its shutdown sequence the stats entry that should be dropped, as the last process holding a reference to the stats entry. The checkpointer was then complaining about such an entry late in the shutdown sequence, after the shutdown checkpoint is finished with the control file updated, causing the stats file to not be generated. In non-assert builds, the entry would just be skipped with the stats file written. Note that only logical replication slots use statistics. A test case based on TAP is added to test_decoding, where a persistent connection peeking at a slot's data is kept with concurrent drops and creates of the same slot. This is based on the isolation test case that Anton has sent. As it requires a node shutdown with a check to make sure that the stats file is written with this specific sequence of events, TAP is used instead. Reported-by: Anton A. Melnikov Reviewed-by: Bertrand Drouvot Discussion: https://postgr.es/m/56bf8ff9-dd8c-47b2-872a-748ede82af99@postgrespro.ru Backpatch-through: 15
1 parent 9d5ce4f commit dc5f905

File tree

2 files changed

+69
-2
lines changed

2 files changed

+69
-2
lines changed

‎contrib/test_decoding/t/001_repl_stats.pl

+60
Original file line numberDiff line numberDiff line change
@@ -118,4 +118,64 @@ sub test_slot_stats
118118
# shutdown
119119
$node->stop;
120120

121+
# Test replication slot stats persistence in a single session. The slot
122+
# is dropped and created concurrently of a session peeking at its data
123+
# repeatedly, hence holding in its local cache a reference to the stats.
124+
$node->start;
125+
126+
my $slot_name_restart = 'regression_slot5';
127+
$node->safe_psql('postgres',
128+
"SELECT pg_create_logical_replication_slot('$slot_name_restart', 'test_decoding');"
129+
);
130+
131+
# Look at slot data, with a persistent connection.
132+
my $bpgsql = $node->background_psql('postgres', on_error_stop => 1);
133+
134+
# Launch query and look at slot data, incrementing the refcount of the
135+
# stats entry.
136+
$bpgsql->query_safe(
137+
"SELECT pg_logical_slot_peek_binary_changes('$slot_name_restart', NULL, NULL)"
138+
);
139+
140+
# Drop the slot entry. The stats entry is not dropped yet as the previous
141+
# session still holds a reference to it.
142+
$node->safe_psql('postgres',
143+
"SELECT pg_drop_replication_slot('$slot_name_restart')");
144+
145+
# Create again the same slot. The stats entry is reinitialized, not marked
146+
# as dropped anymore.
147+
$node->safe_psql('postgres',
148+
"SELECT pg_create_logical_replication_slot('$slot_name_restart', 'test_decoding');"
149+
);
150+
151+
# Look again at the slot data. The local stats reference should be refreshed
152+
# to the reinitialized entry.
153+
$bpgsql->query_safe(
154+
"SELECT pg_logical_slot_peek_binary_changes('$slot_name_restart', NULL, NULL)"
155+
);
156+
# Drop again the slot, the entry is not dropped yet as the previous session
157+
# still has a refcount on it.
158+
$node->safe_psql('postgres',
159+
"SELECT pg_drop_replication_slot('$slot_name_restart')");
160+
161+
# Shutdown the node, which should happen cleanly with the stats file written
162+
# to disk. Note that the background session created previously needs to be
163+
# hold *while* the node is shutting down to check that it drops the stats
164+
# entry of the slot before writing the stats file.
165+
$node->stop;
166+
167+
# Make sure that the node is correctly shut down. Checking the control file
168+
# is not enough, as the node may detect that something is incorrect after the
169+
# control file has been updated and the shutdown checkpoint is finished, so
170+
# also check that the stats file has been written out.
171+
command_like(
172+
[ 'pg_controldata', $node->data_dir ],
173+
qr/Database cluster state:\s+shut down\n/,
174+
'node shut down ok');
175+
176+
my $stats_file = "$datadir/pg_stat/pgstat.stat";
177+
ok(-f "$stats_file", "stats file must exist after shutdown");
178+
179+
$bpgsql->quit;
180+
121181
done_testing();

‎src/backend/utils/activity/pgstat_shmem.c

+9-2
Original file line numberDiff line numberDiff line change
@@ -703,7 +703,8 @@ pgstat_gc_entry_refs(void)
703703
Assert(curage != 0);
704704

705705
/*
706-
* Some entries have been dropped. Invalidate cache pointer to them.
706+
* Some entries have been dropped or reinitialized. Invalidate cache
707+
* pointer to them.
707708
*/
708709
pgstat_entry_ref_hash_start_iterate(pgStatEntryRefHash, &i);
709710
while ((ent = pgstat_entry_ref_hash_iterate(pgStatEntryRefHash, &i)) != NULL)
@@ -713,7 +714,13 @@ pgstat_gc_entry_refs(void)
713714
Assert(!entry_ref->shared_stats ||
714715
entry_ref->shared_stats->magic == 0xdeadbeef);
715716

716-
if (!entry_ref->shared_entry->dropped)
717+
/*
718+
* "generation" checks for the case of entries being reinitialized,
719+
* and "dropped" for the case where these are.. dropped.
720+
*/
721+
if (!entry_ref->shared_entry->dropped &&
722+
pg_atomic_read_u32(&entry_ref->shared_entry->generation) ==
723+
entry_ref->generation)
717724
continue;
718725

719726
/* cannot gc shared ref that has pending data */

0 commit comments

Comments
 (0)