Обсуждение: Weird test mixup
I got a weird test failure while testing my forking refactor patches on Cirrus CI (https://cirrus-ci.com/task/5880724448870400?logs=test_running#L121): > [16:52:39.753] Summary of Failures: > [16:52:39.753] > [16:52:39.753] 66/73 postgresql:intarray-running / intarray-running/regress ERROR 6.27s exit status 1 > [16:52:39.753] > [16:52:39.753] Ok: 72 > [16:52:39.753] Expected Fail: 0 > [16:52:39.753] Fail: 1 > [16:52:39.753] Unexpected Pass: 0 > [16:52:39.753] Skipped: 0 > [16:52:39.753] Timeout: 0 > [16:52:39.753] > [16:52:39.753] Full log written to /tmp/cirrus-ci-build/build/meson-logs/testlog-running.txt And: > diff -U3 /tmp/cirrus-ci-build/contrib/intarray/expected/_int.out /tmp/cirrus-ci-build/build/testrun/intarray-running/regress/results/_int.out > --- /tmp/cirrus-ci-build/contrib/intarray/expected/_int.out 2024-03-14 16:48:48.690367000 +0000 > +++ /tmp/cirrus-ci-build/build/testrun/intarray-running/regress/results/_int.out 2024-03-14 16:52:05.759444000 +0000 > @@ -804,6 +804,7 @@ > > DROP INDEX text_idx; > CREATE INDEX text_idx on test__int using gin ( a gin__int_ops ); > +ERROR: error triggered for injection point gin-leave-leaf-split-incomplete > SELECT count(*) from test__int WHERE a && '{23,50}'; > count > ------- > @@ -877,6 +878,7 @@ > (1 row) > > DROP INDEX text_idx; > +ERROR: index "text_idx" does not exist > -- Repeat the same queries with an extended data set. The data set is the > -- same that we used before, except that each element in the array is > -- repeated three times, offset by 1000 and 2000. For example, {1, 5} Somehow the 'gin-leave-leaf-split-incomplete' injection point was active in the 'intarray' test. That makes no sense. That injection point is only used by the test in src/test/modules/gin/. Perhaps that ran at the same time as the intarray test? But they run in separate instances, with different data directories. And the 'gin' test passed. I'm completely stumped. Anyone have a theory? -- Heikki Linnakangas Neon (https://neon.tech)
Heikki Linnakangas <hlinnaka@iki.fi> writes: > Somehow the 'gin-leave-leaf-split-incomplete' injection point was active > in the 'intarray' test. That makes no sense. That injection point is > only used by the test in src/test/modules/gin/. Perhaps that ran at the > same time as the intarray test? But they run in separate instances, with > different data directories. Do they? It'd be fairly easy to explain this if these things were being run in "installcheck" style. I'm not sure about CI, but from memory, the buildfarm does use installcheck for some things. I wonder if it'd be wise to adjust the injection point stuff so that it's active in only the specific database the injection point was activated in. regards, tom lane
On Fri, Mar 15, 2024 at 11:19 AM Tom Lane <tgl@sss.pgh.pa.us> wrote: > Heikki Linnakangas <hlinnaka@iki.fi> writes: > > Somehow the 'gin-leave-leaf-split-incomplete' injection point was active > > in the 'intarray' test. That makes no sense. That injection point is > > only used by the test in src/test/modules/gin/. Perhaps that ran at the > > same time as the intarray test? But they run in separate instances, with > > different data directories. > > Do they? It'd be fairly easy to explain this if these things were > being run in "installcheck" style. I'm not sure about CI, but from > memory, the buildfarm does use installcheck for some things. Right, as mentioned here: https://www.postgresql.org/message-id/flat/CA%2BhUKGJYhcG_o2nwSK6r01eOZJwNWUJUbX%3D%3DAVnW84f-%2B8yamQ%40mail.gmail.com That's the "running" test, which is like the old installcheck.
I wrote: > Heikki Linnakangas <hlinnaka@iki.fi> writes: >> Somehow the 'gin-leave-leaf-split-incomplete' injection point was active >> in the 'intarray' test. That makes no sense. That injection point is >> only used by the test in src/test/modules/gin/. Perhaps that ran at the >> same time as the intarray test? But they run in separate instances, with >> different data directories. > Do they? It'd be fairly easy to explain this if these things were > being run in "installcheck" style. I'm not sure about CI, but from > memory, the buildfarm does use installcheck for some things. Hmm, Munro's comment yesterday[1] says that current CI does use installcheck mode in some cases. regards, tom lane [1] https://www.postgresql.org/message-id/CA+hUKGJYhcG_o2nwSK6r01eOZJwNWUJUbX==AVnW84f-+8yamQ@mail.gmail.com
Thomas Munro <thomas.munro@gmail.com> writes: > On Fri, Mar 15, 2024 at 11:19 AM Tom Lane <tgl@sss.pgh.pa.us> wrote: >> Do they? It'd be fairly easy to explain this if these things were >> being run in "installcheck" style. I'm not sure about CI, but from >> memory, the buildfarm does use installcheck for some things. > Right, as mentioned here: > https://www.postgresql.org/message-id/flat/CA%2BhUKGJYhcG_o2nwSK6r01eOZJwNWUJUbX%3D%3DAVnW84f-%2B8yamQ%40mail.gmail.com > That's the "running" test, which is like the old installcheck. Hmm. Seems like maybe we need to institute a rule that anything using injection points has to be marked NO_INSTALLCHECK. That's kind of a big hammer though. regards, tom lane
On Thu, Mar 14, 2024 at 06:19:38PM -0400, Tom Lane wrote: > Do they? It'd be fairly easy to explain this if these things were > being run in "installcheck" style. I'm not sure about CI, but from > memory, the buildfarm does use installcheck for some things. > > I wonder if it'd be wise to adjust the injection point stuff so that > it's active in only the specific database the injection point was > activated in. It can be made optional by extending InjectionPointAttach() to specify a database OID or a database name. Note that 041_checkpoint_at_promote.pl wants an injection point to run in the checkpointer, where we don't have a database requirement. Or we could just disable runningcheck because of the concurrency requirement in this test. The test would still be able to run, just less times. -- Michael
Вложения
On Fri, Mar 15, 2024 at 07:53:57AM +0900, Michael Paquier wrote: > It can be made optional by extending InjectionPointAttach() to > specify a database OID or a database name. Note that > 041_checkpoint_at_promote.pl wants an injection point to run in the > checkpointer, where we don't have a database requirement. Slight correction here. It is also possible to not touch InjectionPointAttach() at all: just tweak the callbacks to do that as long as the database that should be used is tracked in shmem with its point name, say with new fields in InjectionPointSharedState. That keeps the backend APIs in a cleaner state. -- Michael
Вложения
Michael Paquier <michael@paquier.xyz> writes: > On Thu, Mar 14, 2024 at 06:19:38PM -0400, Tom Lane wrote: >> I wonder if it'd be wise to adjust the injection point stuff so that >> it's active in only the specific database the injection point was >> activated in. > It can be made optional by extending InjectionPointAttach() to > specify a database OID or a database name. Note that > 041_checkpoint_at_promote.pl wants an injection point to run in the > checkpointer, where we don't have a database requirement. > Or we could just disable runningcheck because of the concurrency > requirement in this test. The test would still be able to run, just > less times. No, actually we *must* mark all these tests NO_INSTALLCHECK if we stick with the current definition of injection points. The point of installcheck mode is that the tests are supposed to be safe to run in a live installation. Side-effects occurring in other databases are completely not OK. I can see that some tests would want to be able to inject code cluster-wide, but I bet that's going to be a small minority. I suggest that we invent a notion of "global" vs "local" injection points, where a "local" one only fires in the DB it was defined in. Then only tests that require a global injection point need to be NO_INSTALLCHECK. regards, tom lane
On Thu, Mar 14, 2024 at 07:13:53PM -0400, Tom Lane wrote: > Michael Paquier <michael@paquier.xyz> writes: >> Or we could just disable runningcheck because of the concurrency >> requirement in this test. The test would still be able to run, just >> less times. > > No, actually we *must* mark all these tests NO_INSTALLCHECK if we > stick with the current definition of injection points. The point > of installcheck mode is that the tests are supposed to be safe to > run in a live installation. Side-effects occurring in other > databases are completely not OK. I really don't want to plug any runtime conditions into the backend APIs, because there can be so much more that can be done there than only restricting a callback to a database. I can imagine process type restrictions, process PID restrictions, etc. So this knowledge should stick into the test module itself, and be expanded there. That's easier ABI-wise, as well. > I can see that some tests would want to be able to inject code > cluster-wide, but I bet that's going to be a small minority. > I suggest that we invent a notion of "global" vs "local" > injection points, where a "local" one only fires in the DB it > was defined in. Then only tests that require a global injection > point need to be NO_INSTALLCHECK. Attached is a POC of what could be done. I have extended the module injection_points so as it is possible to register what I'm calling a "condition" in the module that can be defined with a new SQL function. The condition is stored in shared memory with the point name, then at runtime the conditions are cross-checked in the callbacks. With the interface of this patch, the condition should be registered *before* a point is attached, but this stuff could also be written so as injection_points_attach() takes an optional argument with a database name. Or this could use a different, new SQL function, say a injection_points_attach_local() that registers a condition with MyDatabaseId on top of attaching the point, making the whole happening while holding once the spinlock of the shmem state for the module. By the way, modules/gin/ was missing missing a detach, so the test was not repeatable with successive installchecks. Adding a pg_sleep of a few seconds after 'gin-leave-leaf-split-incomplete' is registered enlarges the window, and the patch avoids failures when running installcheck in parallel for modules/gin/ and something else using gin, like contrib/btree_gin/: while make USE_MODULE_DB=1 installcheck; do :; done 0001 is the condition facility for the module, 0002 is a fix for the GIN test. Thoughts are welcome. -- Michael
Вложения
On 15/03/2024 09:39, Michael Paquier wrote: > On Thu, Mar 14, 2024 at 07:13:53PM -0400, Tom Lane wrote: >> I can see that some tests would want to be able to inject code >> cluster-wide, but I bet that's going to be a small minority. >> I suggest that we invent a notion of "global" vs "local" >> injection points, where a "local" one only fires in the DB it >> was defined in. Then only tests that require a global injection >> point need to be NO_INSTALLCHECK. > > Attached is a POC of what could be done. I have extended the module > injection_points so as it is possible to register what I'm calling a > "condition" in the module that can be defined with a new SQL function. > > The condition is stored in shared memory with the point name, then at > runtime the conditions are cross-checked in the callbacks. With the > interface of this patch, the condition should be registered *before* a > point is attached, but this stuff could also be written so as > injection_points_attach() takes an optional argument with a database > name. Or this could use a different, new SQL function, say a > injection_points_attach_local() that registers a condition with > MyDatabaseId on top of attaching the point, making the whole happening > while holding once the spinlock of the shmem state for the module. For the gin test, a single "SELECT injection_points_attach_local()" at the top of the test file would be most convenient. If I have to do "SELECT injection_points_condition('gin-finish-incomplete-split', :'datname');" for every injection point in the test, I will surely forget it sometimes. In the 'gin' test, they could actually be scoped to the same backend. Wrt. the spinlock and shared memory handling, I think this would be simpler if you could pass some payload in the InjectionPointAttach() call, which would be passed back to the callback function: void InjectionPointAttach(const char *name, const char *library, - const char *function) + const char *function, + uint64 payload) In this case, the payload would be the "slot index" in shared memory. Or perhaps always allocate, say, 1024 bytes of working area for every attached injection point that the test module can use any way it wants. Like for storing extra conditions, or for the wakeup counter stuff in injection_wait(). A fixed size working area is a little crude, but would be very handy in practice. > By the way, modules/gin/ was missing missing a detach, so the test was > not repeatable with successive installchecks. Oops. It would be nice to automatically detach all the injection points on process exit. You wouldn't always want that, but I think most tests hold a session open throughout the test, and for those it would be handy. -- Heikki Linnakangas Neon (https://neon.tech)
On 15/03/2024 01:13, Tom Lane wrote: > Michael Paquier <michael@paquier.xyz> writes: >> Or we could just disable runningcheck because of the concurrency >> requirement in this test. The test would still be able to run, just >> less times. > > No, actually we *must* mark all these tests NO_INSTALLCHECK if we > stick with the current definition of injection points. The point > of installcheck mode is that the tests are supposed to be safe to > run in a live installation. Side-effects occurring in other > databases are completely not OK. I committed a patch to do that, to put out the fire. -- Heikki Linnakangas Neon (https://neon.tech)
On 15/03/2024 13:09, Heikki Linnakangas wrote: > On 15/03/2024 01:13, Tom Lane wrote: >> Michael Paquier <michael@paquier.xyz> writes: >>> Or we could just disable runningcheck because of the concurrency >>> requirement in this test. The test would still be able to run, just >>> less times. >> >> No, actually we *must* mark all these tests NO_INSTALLCHECK if we >> stick with the current definition of injection points. The point >> of installcheck mode is that the tests are supposed to be safe to >> run in a live installation. Side-effects occurring in other >> databases are completely not OK. > > I committed a patch to do that, to put out the fire. That's turning the buildfarm quite red. Many, but not all animals are failing like this: > --- /home/buildfarm/hippopotamus/buildroot/HEAD/pgsql.build/src/test/modules/injection_points/expected/injection_points.out 2024-03-15 12:41:16.363286975 +0100 > +++ /home/buildfarm/hippopotamus/buildroot/HEAD/pgsql.build/src/test/modules/injection_points/results/injection_points.out 2024-03-15 12:53:11.528159615 +0100 > @@ -1,118 +1,111 @@ > CREATE EXTENSION injection_points; > +ERROR: extension "injection_points" is not available > +DETAIL: Could not open extension control file "/home/buildfarm/hippopotamus/buildroot/HEAD/pgsql.build/tmp_install/home/buildfarm/hippopotamus/buildroot/HEAD/inst/share/postgresql/extension/injection_points.control": Nosuch file or directory. > +HINT: The extension must first be installed on the system where PostgreSQL is running. > ... Looks like adding NO_INSTALLCHECK somehow affected how the modules are installed in tmp_install. I'll investigate.. -- Heikki Linnakangas Neon (https://neon.tech)
On 15/03/2024 14:10, Heikki Linnakangas wrote: > On 15/03/2024 13:09, Heikki Linnakangas wrote: >> I committed a patch to do that, to put out the fire. > > That's turning the buildfarm quite red. Many, but not all animals are > failing like this: > >> --- /home/buildfarm/hippopotamus/buildroot/HEAD/pgsql.build/src/test/modules/injection_points/expected/injection_points.out 2024-03-15 12:41:16.363286975 +0100 >> +++ /home/buildfarm/hippopotamus/buildroot/HEAD/pgsql.build/src/test/modules/injection_points/results/injection_points.out 2024-03-15 12:53:11.528159615 +0100 >> @@ -1,118 +1,111 @@ >> CREATE EXTENSION injection_points; >> +ERROR: extension "injection_points" is not available >> +DETAIL: Could not open extension control file "/home/buildfarm/hippopotamus/buildroot/HEAD/pgsql.build/tmp_install/home/buildfarm/hippopotamus/buildroot/HEAD/inst/share/postgresql/extension/injection_points.control": Nosuch file or directory. >> +HINT: The extension must first be installed on the system where PostgreSQL is running. >> ... > > Looks like adding NO_INSTALLCHECK somehow affected how the modules are > installed in tmp_install. I'll investigate.. I think this is a bug in the buildfarm client. In the make_misc_check step, it does this (reduced to just the interesting parts): > # run the modules that can't be run with installcheck > sub make_misc_check > { > ... > my @dirs = glob("$pgsql/src/test/modules/* $pgsql/contrib/*"); > foreach my $dir (@dirs) > { > next unless -e "$dir/Makefile"; > my $makefile = file_contents("$dir/Makefile"); > next unless $makefile =~ /^NO_INSTALLCHECK/m; > my $test = basename($dir); > > # skip redundant TAP tests which are called elsewhere > my @out = run_log("cd $dir && $make $instflags TAP_TESTS= check"); > ... > } So it scans src/test/modules, and runs "make check" for all subdirectories that have NO_INSTALLCHECK in the makefile. But the injection fault tests are also conditional on the enable_injection_points in the parent Makefile: > ifeq ($(enable_injection_points),yes) > SUBDIRS += injection_points gin > else > ALWAYS_SUBDIRS += injection_points gin > endif The buildfarm client doesn't pay any attention to that, and runs the test anyway. I committed an ugly hack to the subdirectory Makefiles, to turn "make check" into a no-op if injection points are disabled. Normally when you run "make check" at the parent level, it doesn't even recurse to the directories, but this works around the buildfarm script. I hope... -- Heikki Linnakangas Neon (https://neon.tech)
Heikki Linnakangas <hlinnaka@iki.fi> writes: > On 15/03/2024 13:09, Heikki Linnakangas wrote: >> I committed a patch to do that, to put out the fire. > That's turning the buildfarm quite red. Many, but not all animals are > failing like this: It may be even worse than it appears from the buildfarm status page. My animals were stuck in infinite loops that required a manual "kill" to get out of, and it's reasonable to assume there are others that will require owner intervention. Why would this test have done that, if the module failed to load? regards, tom lane
On 15/03/2024 16:00, Tom Lane wrote: > Heikki Linnakangas <hlinnaka@iki.fi> writes: >> On 15/03/2024 13:09, Heikki Linnakangas wrote: >>> I committed a patch to do that, to put out the fire. > >> That's turning the buildfarm quite red. Many, but not all animals are >> failing like this: > > It may be even worse than it appears from the buildfarm status page. > My animals were stuck in infinite loops that required a manual "kill" > to get out of, and it's reasonable to assume there are others that > will require owner intervention. Why would this test have done that, > if the module failed to load? The gin_incomplete_split test inserts rows until it hits the injection point, at page split. There is a backstop, it should give up after 10000 iterations, but that was broken. Fixed that, thanks for the report! Hmm, don't we have any timeout that would kill tests if they get stuck? -- Heikki Linnakangas Neon (https://neon.tech)
Heikki Linnakangas <hlinnaka@iki.fi> writes: > On 15/03/2024 16:00, Tom Lane wrote: >> It may be even worse than it appears from the buildfarm status page. >> My animals were stuck in infinite loops that required a manual "kill" >> to get out of, and it's reasonable to assume there are others that >> will require owner intervention. Why would this test have done that, >> if the module failed to load? > The gin_incomplete_split test inserts rows until it hits the injection > point, at page split. There is a backstop, it should give up after 10000 > iterations, but that was broken. Fixed that, thanks for the report! Duh ... > Hmm, don't we have any timeout that would kill tests if they get stuck? AFAIK, the only constraint on a buildfarm animal's runtime is the wait_timeout setting, which is infinite by default, and was on my machines. (Not anymore ;-).) We do have timeouts in (most?) TAP tests, but this wasn't a TAP test. If this is a continuous-insertion loop, presumably it will run the machine out of disk space eventually, which could be unpleasant if there are other services running besides the buildfarm. I think I'll go notify the buildfarm owners list to check for trouble. Are there limits on the runtime of CI or cfbot jobs? Maybe somebody should go check those systems. regards, tom lane
On Sat, Mar 16, 2024 at 7:27 AM Tom Lane <tgl@sss.pgh.pa.us> wrote: > Are there limits on the runtime of CI or cfbot jobs? Maybe > somebody should go check those systems. Those get killed at a higher level after 60 minutes (configurable but we didn't change it AFAIK): https://cirrus-ci.org/faq/#instance-timed-out It's a fresh virtual machine for each run, and after that it's gone (well the ccache directory survives but only by being uploaded/downloaded in explicit steps to transmit it between runs).
On Fri, Mar 15, 2024 at 11:23:31AM +0200, Heikki Linnakangas wrote: > For the gin test, a single "SELECT injection_points_attach_local()" at the > top of the test file would be most convenient. > > If I have to do "SELECT > injection_points_condition('gin-finish-incomplete-split', :'datname');" for > every injection point in the test, I will surely forget it sometimes. So will I, most likely.. The odds never play in favor of hackers. I have a few more tests in mind that can be linked to a specific backend with SQL queries, but I've not been able to get back to it yet. > Wrt. the spinlock and shared memory handling, I think this would be simpler > if you could pass some payload in the InjectionPointAttach() call, which > would be passed back to the callback function: > > In this case, the payload would be the "slot index" in shared memory. > > Or perhaps always allocate, say, 1024 bytes of working area for every > attached injection point that the test module can use any way it wants. Like > for storing extra conditions, or for the wakeup counter stuff in > injection_wait(). A fixed size working area is a little crude, but would be > very handy in practice. Perhaps. I am not sure that we need more than the current signature, all that can just be handled in some module-specific shmem area. The key is to be able to link a point name to some state related to it. Using a hash table would be more efficient, but performance wise a array is not going to matter as there will most likely never be more than 8 points. 4 is already a lot, just doubling that on safety ground. > It would be nice to automatically detach all the injection points on process > exit. You wouldn't always want that, but I think most tests hold a session > open throughout the test, and for those it would be handy. Linking all the points to a PID with a injection_points_attach_local() that switches a static flag while registering a before_shmem_exit() to do an automated cleanup sounds like the simplest approach to me based on what I'm reading on this thread. (Just saw the buildfarm storm, wow.) -- Michael
Вложения
On Sat, Mar 16, 2024 at 08:40:21AM +0900, Michael Paquier wrote: > Linking all the points to a PID with a injection_points_attach_local() > that switches a static flag while registering a before_shmem_exit() to > do an automated cleanup sounds like the simplest approach to me based > on what I'm reading on this thread. Please find a patch to do exactly that, without touching the backend APIs. 0001 adds a new function call injection_points_local() that can be added on top of a SQL test to make it concurrent-safe. 0002 is the fix for the GIN tests. I am going to add an open item to not forget about all that. Comments are welcome. -- Michael
Вложения
> On 18 Mar 2024, at 06:04, Michael Paquier <michael@paquier.xyz> wrote: > > new function call injection_points_local() that can > be added on top of a SQL test to make it concurrent-safe. Maybe consider function injection_points_attach_local(‘point name’) instead of static switch? Or even injection_points_attach_global(‘point name’), while function injection_points_attach(‘point name’) will be global?This would favour writing concurrent test by default… Best regards, Andrey Borodin.
On Mon, Mar 18, 2024 at 10:50:25AM +0500, Andrey M. Borodin wrote: > Maybe consider function injection_points_attach_local(‘point name’) > instead of static switch? > Or even injection_points_attach_global(‘point name’), while function > injection_points_attach(‘point name’) will be global? This would > favour writing concurrent test by default… The point is to limit accidents like the one of this thread. So, for cases already in the tree, not giving the point name in the SQL function would be simple enough. What you are suggesting can be simply done, as well, though I'd rather wait for a reason to justify doing so. -- Michael
Вложения
On Mon, Mar 18, 2024 at 10:04:45AM +0900, Michael Paquier wrote: > Please find a patch to do exactly that, without touching the backend > APIs. 0001 adds a new function call injection_points_local() that can > be added on top of a SQL test to make it concurrent-safe. 0002 is the > fix for the GIN tests. > > I am going to add an open item to not forget about all that. It's been a couple of weeks since this has been sent, and this did not get any reviews. I'd still be happy with the simplicity of a single injection_points_local() that can be used to link all the injection points created in a single process to it, discarding them once the process exists with a shmem exit callback. And I don't really see an argument to tweak the backend-side routines, as well. Comments and/or objections? -- Michael
Вложения
> On 5 Apr 2024, at 07:19, Michael Paquier <michael@paquier.xyz> wrote: > > It's been a couple of weeks since this has been sent, and this did not > get any reviews. I'd still be happy with the simplicity of a single > injection_points_local() that can be used to link all the injection > points created in a single process to it, discarding them once the > process exists with a shmem exit callback. OK, makes sense. I find name of the function "injection_points_local()" strange, because there is no verb in the name. How about "injection_points_set_local"? > And I don't really see an > argument to tweak the backend-side routines, as well. > Comments and/or > objections? I'm not sure if we should refactor anything here, but InjectionPointSharedState has singular name, plural wait_counts andsingular condition. InjectionPointSharedState is already an array of injection points, maybe let's add there optional pid instead of inventingseparate array of pids? Can we set global point to 'notice', but same local to 'wait'? Looks like now we can't, but allowing to do so would makecode simpler. Besides this opportunity to simplify stuff, both patches looks good to me. Best regards, Andrey Borodin.
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
Вложения
On Mon, Apr 08, 2024 at 10:22:40AM +0900, Michael Paquier wrote: > 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. Bharath has reported me offlist that one of the new tests has a race condition when doing the reconnection. When the backend creating the local points is very slow to exit, the backend created after the reconnection may detect that a local point previously created still exists, causing a failure. The failure can be reproduced with a sleep in the shmem exit callback, like: --- a/src/test/modules/injection_points/injection_points.c +++ b/src/test/modules/injection_points/injection_points.c @@ -163,6 +163,8 @@ injection_points_cleanup(int code, Datum arg) if (!injection_point_local) return; + pg_usleep(1000000 * 1L); + SpinLockAcquire(&inj_state->lock); for (int i = 0; i < INJ_MAX_CONDITION; i++) { At first I was looking at a loop with a scan of pg_stat_activity, but I've noticed that regress.so includes a wait_pid() that we can use to make sure that a given process exits before moving on to the next parts of a test, so I propose to just reuse that here. This requires tweaks with --dlpath for meson and ./configure, nothing new. The CI is clean. Patch attached. Thoughts? -- Michael
Вложения
> On 8 Apr 2024, at 10:33, Michael Paquier <michael@paquier.xyz> wrote: > > Thoughts? As an alternative we can make local injection points mutually exclusive. Best regards, Andrey Borodin.
On Mon, Apr 08, 2024 at 10:42:08AM +0300, Andrey M. Borodin wrote: > As an alternative we can make local injection points mutually exclusive. Sure. Now, the point of the test is to make sure that the local cleanup happens, so I'd rather keep it as-is and use the same names across reloads. -- Michael
Вложения
> On 8 Apr 2024, at 11:55, Michael Paquier <michael@paquier.xyz> wrote: > > the point of the test is to make sure that the local > cleanup happens Uh, I did not understand this. Because commit message was about stabiilzizing tests, not extending coverage. Also, should we drop function wait_pid() at the end of a test? Given that tweaks with are nothing new, I think patch looks good. Best regards, Andrey Borodin.
On Mon, Apr 08, 2024 at 12:29:43PM +0300, Andrey M. Borodin wrote: > On 8 Apr 2024, at 11:55, Michael Paquier <michael@paquier.xyz> wrote: >> Uh, I did not understand this. Because commit message was about >> stabiilzizing tests, not extending coverage. Okay, it is about stabilizing an existing test. > Also, should we drop function wait_pid() at the end of a test? Sure. > Given that tweaks with are nothing new, I think patch looks good. Applied that after a second check. And thanks to Bharath for the poke. -- Michael
Вложения
On Tue, Apr 09, 2024 at 12:41:57PM +0900, Michael Paquier wrote: > Applied that after a second check. And thanks to Bharath for the > poke. And now that the buildfarm is cooler, I've also applied the final patch in the series as of 5105c9079681 to make the GIN module concurrent-safe using injection_points_set_local(). -- Michael
Вложения
While writing an injection point test, I encountered a variant of the race condition that f4083c4 fixed. It had three sessions and this sequence of events: s1: local-attach to POINT s2: enter InjectionPointRun(POINT), yield CPU just before injection_callback() s3: detach POINT, deleting the InjectionPointCondition record s2: wake up and run POINT as though it had been non-local On Sat, Mar 16, 2024 at 08:40:21AM +0900, Michael Paquier wrote: > On Fri, Mar 15, 2024 at 11:23:31AM +0200, Heikki Linnakangas wrote: > > Wrt. the spinlock and shared memory handling, I think this would be simpler > > if you could pass some payload in the InjectionPointAttach() call, which > > would be passed back to the callback function: > > > > In this case, the payload would be the "slot index" in shared memory. > > > > Or perhaps always allocate, say, 1024 bytes of working area for every > > attached injection point that the test module can use any way it wants. Like > > for storing extra conditions, or for the wakeup counter stuff in > > injection_wait(). A fixed size working area is a little crude, but would be > > very handy in practice. That would be one good way to solve it. (Storing a slot index has the same race condition, but it fixes the race to store a struct containing the PID.) The best alternative I see is to keep an InjectionPointCondition forever after creating it. Give it a "bool valid" field that we set on detach. I don't see a major reason to prefer one of these over the other. One puts a negligible amount of memory pressure on the main segment, but it simplifies the module code. I lean toward the "1024 bytes of working area" idea. Other ideas or opinions? Separately, injection_points_cleanup() breaks the rules by calling InjectionPointDetach() while holding a spinlock. The latter has an elog(ERROR), and reaching that elog(ERROR) leaves a stuck spinlock. I haven't given as much thought to solutions for this one. Thanks, nm
On Wed, May 01, 2024 at 04:12:14PM -0700, Noah Misch wrote: > While writing an injection point test, I encountered a variant of the race > condition that f4083c4 fixed. It had three sessions and this sequence of > events: > > s1: local-attach to POINT > s2: enter InjectionPointRun(POINT), yield CPU just before injection_callback() > s3: detach POINT, deleting the InjectionPointCondition record > s2: wake up and run POINT as though it had been non-local Fun. One thing I would ask is why it makes sense to be able to detach a local point from a different session than the one who defined it as local. Shouldn't the operation of s3 be restricted rather than authorized as a safety measure, instead? > On Sat, Mar 16, 2024 at 08:40:21AM +0900, Michael Paquier wrote: >> On Fri, Mar 15, 2024 at 11:23:31AM +0200, Heikki Linnakangas wrote: >>> Wrt. the spinlock and shared memory handling, I think this would be simpler >>> if you could pass some payload in the InjectionPointAttach() call, which >>> would be passed back to the callback function: >>> >>> In this case, the payload would be the "slot index" in shared memory. >>> >>> Or perhaps always allocate, say, 1024 bytes of working area for every >>> attached injection point that the test module can use any way it wants. Like >>> for storing extra conditions, or for the wakeup counter stuff in >>> injection_wait(). A fixed size working area is a little crude, but would be >>> very handy in practice. > > That would be one good way to solve it. (Storing a slot index has the same > race condition, but it fixes the race to store a struct containing the PID.) > > The best alternative I see is to keep an InjectionPointCondition forever after > creating it. Give it a "bool valid" field that we set on detach. I don't see > a major reason to prefer one of these over the other. One puts a negligible > amount of memory pressure on the main segment, but it simplifies the module > code. I lean toward the "1024 bytes of working area" idea. Other ideas or > opinions? If more state data is needed, the fixed area injection_point.c would be better. Still, I am not sure that this is required here, either. > Separately, injection_points_cleanup() breaks the rules by calling > InjectionPointDetach() while holding a spinlock. The latter has an > elog(ERROR), and reaching that elog(ERROR) leaves a stuck spinlock. I haven't > given as much thought to solutions for this one. Indeed. That's a brain fade. This one could be fixed by collecting the point names when cleaning up the conditions and detach after releasing the spinlock. This opens a race condition between the moment when the spinlock is released and the detach, where another backend could come in and detach a point before the shmem_exit callback has the time to do its cleanup, even if detach() is restricted for local points. So we could do the callback cleanup in three steps in the shmem exit callback: - Collect the names of the points to detach, while holding the spinlock. - Do the Detach. - Take again the spinlock, clean up the conditions. Please see the attached. -- Michael
Вложения
> On 2 May 2024, at 12:27, Michael Paquier <michael@paquier.xyz> wrote: > > On Wed, May 01, 2024 at 04:12:14PM -0700, Noah Misch wrote: >> While writing an injection point test, I encountered a variant of the race >> condition that f4083c4 fixed. It had three sessions and this sequence of >> events: >> >> s1: local-attach to POINT >> s2: enter InjectionPointRun(POINT), yield CPU just before injection_callback() >> s3: detach POINT, deleting the InjectionPointCondition record >> s2: wake up and run POINT as though it had been non-local > > Fun. One thing I would ask is why it makes sense to be able to detach > a local point from a different session than the one who defined it as > local. Shouldn't the operation of s3 be restricted rather than > authorized as a safety measure, instead? That seems to prevent meaningful use case. If we want exactly one session to be waiting just before some specific point,the only way to achieve this is to create local injection point. But the session must be resumable from another session. Without this local waiting injection points are meaningless. Best regards, Andrey Borodin.
On Thu, May 02, 2024 at 01:33:45PM +0500, Andrey M. Borodin wrote: > That seems to prevent meaningful use case. If we want exactly one > session to be waiting just before some specific point, the only way > to achieve this is to create local injection point. But the session > must be resumable from another session. > Without this local waiting injection points are meaningless. I am not quite sure to follow your argument here. It is still possible to attach a local injection point with a wait callback that can be awaken by a different backend: s1: select injection_points_set_local(); s1: select injection_points_attach('popo', 'wait'); s1: select injection_points_run('popo'); -- waits s2: select injection_points_wakeup('popo'); s1: -- ready for action. A detach is not a wakeup. -- Michael
Вложения
> On 2 May 2024, at 13:43, Michael Paquier <michael@paquier.xyz> wrote: > > A detach is not a wakeup. Oh, now I see. Sorry for the noise. Detaching local injection point of other backend seems to be useless and can be forbidden. As far as I understand, your patch is already doing this in + if (!injection_point_allowed(name)) + elog(ERROR, "cannot detach injection point \"%s\" not allowed to run", + name); + As far as I understand this will effectively forbid calling injection_points_detach() for local injection point of otherbackend. Do I get it right? Best regards, Andrey Borodin.
On Thu, May 02, 2024 at 04:27:12PM +0900, Michael Paquier wrote: > On Wed, May 01, 2024 at 04:12:14PM -0700, Noah Misch wrote: > > While writing an injection point test, I encountered a variant of the race > > condition that f4083c4 fixed. It had three sessions and this sequence of > > events: > > > > s1: local-attach to POINT > > s2: enter InjectionPointRun(POINT), yield CPU just before injection_callback() > > s3: detach POINT, deleting the InjectionPointCondition record > > s2: wake up and run POINT as though it had been non-local I should have given a simpler example: s1: local-attach to POINT s2: enter InjectionPointRun(POINT), yield CPU just before injection_callback() s1: exit s2: wake up and run POINT as though it had been non-local > Fun. One thing I would ask is why it makes sense to be able to detach > a local point from a different session than the one who defined it as > local. Shouldn't the operation of s3 be restricted rather than > authorized as a safety measure, instead? (That's orthogonal to the race condition.) When s1 would wait at the injection point multiple times in one SQL statement, I like issuing the detach from s3 so s1 waits at just the first encounter with the injection point. This mimics setting a gdb breakpoint and deleting that breakpoint before "continue". The alternative, waking s1 repeatedly until it finishes the SQL statement, is less convenient. (I also patched _detach() to wake the waiter, and I plan to propose that.) > > Separately, injection_points_cleanup() breaks the rules by calling > > InjectionPointDetach() while holding a spinlock. The latter has an > > elog(ERROR), and reaching that elog(ERROR) leaves a stuck spinlock. I haven't > > given as much thought to solutions for this one. > > Indeed. That's a brain fade. This one could be fixed by collecting > the point names when cleaning up the conditions and detach after > releasing the spinlock. This opens a race condition between the > moment when the spinlock is released and the detach, where another > backend could come in and detach a point before the shmem_exit > callback has the time to do its cleanup, even if detach() is > restricted for local points. So we could do the callback cleanup in That race condition seems fine. The test can be expected to control the timing of backend exits vs. detach calls. Unlike the InjectionPointRun() race, it wouldn't affect backends unrelated to the test. > three steps in the shmem exit callback: > - Collect the names of the points to detach, while holding the > spinlock. > - Do the Detach. > - Take again the spinlock, clean up the conditions. > > Please see the attached. The injection_points_cleanup() parts look good. Thanks. > @@ -403,6 +430,10 @@ injection_points_detach(PG_FUNCTION_ARGS) > { > char *name = text_to_cstring(PG_GETARG_TEXT_PP(0)); > > + if (!injection_point_allowed(name)) > + elog(ERROR, "cannot detach injection point \"%s\" not allowed to run", > + name); > + As above, I disagree with the injection_points_detach() part.
On Thu, May 02, 2024 at 01:52:20PM +0500, Andrey M. Borodin wrote: > As far as I understand this will effectively forbid calling > injection_points_detach() for local injection point of other > backend. Do I get it right? Yes, that would be the intention. Noah has other use cases in mind with this interface that I did not think about supporting, hence he's objecting to this restriction. -- Michael
Вложения
On Thu, May 02, 2024 at 12:35:55PM -0700, Noah Misch wrote: > I should have given a simpler example: > > s1: local-attach to POINT > s2: enter InjectionPointRun(POINT), yield CPU just before injection_callback() > s1: exit > s2: wake up and run POINT as though it had been non-local Hmm. Even if you were to emulate that in a controlled manner, you would need a second injection point that does a wait in s2, which is something that would happen before injection_callback() and before scanning the local entry. This relies on the fact that we're holding CPU in s2 between the backend shmem hash table lookup and the callback being called. >> Fun. One thing I would ask is why it makes sense to be able to detach >> a local point from a different session than the one who defined it as >> local. Shouldn't the operation of s3 be restricted rather than >> authorized as a safety measure, instead? > > (That's orthogonal to the race condition.) When s1 would wait at the > injection point multiple times in one SQL statement, I like issuing the detach > from s3 so s1 waits at just the first encounter with the injection point. > This mimics setting a gdb breakpoint and deleting that breakpoint before > "continue". The alternative, waking s1 repeatedly until it finishes the SQL > statement, is less convenient. (I also patched _detach() to wake the waiter, > and I plan to propose that.) Okay. >> Indeed. That's a brain fade. This one could be fixed by collecting >> the point names when cleaning up the conditions and detach after >> releasing the spinlock. This opens a race condition between the >> moment when the spinlock is released and the detach, where another >> backend could come in and detach a point before the shmem_exit >> callback has the time to do its cleanup, even if detach() is >> restricted for local points. So we could do the callback cleanup in > > That race condition seems fine. The test can be expected to control the > timing of backend exits vs. detach calls. Unlike the InjectionPointRun() > race, it wouldn't affect backends unrelated to the test. Sure. The fact that there are two spinlocks in the backend code and the module opens concurrency issues. Making that more robust if there is a case for it is OK by me, but I'd rather avoid making the backend-side more complicated than need be. >> three steps in the shmem exit callback: >> - Collect the names of the points to detach, while holding the >> spinlock. >> - Do the Detach. >> - Take again the spinlock, clean up the conditions. >> >> Please see the attached. > > The injection_points_cleanup() parts look good. Thanks. Thanks for the check. >> @@ -403,6 +430,10 @@ injection_points_detach(PG_FUNCTION_ARGS) >> { >> char *name = text_to_cstring(PG_GETARG_TEXT_PP(0)); >> >> + if (!injection_point_allowed(name)) >> + elog(ERROR, "cannot detach injection point \"%s\" not allowed to run", >> + name); >> + > > As above, I disagree with the injection_points_detach() part. Okay, noted. Fine by me to expand this stuff as you feel, the code has been written to be extended depending on what people want to support. There should be tests in the tree that rely on any new behavior, though. I've applied the patch to fix the spinlock logic in the exit callback for now. -- Michael
Вложения
On Mon, May 06, 2024 at 10:03:37AM +0900, Michael Paquier wrote: > On Thu, May 02, 2024 at 12:35:55PM -0700, Noah Misch wrote: > > I should have given a simpler example: > > > > s1: local-attach to POINT > > s2: enter InjectionPointRun(POINT), yield CPU just before injection_callback() > > s1: exit > > s2: wake up and run POINT as though it had been non-local Here's how I've patched it locally. It does avoid changing the backend-side, which has some attraction. Shall I just push this? > Hmm. Even if you were to emulate that in a controlled manner, you > would need a second injection point that does a wait in s2, which is > something that would happen before injection_callback() and before > scanning the local entry. This relies on the fact that we're holding > CPU in s2 between the backend shmem hash table lookup and the callback > being called. Right. We would need "second-level injection points" to write a test for that race in the injection point system.
Вложения
On Mon, May 06, 2024 at 02:23:24PM -0700, Noah Misch wrote: > Here's how I've patched it locally. It does avoid changing the backend-side, > which has some attraction. Shall I just push this? It looks like you did not rebase on top of HEAD to avoid the spinlock taken with InjectionPointDetach() in the shmem callback. I think that you'd mean the attached, once rebased (apologies I've reset the author field). + * s1: local-attach to POINT + * s2: yield CPU before InjectionPointRun(POINT) calls injection_callback() + * s1: exit() + * s2: run POINT as though it had been non-local I see. So you are taking a shortcut in the shape of never resetting the name of a condition, so as it is possible to let the point of step 4 check the runtime condition with a condition still stored while the point has been detached, removed from the hash table. if (strcmp(condition->name, name) == 0) { + condition->valid = false; condition->pid = 0; - condition->name[0] = '\0'; } } As of HEAD, we rely on InjectionPointCondition->name to be set to check if a condition is valid. Your patch adds a different variable to do mostly the same thing, and never clears the name in any existing conditions. A side effect is that this causes the conditions to pile up on a running server when running installcheck, and assuming that many test suites are run on a server left running this could cause spurious failures when failing to find a new slot. Always resetting condition->name when detaching a point is a simpler flow and saner IMO. Overall, this switches from one detach behavior to a different one, which may or may not be intuitive depending on what one is looking for. FWIW, I see InjectionPointCondition as something that should be around as long as its injection point exists, with the condition entirely gone once the point is detached because it should not exist anymore on the server running, with no information left in shmem. Through your patch, you make conditions have a different meaning, with a mix of "local" definition, but it is kind of permanent as it keeps a trace of the point's name in shmem. I find the behavior of the patch less intuitive. Perhaps it would be interesting to see first the bug and/or problem you are trying to tackle with this different behavior as I feel like we could do something even with the module as-is. As far as I understand, the implementation of the module on HEAD allows one to emulate a breakpoint with a wait/wake, which can avoid the window mentioned in step 2. Even if a wait point is detached concurrently, it can be awaken with its traces in shmem removed. -- Michael
Вложения
On Tue, May 07, 2024 at 10:17:49AM +0900, Michael Paquier wrote: > On Mon, May 06, 2024 at 02:23:24PM -0700, Noah Misch wrote: > > Here's how I've patched it locally. It does avoid changing the backend-side, > > which has some attraction. Shall I just push this? > > It looks like you did not rebase on top of HEAD Yes, the base was 713cfaf (Sunday). > A side effect is that this causes the conditions to pile > up on a running server when running installcheck, and assuming that > many test suites are run on a server left running this could cause > spurious failures when failing to find a new slot. Yes, we'd be raising INJ_MAX_CONDITION more often under this approach. > Always resetting > condition->name when detaching a point is a simpler flow and saner > IMO. > > Overall, this switches from one detach behavior to a different one, Can you say more about that? The only behavior change known to me is that a given injection point workload uses more of INJ_MAX_CONDITION. If there's another behavior change, it was likely unintended. > which may or may not be intuitive depending on what one is looking > for. FWIW, I see InjectionPointCondition as something that should be > around as long as its injection point exists, with the condition > entirely gone once the point is detached because it should not exist > anymore on the server running, with no information left in shmem. > > Through your patch, you make conditions have a different meaning, with > a mix of "local" definition, but it is kind of permanent as it keeps a > trace of the point's name in shmem. I find the behavior of the patch > less intuitive. Perhaps it would be interesting to see first the bug > and/or problem you are trying to tackle with this different behavior > as I feel like we could do something even with the module as-is. As > far as I understand, the implementation of the module on HEAD allows > one to emulate a breakpoint with a wait/wake, which can avoid the > window mentioned in step 2. Even if a wait point is detached > concurrently, it can be awaken with its traces in shmem removed. The problem I'm trying to tackle in this thread is to make src/test/modules/gin installcheck-safe. $SUBJECT's commit 5105c90 started that work, having seen the intarray test suite break when run concurrently with the injection_points test suite. That combination still does break at the exit-time race condition. To reproduce, apply this attachment to add sleeps, and run: make -C src/test/modules/gin installcheck USE_MODULE_DB=1 & sleep 2; make -C contrib/intarray installcheck USE_MODULE_DB=1 Separately, I see injection_points_attach() populates InjectionPointCondition after InjectionPointAttach(). Shouldn't InjectionPointAttach() come last, to avoid the same sort of race? I've not tried to reproduce that one.
Вложения
On Tue, May 07, 2024 at 11:53:10AM -0700, Noah Misch wrote: > On Tue, May 07, 2024 at 10:17:49AM +0900, Michael Paquier wrote: > > Overall, this switches from one detach behavior to a different one, > > Can you say more about that? The only behavior change known to me is that a > given injection point workload uses more of INJ_MAX_CONDITION. If there's > another behavior change, it was likely unintended. I see patch inplace030-inj-exit-race-v1.patch does not fix the race seen with repro-inj-exit-race-v1.patch. I withdraw inplace030-inj-exit-race-v1.patch, and I withdraw the above question. > To reproduce, apply [repro-inj-exit-race-v1.patch] to add > sleeps, and run: > > make -C src/test/modules/gin installcheck USE_MODULE_DB=1 & sleep 2; make -C contrib/intarray installcheck USE_MODULE_DB=1 > > Separately, I see injection_points_attach() populates InjectionPointCondition > after InjectionPointAttach(). Shouldn't InjectionPointAttach() come last, to > avoid the same sort of race? I've not tried to reproduce that one.
On Tue, May 07, 2024 at 11:53:10AM -0700, Noah Misch wrote: > On Tue, May 07, 2024 at 10:17:49AM +0900, Michael Paquier wrote: >> Always resetting >> condition->name when detaching a point is a simpler flow and saner >> IMO. >> >> Overall, this switches from one detach behavior to a different one, > > Can you say more about that? The only behavior change known to me is that a > given injection point workload uses more of INJ_MAX_CONDITION. If there's > another behavior change, it was likely unintended. As far as I read the previous patch, the conditions stored in InjectionPointSharedState would never be really gone, even if the points are removed from InjectionPointHash. > The problem I'm trying to tackle in this thread is to make > src/test/modules/gin installcheck-safe. $SUBJECT's commit 5105c90 started > that work, having seen the intarray test suite break when run concurrently > with the injection_points test suite. That combination still does break at > the exit-time race condition. To reproduce, apply this attachment to add > sleeps, and run: > > make -C src/test/modules/gin installcheck USE_MODULE_DB=1 & sleep 2; make -C contrib/intarray installcheck USE_MODULE_DB=1 Thanks for that. I am not really sure how to protect that without a small cost in flexibility for the cases of detach vs run paths. This comes down to the fact that a custom routine could be run while it could be detached concurrently, removing any stuff a callback could depend on in the module. It was mentioned upthread to add to InjectionPointCacheEntry a fixed area of memory that modules could use to store some "status" data, but it would not close the run/detach race because a backend could still hold a pointer to a callback, with concurrent backends playing with the contents of InjectionPointCacheEntry (concurrent detaches and attaches that would cause the same entries to be reused). One way to close entirely the window would be to hold InjectionPointLock longer in InjectionPointRun() until the callback finishes or until it triggers an ERROR. This would mean that the case you've mentioned in [1] would change, by blocking the detach() of s3 until the callback of s2 finishes. We don't have tests in the tree that do any of that, so holding InjectionPointLock longer would not break anything on HEAD. A detach being possible while the callback is run is something I've considered as valid in d86d20f0ba79, but perhaps that was too cute of me, even more with the use case of local points. > Separately, I see injection_points_attach() populates InjectionPointCondition > after InjectionPointAttach(). Shouldn't InjectionPointAttach() come last, to > avoid the same sort of race? I've not tried to reproduce that one. Good point. You could run into the case of a concurrent backend running an injection point that should be local if waiting between InjectionPointAttach() and the condition getting registered in injection_points_attach(). That should be reversed. [1] https://www.postgresql.org/message-id/20240501231214.40@rfd.leadboat.com -- Michael
Вложения
On Thu, May 09, 2024 at 09:37:54AM +0900, Michael Paquier wrote: > On Tue, May 07, 2024 at 11:53:10AM -0700, Noah Misch wrote: > > The problem I'm trying to tackle in this thread is to make > > src/test/modules/gin installcheck-safe. $SUBJECT's commit 5105c90 started > > that work, having seen the intarray test suite break when run concurrently > > with the injection_points test suite. That combination still does break at > > the exit-time race condition. To reproduce, apply this attachment to add > > sleeps, and run: > > > > make -C src/test/modules/gin installcheck USE_MODULE_DB=1 & sleep 2; make -C contrib/intarray installcheck USE_MODULE_DB=1 > > Thanks for that. I am not really sure how to protect that without a > small cost in flexibility for the cases of detach vs run paths. This > comes down to the fact that a custom routine could be run while it > could be detached concurrently, removing any stuff a callback could > depend on in the module. > > It was mentioned upthread to add to InjectionPointCacheEntry a fixed > area of memory that modules could use to store some "status" data, but > it would not close the run/detach race because a backend could still > hold a pointer to a callback, with concurrent backends playing with > the contents of InjectionPointCacheEntry (concurrent detaches and > attaches that would cause the same entries to be reused). > One way to close entirely the window would be to hold > InjectionPointLock longer in InjectionPointRun() until the callback > finishes or until it triggers an ERROR. This would mean that the case > you've mentioned in [1] would change, by blocking the detach() of s3 > until the callback of s2 finishes. > [1] https://www.postgresql.org/message-id/20240501231214.40@rfd.leadboat.com Yes, that would be a loss for test readability. Also, I wouldn't be surprised if some test will want to attach POINT-B while POINT-A is in injection_wait(). Various options avoiding those limitations: 1. The data area formerly called a "status" area is immutable after attach. The core code copies it into a stack buffer to pass in a const callback argument. 2. InjectionPointAttach() returns an attachment serial number, and the callback receives that as an argument. injection_points.c would maintain an InjectionPointCondition for every still-attached serial number, including global attachments. Finding no condition matching the serial number argument means detach already happened and callback should do nothing. 3. Move the PID check into core code. 4. Separate the concept of "make ineligible to fire" from "detach", with stronger locking for detach. v1 pointed in this direction, though not using that terminology. 5. Injection point has multiple callbacks. At least one runs with the lock held and can mutate the "status" data. At least one runs without the lock. (1) is, I think, simple and sufficient. How about that?
On Wed, May 08, 2024 at 08:15:53PM -0700, Noah Misch wrote: > Yes, that would be a loss for test readability. Also, I wouldn't be surprised > if some test will want to attach POINT-B while POINT-A is in injection_wait(). Not impossible, still annoying with more complex scenarios. > Various options avoiding those limitations: > > 1. The data area formerly called a "status" area is immutable after attach. > The core code copies it into a stack buffer to pass in a const callback > argument. > > 3. Move the PID check into core code. The PID checks are specific to the module, and there could be much more conditions like running only in specific backend types (first example coming into mind), so I want to avoid that kind of knowledge in the backend. > (1) is, I think, simple and sufficient. How about that? That sounds fine to do that at the end.. I'm not sure how large this chunk area added to each InjectionPointEntry should be, though. 128B stored in each InjectionPointEntry would be more than enough I guess? Or more like 1024? The in-core module does not need much currently, but larger is likely better for pluggability. -- Michael
Вложения
On Thu, May 09, 2024 at 01:47:45PM +0900, Michael Paquier wrote: > That sounds fine to do that at the end.. I'm not sure how large this > chunk area added to each InjectionPointEntry should be, though. 128B > stored in each InjectionPointEntry would be more than enough I guess? > Or more like 1024? The in-core module does not need much currently, > but larger is likely better for pluggability. Actually, this is leading to simplifications in the module, giving me the attached: 4 files changed, 117 insertions(+), 134 deletions(-) So I like your suggestion. This version closes the race window between the shmem exit detach in backend A and a concurrent backend B running a point local to A so as B will never run the local point of A. However, it can cause a failure in the shmem exit callback of backend A if backend B does a detach of a point local to A because A tracks its local points with a static list in its TopMemoryContext, at least in the attached. The second case could be solved by tracking the list of local points in the module's InjectionPointSharedState, but is this case really worth the complications it adds in the module knowing that the first case would be solid enough? Perhaps not. Another idea I have for the second case is to make InjectionPointDetach() a bit "softer", by returning a boolean status rather than fail if the detach cannot be done, so as the shmem exit callback can still loop through the entries it has in store. It could always be possible that a concurrent backend does a detach followed by an attach with the same name, causing the shmem exit callback to drop a point it should not, but that's not really a plausible case IMO :) This stuff can be adjusted in subtle ways depending on the cases you are most interested in. What do you think? -- Michael
Вложения
On Thu, May 09, 2024 at 04:40:43PM +0900, Michael Paquier wrote: > So I like your suggestion. This version closes the race window > between the shmem exit detach in backend A and a concurrent backend B > running a point local to A so as B will never run the local point of > A. However, it can cause a failure in the shmem exit callback of > backend A if backend B does a detach of a point local to A because A > tracks its local points with a static list in its TopMemoryContext, at > least in the attached. The second case could be solved by tracking > the list of local points in the module's InjectionPointSharedState, > but is this case really worth the complications it adds in the module > knowing that the first case would be solid enough? Perhaps not. > > Another idea I have for the second case is to make > InjectionPointDetach() a bit "softer", by returning a boolean status > rather than fail if the detach cannot be done, so as the shmem exit > callback can still loop through the entries it has in store. The return-bool approach sounds fine. Up to you whether to do in this patch, else I'll do it when I add the test. > It could > always be possible that a concurrent backend does a detach followed by > an attach with the same name, causing the shmem exit callback to drop > a point it should not, but that's not really a plausible case IMO :) Agreed. It's reasonable to expect test cases to serialize backend exits, attach calls, and detach calls. If we need to fix that later, we can use attachment serial numbers. > --- a/src/test/modules/injection_points/injection_points.c > +++ b/src/test/modules/injection_points/injection_points.c > +typedef enum InjectionPointConditionType > +{ > + INJ_CONDITION_INVALID = 0, I'd name this INJ_CONDITION_UNCONDITIONAL or INJ_CONDITION_ALWAYS. INVALID sounds like a can't-happen event or an injection point that never runs. Otherwise, the patch looks good and makes src/test/modules/gin safe for installcheck. Thanks.
On Thu, May 09, 2024 at 04:39:00PM -0700, Noah Misch wrote: Thanks for the feedback. > The return-bool approach sounds fine. Up to you whether to do in this patch, > else I'll do it when I add the test. I see no reason to not change the signature of the routine now if we know that we're going to do it anyway in the future. I was shortly wondering if doing the same for InjectionpointAttach() would make sense, but it has more error states, so I'm not really tempted without an actual reason (cannot think of a case where I'd want to put more control into a module after a failed attach). >> It could >> always be possible that a concurrent backend does a detach followed by >> an attach with the same name, causing the shmem exit callback to drop >> a point it should not, but that's not really a plausible case IMO :) > > Agreed. It's reasonable to expect test cases to serialize backend exits, > attach calls, and detach calls. If we need to fix that later, we can use > attachment serial numbers. Okay by me. > I'd name this INJ_CONDITION_UNCONDITIONAL or INJ_CONDITION_ALWAYS. INVALID > sounds like a can't-happen event or an injection point that never runs. > Otherwise, the patch looks good and makes src/test/modules/gin safe for > installcheck. Thanks. INJ_CONDITION_ALWAYS sounds like a good compromise here. Attached is an updated patch for now, indented with a happy CI. I am still planning to look at that a second time on Monday with a fresher mind, in case I'm missing something now. -- Michael
Вложения
> On 10 May 2024, at 06:04, Michael Paquier <michael@paquier.xyz> wrote: > > Attached is an updated patch for now Can you, please, add some more comments regarding purpose of private data? I somewhat lost understanding of the discussion for a week or so. And I hoped to grasp the idea of private_data from resultingcode. But I cannot do so from current patch version... I see that you store condition in private_data. So "private" means that this is a data specific to extension, do I understandit right? As long as I started anyway, I also want to ask some more stupid questions: 1. Where is the border between responsibility of an extension and the core part? I mean can we define in simple words whatfunctionality must be in extension? 2. If we have some concurrency issues, why can't we just protect everything with one giant LWLock\SpinLock. We have somelocking model instead of serializing access from enter until exit. Most probably, this was discussed somewhere, but I could not find it. Thanks! Best regards, Andrey Borodin.
On Fri, May 10, 2024 at 10:04:17AM +0900, Michael Paquier wrote: > Attached is an updated patch for now, indented with a happy CI. I am > still planning to look at that a second time on Monday with a fresher > mind, in case I'm missing something now. This looks correct, and it works well in my tests. Thanks.
On Sun, May 12, 2024 at 10:48:51AM -0700, Noah Misch wrote: > This looks correct, and it works well in my tests. Thanks. Thanks for looking. While looking at it yesterday I've decided to split the change into two commits, one for the infra and one for the module. While doing so, I've noticed that the case of a private area passed as NULL was not completely correct as memcpy would be undefined. The open item has been marked as fixed. -- Michael
Вложения
On Sat, May 11, 2024 at 11:45:33AM +0500, Andrey M. Borodin wrote: > I see that you store condition in private_data. So "private" means > that this is a data specific to extension, do I understand it right? Yes, it is possible to pass down some custom data to the callbacks registered, generated in a module. One example would be more complex condition grammar, like a JSON-based thing. I don't really see the need for this amount of complexity in the tree yet, but one could do that outside the tree easily. > As long as I started anyway, I also want to ask some more stupid > questions: > 1. Where is the border between responsibility of an extension and > the core part? I mean can we define in simple words what > functionality must be in extension? Rule 0 I've been using here: keep the footprint on the backend as simple as possible. These have as absolute minimum requirement: - A function name. - A library name. - A point name. The private area contents and size are added to address the concurrency cases with runtime checks. I didn't see a strong use for that first, but Noah has been convincing enough with his use cases and the fact that the race between detach and run was not completely closed because we lacked consistency with the shmem hash table lookup. > 2. If we have some concurrency issues, why can't we just protect > everything with one giant LWLock\SpinLock. We have some locking > model instead of serializing access from enter until exit. This reduces the test infrastructure flexibility, because one may want to attach or detach injection points while a point is running. So it is by design that the LWLock protecting the shmem hash table is not hold when a point is running. This has been covered a bit upthread, and I want to be able to do that as well. -- Michael