On Sat, Apr 06, 2024 at 10:34:46AM +0500, Andrey M. Borodin wrote: > I find name of the function "injection_points_local()" strange, > because there is no verb in the name. How about > "injection_points_set_local"?
That makes sense. > I'm not sure if we should refactor anything here, but > InjectionPointSharedState has singular name, plural wait_counts and > singular condition. > InjectionPointSharedState is already an array of injection points, > maybe let's add there optional pid instead of inventing separate > array of pids? Perhaps we could unify these two concepts, indeed, with a "kind" added to InjectionPointCondition. Now waits/wakeups are a different beast than the conditions that could be assigned to a point to filter if it should be executed. More runtime conditions coming immediately into my mind, that could be added to this structure relate mostly to global objects, like: - Specific database name and/or OID. - Specific role(s). So that's mostly cross-checking states coming from miscadmin.h for now. > Can we set global point to 'notice', but same local to 'wait'? Looks > like now we can't, but allowing to do so would make code simpler. You mean using the name point name with more than more callback? Not sure we'd want to be able to do that. Perhaps you're right, though, if there is a use case that justifies it. > Besides this opportunity to simplify stuff, both patches looks good > to me. Yeah, this module can be always tweaked more if necessary. Saying that, naming the new thing "condition" in InjectionPointSharedState felt strange, as you said, because it is an array of multiple conditions. For now I have applied 997db123c054 to make the GIN tests with injection points repeatable as it was an independent issue, and f587338dec87 to add the local function pieces. Attached is the last piece to switch the GIN test to use local injection points. 85f65d7a26fc should maintain the buildfarm at bay, but I'd rather not take a bet and accidently freeze the buildfarm as it would impact folks who aim at getting patches committed just before the finish line. So I am holding on this one for a few more days until we're past the freeze and the buildfarm is more stable. -- Michael
From bc2ce072cad0efa94084297330db3102ee346c25 Mon Sep 17 00:00:00 2001
From: Michael Paquier <mich...@paquier.xyz>
Date: Mon, 8 Apr 2024 10:20:19 +0900
Subject: [PATCH v3] Switch GIN tests with injection points to be
concurrent-safe
---
src/test/modules/gin/Makefile | 3 ---
src/test/modules/gin/expected/gin_incomplete_splits.out | 7 +++++++
src/test/modules/gin/meson.build | 2 --
src/test/modules/gin/sql/gin_incomplete_splits.sql | 3 +++
4 files changed, 10 insertions(+), 5 deletions(-)
diff --git a/src/test/modules/gin/Makefile b/src/test/modules/gin/Makefile
index da4c9cea5e..e007e38ac2 100644
--- a/src/test/modules/gin/Makefile
+++ b/src/test/modules/gin/Makefile
@@ -4,9 +4,6 @@ EXTRA_INSTALL = src/test/modules/injection_points
REGRESS = gin_incomplete_splits
-# The injection points are cluster-wide, so disable installcheck
-NO_INSTALLCHECK = 1
-
ifdef USE_PGXS
PG_CONFIG = pg_config
PGXS := $(shell $(PG_CONFIG) --pgxs)
diff --git a/src/test/modules/gin/expected/gin_incomplete_splits.out b/src/test/modules/gin/expected/gin_incomplete_splits.out
index 973a8ce6c8..15574e547a 100644
--- a/src/test/modules/gin/expected/gin_incomplete_splits.out
+++ b/src/test/modules/gin/expected/gin_incomplete_splits.out
@@ -12,6 +12,13 @@
-- This uses injection points to cause errors that leave some page
-- splits in "incomplete" state
create extension injection_points;
+-- Make all injection points local to this process, for concurrency.
+SELECT injection_points_set_local();
+ injection_points_set_local
+----------------------------
+
+(1 row)
+
-- Use the index for all the queries
set enable_seqscan=off;
-- Print a NOTICE whenever an incomplete split gets fixed
diff --git a/src/test/modules/gin/meson.build b/src/test/modules/gin/meson.build
index 5ec0760a27..9734b51de2 100644
--- a/src/test/modules/gin/meson.build
+++ b/src/test/modules/gin/meson.build
@@ -12,7 +12,5 @@ tests += {
'sql': [
'gin_incomplete_splits',
],
- # The injection points are cluster-wide, so disable installcheck
- 'runningcheck': false,
},
}
diff --git a/src/test/modules/gin/sql/gin_incomplete_splits.sql b/src/test/modules/gin/sql/gin_incomplete_splits.sql
index ea3667b38d..ebf0f620f0 100644
--- a/src/test/modules/gin/sql/gin_incomplete_splits.sql
+++ b/src/test/modules/gin/sql/gin_incomplete_splits.sql
@@ -14,6 +14,9 @@
-- splits in "incomplete" state
create extension injection_points;
+-- Make all injection points local to this process, for concurrency.
+SELECT injection_points_set_local();
+
-- Use the index for all the queries
set enable_seqscan=off;
--
2.43.0
signature.asc
Description: PGP signature
