Re: Weird test mixup
Michael Paquier <michael@paquier.xyz>
From: Michael Paquier <michael@paquier.xyz>
To: "Andrey M. Borodin" <x4mmm@yandex-team.ru>
Cc: Heikki Linnakangas <hlinnaka@iki.fi>, Tom Lane <tgl@sss.pgh.pa.us>, pgsql-hackers <pgsql-hackers@postgresql.org>
Date: 2024-04-08T01:22:40Z
Lists: pgsql-hackers
Attachments
- v3-0002-Switch-GIN-tests-with-injection-points-to-be-conc.patch (text/x-diff) patch v3-0002
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
Commits
-
injection_points: Store runtime conditions in private area
- 267d41dc4f41 17.0 landed
-
Introduce private data area for injection points
- 33181b48fd0e 17.0 landed
-
injection_points: Fix incorrect spinlock acquisition
- 597f66942d0b 17.0 landed
-
Make GIN tests using injection points concurrent-safe
- 5105c9079681 17.0 landed
-
injection_points: Fix race condition with local injection point tests
- f4083c497510 17.0 landed
-
injection_points: Introduce runtime conditions
- f587338dec87 17.0 landed
-
Make GIN test using injection points repeatable
- 997db123c054 17.0 landed
-
Fix backstop in gin test if injection point is not reached
- d802ff06d021 17.0 landed
-
Try to unbreak injection-fault tests in the buildfarm
- 85f65d7a26fc 17.0 landed
-
Disable tests using injection points in installcheck
- e2e3b8ae9ed7 17.0 landed