Обсуждение: Injection points: preloading and runtime arguments
Hi all, I have a couple of extra toys for injection points in my bucket that I'd like to propose for integration in v18, based on some feedback I have received: 1) Preload an injection point into the backend-level cache without running it. This has come up because an injection point run for the first time needs to be loaded with load_external_function that internally does some allocations, and this would not work if the injection point is in a critical section. Being able to preload an injection point requires an extra macro, called INJECTION_POINT_PRELOAD. Perhaps "load" makes more sense as keyword, here. 2) Grab values at runtime from the code path where an injection point is run and give them to the callback. The case here is to be able to do some dynamic manipulation or a stack, reads of some runtime data or even decide of a different set of actions in a callback based on what the input has provided. One case that I've been playing with here is the dynamic manipulation of pages in specific code paths to stress internal checks, as one example. This introduces a 1-argument version, as multiple args could always be passed down to the callback within a structure. The in-core module injection_points is extended to provide a SQL interface to be able to do the preloading or define a callback with arguments. The two changes are split into patches of their own. These new facilities could be backpatched if there is a need for them in the future in stable branches, as these are aimed for tests and the changes do not introduce any ABI breakages with the existing APIs or the in-core module. Thoughts and comments are welcome. -- Michael
Вложения
Hi!
> On 20 May 2024, at 08:18, Michael Paquier <michael@paquier.xyz> wrote:
Both features look useful to me.
I've tried to rebase my test of CV sleep during multixact generation[0]. I used it like this:
INJECTION_POINT_PRELOAD("GetNewMultiXactId-done");
multi = GetNewMultiXactId(nmembers, &offset); // starts critsection
INJECTION_POINT("GetNewMultiXactId-done");
And it fails like this:
2024-05-20 16:50:40.430 +05 [21830] 001_multixact.pl LOG: statement: select test_create_multixact();
TRAP: failed Assert("CritSectionCount == 0 || (context)->allowInCritSection"), File: "mcxt.c", Line: 1185, PID: 21830
0 postgres 0x0000000101452ed0 ExceptionalCondition + 220
1 postgres 0x00000001014a6050 MemoryContextAlloc + 208
2 postgres 0x00000001011c3bf0 dsm_create_descriptor + 72
3 postgres 0x00000001011c3ef4 dsm_attach + 400
4 postgres 0x00000001014990d8 dsa_attach + 24
5 postgres 0x00000001011c716c init_dsm_registry + 240
6 postgres 0x00000001011c6e60 GetNamedDSMSegment + 456
7 injection_points.dylib 0x0000000101c871f8 injection_init_shmem + 60
8 injection_points.dylib 0x0000000101c86f1c injection_wait + 64
9 postgres 0x000000010148e228 InjectionPointRunInternal + 376
10 postgres 0x000000010148e0a4 InjectionPointRun + 32
11 postgres 0x0000000100cab798 MultiXactIdCreateFromMembers + 344
12 postgres 0x0000000100cab604 MultiXactIdCreate + 312
Am I doing something wrong? Seems like extension have to know too that it is preloaded.
Best regards, Andrey Borodin.
[0] https://www.postgresql.org/message-id/0925F9A9-4D53-4B27-A87E-3D83A757B0E0%40yandex-team.ru
> On 20 May 2024, at 17:01, Andrey M. Borodin <x4mmm@yandex-team.ru> wrote: Ugh, accidentally send without attaching the patch itself. Sorry for the noise. Best regards, Andrey Borodin.
Вложения
On Mon, May 20, 2024 at 05:01:15PM +0500, Andrey M. Borodin wrote:
> Both features look useful to me.
> I've tried to rebase my test of CV sleep during multixact generation[0]. I used it like this:
>
> INJECTION_POINT_PRELOAD("GetNewMultiXactId-done");
> multi = GetNewMultiXactId(nmembers, &offset); // starts critsection
> INJECTION_POINT("GetNewMultiXactId-done");
Thanks for the feedback.
> And it fails like this:
>
> 2024-05-20 16:50:40.430 +05 [21830] 001_multixact.pl LOG: statement: select test_create_multixact();
> TRAP: failed Assert("CritSectionCount == 0 || (context)->allowInCritSection"), File: "mcxt.c", Line: 1185, PID: 21830
> 0 postgres 0x0000000101452ed0 ExceptionalCondition + 220
> 1 postgres 0x00000001014a6050 MemoryContextAlloc + 208
> 2 postgres 0x00000001011c3bf0 dsm_create_descriptor + 72
> 3 postgres 0x00000001011c3ef4 dsm_attach + 400
> 4 postgres 0x00000001014990d8 dsa_attach + 24
> 5 postgres 0x00000001011c716c init_dsm_registry + 240
> 6 postgres 0x00000001011c6e60 GetNamedDSMSegment + 456
> 7 injection_points.dylib 0x0000000101c871f8 injection_init_shmem + 60
> 8 injection_points.dylib 0x0000000101c86f1c injection_wait + 64
> 9 postgres 0x000000010148e228 InjectionPointRunInternal + 376
> 10 postgres 0x000000010148e0a4 InjectionPointRun + 32
> 11 postgres 0x0000000100cab798 MultiXactIdCreateFromMembers + 344
> 12 postgres 0x0000000100cab604 MultiXactIdCreate + 312
>
> Am I doing something wrong? Seems like extension have to know too that it is preloaded.
Your stack is pointing at the shared memory section initialized in the
module injection_points, which is a bit of a chicken-and-egg problem
because you'd want an extra preload to happen before even that, like a
pre-preload. From what I can see, you have a good point about the
shmem initialized in the module: injection_points_preload() should
call injection_init_shmem() so as this area would not trigger the
assertion.
However, there is a second thing here inherent to your test: shouldn't
the script call injection_points_preload() to make sure that the local
cache behind GetNewMultiXactId-done is fully allocated and prepared
for the moment where injection point will be run?
So I agree that 0002 ought to call injection_init_shmem() when calling
injection_points_preload(), but it also seems to me that the test is
missing the fact that it should heat the backend cache to avoid the
allocations in the critical sections.
Note that I disagree with taking a shortcut in the backend-side
injection point code where we would bypass CritSectionCount or
allowInCritSection. These states should stay consistent for the sake
of the callbacks registered so as these can rely on the same stack and
conditions as the code where they are called.
--
Michael
Вложения
> On 21 May 2024, at 06:31, Michael Paquier <michael@paquier.xyz> wrote:
>
> So I agree that 0002 ought to call injection_init_shmem() when calling
> injection_points_preload(), but it also seems to me that the test is
> missing the fact that it should heat the backend cache to avoid the
> allocations in the critical sections.
>
> Note that I disagree with taking a shortcut in the backend-side
> injection point code where we would bypass CritSectionCount or
> allowInCritSection. These states should stay consistent for the sake
> of the callbacks registered so as these can rely on the same stack and
> conditions as the code where they are called.
Currently I'm working on the test using this
$creator->query_until(qr/start/, q(
\echo start
select injection_points_wakeup('');
select test_create_multixact();
));
I'm fine if instead of injection_points_wakeup('') I'll have to use select injection_points_preload('point name');.
Thanks!
Best regards, Andrey Borodin.
On Tue, May 21, 2024 at 04:29:54PM +0500, Andrey M. Borodin wrote:
> Currently I'm working on the test using this
> $creator->query_until(qr/start/, q(
> \echo start
> select injection_points_wakeup('');
> select test_create_multixact();
> ));
>
> I'm fine if instead of injection_points_wakeup('') I'll have to use
> select injection_points_preload('point name');.
Based on our discussion of last week, please find attached the
promised patch set to allow your SLRU tests to work. I have reversed
the order of the patches, moving the loading part in 0001 and the
addition of the runtime arguments in 0002 as we have a use-case for
the loading, nothing yet for the runtime arguments.
I have also come back to the naming, feeling that "preload" was
overcomplicated. So I have used the word "load" instead across the
board for 0001.
Note that the SQL function injection_points_load() does now an
initialization of the shmem area when a process plugs into the module
for the first time, fixing the issue you have mentioned with your SLRU
test. Hence, you should be able to do a load(), then a wait in the
critical section as there would be no memory allocation done when the
point runs. Another thing you could do is to define a
INJECTION_POINT_LOAD() in the code path you're stressing outside the
critical section where the point is run. This should save from a call
to the SQL function. This choice is up to the one implementing the
test, both can be useful depending on what one is trying to achieve.
--
Michael
Вложения
> On 5 Jun 2024, at 03:52, Michael Paquier <michael@paquier.xyz> wrote:
>
> Another thing you could do is to define a
> INJECTION_POINT_LOAD() in the code path you're stressing outside the
> critical section where the point is run. This should save from a call
> to the SQL function. This choice is up to the one implementing the
> test, both can be useful depending on what one is trying to achieve.
Thanks!
Interestingly, previously having INJECTION_POINT_PRELOAD() was not enough.
But now both INJECTION_POINT_LOAD() or injection_points_load() do the trick, so for me any of them is enough.
My test works with current version, but I have one slight problem, I need to call
$node->safe_psql('postgres', q(select injection_points_detach('GetMultiXactIdMembers-CV-sleep')));
Before
$node->safe_psql('postgres', q(select injection_points_wakeup('GetMultiXactIdMembers-CV-sleep')));
Is it OK to detach() before wakeup()? Or, perhaps, can a detach() do a wakeup() automatically?
Best regards, Andrey Borodin.
On Thu, Jun 06, 2024 at 03:47:47PM +0500, Andrey M. Borodin wrote: > Is it OK to detach() before wakeup()? Or, perhaps, can a detach() do a wakeup() automatically? It is OK to do a detach before a wakeup. Noah has been relying on this behavior in an isolation test for a patch he's worked on. See inplace110-successors-v1.patch here: https://www.postgresql.org/message-id/20240512232923.aa.nmisch@google.com That's also something we've discussed for 33181b48fd0e, where Noah wanted to emulate in an automated fashion what one can do with a debugger and one or more breakpoints. Not sure that wakeup() involving a automated detach() is the behavior to hide long-term, actually, as there is also an argument for waking up a point and *not* detach it to force multiple waits. -- Michael
Вложения
> On 7 Jun 2024, at 04:38, Michael Paquier <michael@paquier.xyz> wrote: Thanks Michael! Tests of injection points with injection points are neat :) Alvaro, here’s the test for multixact CV sleep that I was talking about on PGConf. It is needed to test [0]. It is based on loaded injection points. This technique is not committed yet, but the patch looksgood. When all prerequisites are ready I will post it to corresponding thread and create CF item. Thanks! Best regards, Andrey Borodin. [0] https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=a0e0fb1ba
Вложения
On Sat, Jun 08, 2024 at 04:52:25PM +0500, Andrey M. Borodin wrote:
> Alvaro, here’s the test for multixact CV sleep that I was talking
> about on PGConf.
> It is needed to test [0]. It is based on loaded injection
> points.
> This technique is not committed yet, but the patch looks good.
OK, cool. I'll try to get that into the tree once v18 opens up. I
can see that GetNewMultiXactId() opens a critical section. I am
slightly surprised that you need both the SQL function
injection_points_load() and the macro INJECTION_POINT_LOAD().
Wouldn't one or the other be sufficient?
The test takes 20ms to run here, which is good enough.
+ INJECTION_POINT_LOAD("GetNewMultiXactId-done");
[...]
+ INJECTION_POINT("GetNewMultiXactId-done");
[...]
+ INJECTION_POINT("GetMultiXactIdMembers-CV-sleep");
Be careful about the naming here. All the points use lower case
characters currently.
+# and another multixact have no offest yet, we must wait until this offset
s/offest/offset/.
> When all prerequisites are ready I will post it to corresponding
> thread and create CF item.
OK, let's do that.
--
Michael