Author: Noah Misch Commit: Noah Misch Add an injection_points isolation test suite. Make the isolation harness recognize injection_points wait events as a type of blocked state. To simplify that, change that wait event naming scheme to INJECTION_POINT(name). Add an initial test for an extant inplace-update bug. Reviewed by FIXME. Discussion: https://postgr.es/m/FIXME diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.c index 4be0dee..4eda445 100644 --- a/src/backend/access/heap/heapam.c +++ b/src/backend/access/heap/heapam.c @@ -63,6 +63,7 @@ #include "storage/procarray.h" #include "storage/standby.h" #include "utils/datum.h" +#include "utils/injection_point.h" #include "utils/inval.h" #include "utils/relcache.h" #include "utils/snapmgr.h" @@ -6077,6 +6078,7 @@ heap_inplace_update(Relation relation, HeapTuple tuple) (errcode(ERRCODE_INVALID_TRANSACTION_STATE), errmsg("cannot update tuples during a parallel operation"))); + INJECTION_POINT("inplace-before-pin"); buffer = ReadBuffer(relation, ItemPointerGetBlockNumber(&(tuple->t_self))); LockBuffer(buffer, BUFFER_LOCK_EXCLUSIVE); page = (Page) BufferGetPage(buffer); diff --git a/src/backend/utils/adt/waitfuncs.c b/src/backend/utils/adt/waitfuncs.c index d9c92c3..b524a8a 100644 --- a/src/backend/utils/adt/waitfuncs.c +++ b/src/backend/utils/adt/waitfuncs.c @@ -14,8 +14,13 @@ #include "catalog/pg_type.h" #include "storage/predicate_internals.h" +#include "storage/proc.h" +#include "storage/procarray.h" #include "utils/array.h" #include "utils/builtins.h" +#include "utils/wait_event.h" + +#define UINT32_ACCESS_ONCE(var) ((uint32)(*((volatile uint32 *)&(var)))) /* @@ -23,8 +28,9 @@ * * Check if specified PID is blocked by any of the PIDs listed in the second * argument. Currently, this looks for blocking caused by waiting for - * heavyweight locks or safe snapshots. We ignore blockage caused by PIDs - * not directly under the isolationtester's control, eg autovacuum. + * injection points, heavyweight locks, or safe snapshots. We ignore blockage + * caused by PIDs not directly under the isolationtester's control, eg + * autovacuum. * * This is an undocumented function intended for use by the isolation tester, * and may change in future releases as required for testing purposes. @@ -34,6 +40,8 @@ pg_isolation_test_session_is_blocked(PG_FUNCTION_ARGS) { int blocked_pid = PG_GETARG_INT32(0); ArrayType *interesting_pids_a = PG_GETARG_ARRAYTYPE_P(1); + PGPROC *proc; + const char *wait_event; ArrayType *blocking_pids_a; int32 *interesting_pids; int32 *blocking_pids; @@ -43,6 +51,17 @@ pg_isolation_test_session_is_blocked(PG_FUNCTION_ARGS) int i, j; + /* Check if blocked_pid is in injection_wait(). */ + proc = BackendPidGetProc(blocked_pid); + if (proc == NULL) + PG_RETURN_BOOL(false); /* session gone: definitely unblocked */ + wait_event = + pgstat_get_wait_event(UINT32_ACCESS_ONCE(proc->wait_event_info)); + if (wait_event && strncmp("INJECTION_POINT(", + wait_event, + strlen("INJECTION_POINT(")) == 0) + PG_RETURN_BOOL(true); + /* Validate the passed-in array */ Assert(ARR_ELEMTYPE(interesting_pids_a) == INT4OID); if (array_contains_nulls(interesting_pids_a)) diff --git a/src/test/modules/injection_points/Makefile b/src/test/modules/injection_points/Makefile index 31bd787..2ffd2f7 100644 --- a/src/test/modules/injection_points/Makefile +++ b/src/test/modules/injection_points/Makefile @@ -9,6 +9,8 @@ PGFILEDESC = "injection_points - facility for injection points" REGRESS = injection_points REGRESS_OPTS = --dlpath=$(top_builddir)/src/test/regress +ISOLATION = inplace + # The injection points are cluster-wide, so disable installcheck NO_INSTALLCHECK = 1 diff --git a/src/test/modules/injection_points/expected/inplace.out b/src/test/modules/injection_points/expected/inplace.out new file mode 100644 index 0000000..123f45a --- /dev/null +++ b/src/test/modules/injection_points/expected/inplace.out @@ -0,0 +1,43 @@ +Parsed test spec with 3 sessions + +starting permutation: vac1 grant2 vac3 mkrels3 read1 +mkrels +------ + +(1 row) + +injection_points_attach +----------------------- + +(1 row) + +step vac1: VACUUM vactest.orig50; -- wait during inplace update +step grant2: GRANT SELECT ON TABLE vactest.orig50 TO PUBLIC; +step vac3: VACUUM pg_class; +step mkrels3: + SELECT vactest.mkrels('intruder', 1, 100); -- repopulate LP_UNUSED + SELECT injection_points_detach('inplace-before-pin'); + SELECT injection_points_wakeup('inplace-before-pin'); + +mkrels +------ + +(1 row) + +injection_points_detach +----------------------- + +(1 row) + +injection_points_wakeup +----------------------- + +(1 row) + +step vac1: <... completed> +step read1: + REINDEX TABLE pg_class; -- look for duplicates + SELECT reltuples = -1 AS reltuples_unknown + FROM pg_class WHERE oid = 'vactest.orig50'::regclass; + +ERROR: could not create unique index "pg_class_oid_index" diff --git a/src/test/modules/injection_points/injection_points.c b/src/test/modules/injection_points/injection_points.c index 5c44625..4193061 100644 --- a/src/test/modules/injection_points/injection_points.c +++ b/src/test/modules/injection_points/injection_points.c @@ -200,6 +200,7 @@ injection_notice(const char *name, const void *private_data) void injection_wait(const char *name, const void *private_data) { + char event_name[NAMEDATALEN]; uint32 old_wait_counts = 0; int index = -1; uint32 injection_wait_event = 0; @@ -212,11 +213,11 @@ injection_wait(const char *name, const void *private_data) return; /* - * Use the injection point name for this custom wait event. Note that - * this custom wait event name is not released, but we don't care much for - * testing as this should be short-lived. + * Note that this custom wait event name is not released, but we don't + * care much for testing as this should be short-lived. */ - injection_wait_event = WaitEventExtensionNew(name); + snprintf(event_name, sizeof(event_name), "INJECTION_POINT(%s)", name); + injection_wait_event = WaitEventExtensionNew(event_name); /* * Find a free slot to wait for, and register this injection point's name. diff --git a/src/test/modules/injection_points/meson.build b/src/test/modules/injection_points/meson.build index 8e1b5b4..3c23c14 100644 --- a/src/test/modules/injection_points/meson.build +++ b/src/test/modules/injection_points/meson.build @@ -37,4 +37,9 @@ tests += { # The injection points are cluster-wide, so disable installcheck 'runningcheck': false, }, + 'isolation': { + 'specs': [ + 'inplace', + ], + }, } diff --git a/src/test/modules/injection_points/specs/inplace.spec b/src/test/modules/injection_points/specs/inplace.spec new file mode 100644 index 0000000..e957713 --- /dev/null +++ b/src/test/modules/injection_points/specs/inplace.spec @@ -0,0 +1,83 @@ +# Test race conditions involving: +# - s1: VACUUM inplace-updating a pg_class row +# - s2: GRANT/REVOKE making pg_class rows dead +# - s3: "VACUUM pg_class" making dead rows LP_UNUSED; DDL reusing them + +# Need GRANT to make a non-HOT update. Otherwise, "VACUUM pg_class" would +# leave an LP_REDIRECT that persists. To get non-HOT, make rels so the +# pg_class row for vactest.orig50 is on a filled page (assuming BLCKSZ=8192). +# Just to save on filesystem syscalls, use relkind=c for every other rel. +setup +{ + CREATE EXTENSION injection_points; + CREATE SCHEMA vactest; + CREATE FUNCTION vactest.mkrels(text, int, int) RETURNS void + LANGUAGE plpgsql SET search_path = vactest AS $$ + DECLARE + tname text; + BEGIN + FOR i in $2 .. $3 LOOP + tname := $1 || i; + EXECUTE FORMAT('CREATE TYPE ' || tname || ' AS ()'); + RAISE DEBUG '% at %', tname, ctid + FROM pg_class WHERE oid = tname::regclass; + END LOOP; + END + $$; +} +setup { VACUUM FULL pg_class; -- reduce free space } +setup +{ + SELECT vactest.mkrels('orig', 1, 49); + CREATE TABLE vactest.orig50 (); + SELECT vactest.mkrels('orig', 51, 100); +} + +# XXX DROP causes an assertion failure; adopt DROP once fixed +teardown +{ + --DROP SCHEMA vactest CASCADE; + DO $$BEGIN EXECUTE 'ALTER SCHEMA vactest RENAME TO schema' || oid FROM pg_namespace where nspname = 'vactest'; END$$; + DROP EXTENSION injection_points; +} + +# Wait during inplace update, in a VACUUM of vactest.orig50. +session s1 +setup { + SELECT injection_points_set_local(); + SELECT injection_points_attach('inplace-before-pin', 'wait'); +} +step vac1 { VACUUM vactest.orig50; -- wait during inplace update } +# One bug scenario leaves two live pg_class tuples for vactest.orig50 and zero +# live tuples for one of the "intruder" rels. REINDEX observes the duplicate. +step read1 { + REINDEX TABLE pg_class; -- look for duplicates + SELECT reltuples = -1 AS reltuples_unknown + FROM pg_class WHERE oid = 'vactest.orig50'::regclass; +} + + +# Transactional updates of the tuple vac1 is waiting to inplace-update. +session s2 +step grant2 { GRANT SELECT ON TABLE vactest.orig50 TO PUBLIC; } + + +# Non-blocking actions. +session s3 +step vac3 { VACUUM pg_class; } +# Reuse the lp that vac1 is waiting to change. I've observed reuse at the 1st +# or 18th CREATE, so create excess. +step mkrels3 { + SELECT vactest.mkrels('intruder', 1, 100); -- repopulate LP_UNUSED + SELECT injection_points_detach('inplace-before-pin'); + SELECT injection_points_wakeup('inplace-before-pin'); +} + + +# XXX extant bug +permutation + vac1(mkrels3) # reads pg_class tuple T0 for vactest.orig50, xmax invalid + grant2 # T0 becomes eligible for pruning, T1 is successor + vac3 # T0 becomes LP_UNUSED + mkrels3 # T0 reused; vac1 wakes and overwrites the reused T0 + read1 diff --git a/src/test/modules/test_misc/t/005_timeouts.pl b/src/test/modules/test_misc/t/005_timeouts.pl index a792610..18c800c 100644 --- a/src/test/modules/test_misc/t/005_timeouts.pl +++ b/src/test/modules/test_misc/t/005_timeouts.pl @@ -50,7 +50,8 @@ $psql_session->query_until( # Wait until the backend enters the timeout injection point. Will get an error # here if anything goes wrong. -$node->wait_for_event('client backend', 'transaction-timeout'); +$node->wait_for_event('client backend', + 'INJECTION_POINT(transaction-timeout)'); my $log_offset = -s $node->logfile; @@ -86,7 +87,7 @@ $psql_session->query_until( # Wait until the backend enters the timeout injection point. $node->wait_for_event('client backend', - 'idle-in-transaction-session-timeout'); + 'INJECTION_POINT(idle-in-transaction-session-timeout)'); $log_offset = -s $node->logfile; @@ -116,7 +117,8 @@ $psql_session->query_until( )); # Wait until the backend enters the timeout injection point. -$node->wait_for_event('client backend', 'idle-session-timeout'); +$node->wait_for_event('client backend', + 'INJECTION_POINT(idle-session-timeout)'); $log_offset = -s $node->logfile; diff --git a/src/test/recovery/t/041_checkpoint_at_promote.pl b/src/test/recovery/t/041_checkpoint_at_promote.pl index 7c30731..538facb 100644 --- a/src/test/recovery/t/041_checkpoint_at_promote.pl +++ b/src/test/recovery/t/041_checkpoint_at_promote.pl @@ -78,7 +78,8 @@ $node_primary->wait_for_replay_catchup($node_standby); # Wait until the checkpointer is in the middle of the restart point # processing. -$node_standby->wait_for_event('checkpointer', 'create-restart-point'); +$node_standby->wait_for_event('checkpointer', + 'INJECTION_POINT(create-restart-point)'); # Check the logs that the restart point has started on standby. This is # optional, but let's be sure.