Injection points: some tools to wait and wake

On Mon, Feb 19, 2024 at 03:01:40PM +0900, Michael Paquier wrote:

0002 is a polished version of the TAP test that makes use of this
facility, providing coverage for the bug fixed by 7863ee4def65
(reverting this commit causes the test to fail), where a restart point
runs across a promotion request. The trick is to stop the
checkpointer in the middle of a restart point and issue a promotion
in-between.

The CF bot has been screaming at this one on Windows because the
process started with IPC::Run::start was not properly finished, so
attached is an updated version to bring that back to green.
--
Michael

On 19 Feb 2024, at 09:01, Michael Paquier <michael@paquier.xyz> wrote:

Thoughts and comments are welcome.

Hi Michael,

thanks for your work on injection points! I want to test a bunch of stuff using this facility.

I have a wishlist of functionality that I'd like to see in injection points. I hope you will find some of these ideas useful to improve the feature.
1. injection_points_wake() will wake all of waiters. But it's not suitable for complex tests. I think there must be a way to wake only specific waiter by injection point name.
2. Alexander Korotkov's stopevents could be used in isolation tests. This kind of tests is perfect for describing complex race conditions. (as a side note, I'd be happy if we could have primary\standby in isolation tests too)
3. Can we have some Perl function for this?
+# Wait until the checkpointer is in the middle of the restart point
+# processing, relying on the custom wait event generated in the
+# wait callback used in the injection point previously attached.
+ok( $node_standby->poll_query_until(
+		'postgres',
+		qq[SELECT count(*) FROM pg_stat_activity
+           WHERE backend_type = 'checkpointer' AND wait_event = 'injection_wait' ;],
+		'1'),
+	'checkpointer is waiting in restart point'
+) or die "Timed out while waiting for checkpointer to run restart point";

Perhaps something like
$node->do_a_query_and_wait_for_injection_point_observed(sql,injection_point_name);
4. Maybe I missed it, but I'd like to see a guideline on how to name injection points.
5. In many cases we need to have injection point under critical section. I propose to have a "prepared injection point". See [0] for example in v2-0003-Test-multixact-CV-sleep.patch
+   INJECTION_POINT_PREPARE("GetNewMultiXactId-done");
+
    START_CRIT_SECTION();

+ INJECTION_POINT_RUN_PREPARED();
6. Currently our codebase have files injection_point.c and injection_points.c. It's very difficult to remember which is where...
7. This is extremely distant, but some DBMSs allow to enable injection points by placing files on the filesystem. That would allow to test something during recovery when no SQL interface is present.

Let's test all the neat stuff! Thank you!

Best regards, Andrey Borodin.

[0]: /messages/by-id/0925F9A9-4D53-4B27-A87E-3D83A757B0E0@yandex-team.ru

Hi,

On Mon, Feb 19, 2024 at 04:51:45PM +0900, Michael Paquier wrote:

On Mon, Feb 19, 2024 at 03:01:40PM +0900, Michael Paquier wrote:

0002 is a polished version of the TAP test that makes use of this
facility, providing coverage for the bug fixed by 7863ee4def65
(reverting this commit causes the test to fail), where a restart point
runs across a promotion request. The trick is to stop the
checkpointer in the middle of a restart point and issue a promotion
in-between.

The CF bot has been screaming at this one on Windows because the
process started with IPC::Run::start was not properly finished, so
attached is an updated version to bring that back to green.

Thanks for the patch, that's a very cool feature!

Random comments:

1 ===

+CREATE FUNCTION injection_points_wake()

what about injection_points_wakeup() instead?

2 ===
+/* Shared state information for injection points. */
+typedef struct InjectionPointSharedState
+{
+       /* protects accesses to wait_counts */
+       slock_t         lock;
+
+       /* Counter advancing when injection_points_wake() is called */
+       int                     wait_counts;

If slock_t protects "only" one counter, then what about using pg_atomic_uint64
or pg_atomic_uint32 instead? And btw do we need wait_counts at all? (see comment
number 4)

3 ===

+ * SQL function for waking a condition variable

waking up instead?

4 ===

+       for (;;)
+       {
+               int                     new_wait_counts;
+
+               SpinLockAcquire(&inj_state->lock);
+               new_wait_counts = inj_state->wait_counts;
+               SpinLockRelease(&inj_state->lock);
+
+               if (old_wait_counts != new_wait_counts)
+                       break;
+               ConditionVariableSleep(&inj_state->wait_point, injection_wait_event);
+       }

I'm wondering if this loop and wait_counts are needed, why not doing something
like (and completely get rid of wait_counts)?

"
ConditionVariablePrepareToSleep(&inj_state->wait_point);
ConditionVariableSleep(&inj_state->wait_point, injection_wait_event);
ConditionVariableCancelSleep();
"

It's true that the comment above ConditionVariableSleep() mentions that:

"
* This should be called in a predicate loop that tests for a specific exit
* condition and otherwise sleeps
"

but is it needed in our particular case here?

Regards,

--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com

On Mon, Feb 19, 2024 at 11:54:20AM +0300, Andrey M. Borodin wrote:

1. injection_points_wake() will wake all of waiters. But it's not
suitable for complex tests. I think there must be a way to wake only
specific waiter by injection point name.

I don't disagree with that, but I don't have a strong argument for
implementing that until there is an explicit need for it in core. It
is also possible to plug in your own module, outside of core, if you
are looking for something more specific. The backend APIs allow that.

2. Alexander Korotkov's stopevents could be used in isolation
tests. This kind of tests is perfect for describing complex race
conditions. (as a side note, I'd be happy if we could have
primary\standby in isolation tests too)

This requires plugging is more into src/test/isolation/, with multiple
connection strings. This has been suggested in the past.

3. Can we have some Perl function for this?
+# Wait until the checkpointer is in the middle of the restart point
+# processing, relying on the custom wait event generated in the
+# wait callback used in the injection point previously attached.
+ok( $node_standby->poll_query_until(
+		'postgres',
+		qq[SELECT count(*) FROM pg_stat_activity
+           WHERE backend_type = 'checkpointer' AND wait_event = 'injection_wait' ;],
+		'1'),
+	'checkpointer is waiting in restart point'
+) or die "Timed out while waiting for checkpointer to run restart point";

Perhaps something like
$node->do_a_query_and_wait_for_injection_point_observed(sql,injection_point_name);

I don't see why not. But I'm not sure how much I need to plug in into
the main modules yet.

4. Maybe I missed it, but I'd like to see a guideline on how to name
injection points.

I don't think we have any of that, or even that we need one. In
short, I'm OK to be more consistent with the choice of ginbtree.c and
give priority to it and make it more official in the docs.

5. In many cases we need to have injection point under critical
section. I propose to have a "prepared injection point". See [0] for
example in v2-0003-Test-multixact-CV-sleep.patch
+   INJECTION_POINT_PREPARE("GetNewMultiXactId-done");
+
START_CRIT_SECTION();

+ INJECTION_POINT_RUN_PREPARED();

I don't see how that's different from a wait/wake logic? The only
thing you've changed is to stop a wait when a point is detached and
you want to make the stop conditional. Plugging in a condition
variable is more flexible than a hardcoded sleep in terms of wait,
while being more responsive.

7. This is extremely distant, but some DBMSs allow to enable
injection points by placing files on the filesystem. That would
allow to test something during recovery when no SQL interface is
present.

Yeah, I could see why you'd want to do something like that. If a use
case pops up, that can surely be discussed.
--
Michael

On Mon, Feb 19, 2024 at 02:28:04PM +0000, Bertrand Drouvot wrote:

+CREATE FUNCTION injection_points_wake()

what about injection_points_wakeup() instead?

Sure.

+/* Shared state information for injection points. */
+typedef struct InjectionPointSharedState
+{
+       /* protects accesses to wait_counts */
+       slock_t         lock;
+
+       /* Counter advancing when injection_points_wake() is called */
+       int                     wait_counts;

If slock_t protects "only" one counter, then what about using pg_atomic_uint64
or pg_atomic_uint32 instead? And btw do we need wait_counts at all? (see comment
number 4)

We could, indeed, even if we use more than one counter. Still, I
would be tempted to keep it in case more data is added to this
structure as that would make for less stuff to do while being normally
non-critical. This sentence may sound pedantic, though..

+ * SQL function for waking a condition variable

waking up instead?

Okay.

I'm wondering if this loop and wait_counts are needed, why not doing something
like (and completely get rid of wait_counts)?

"
ConditionVariablePrepareToSleep(&inj_state->wait_point);
ConditionVariableSleep(&inj_state->wait_point, injection_wait_event);
ConditionVariableCancelSleep();
"

It's true that the comment above ConditionVariableSleep() mentions that:

Perhaps not, but it encourages good practices around the use of
condition variables, and we need to track all that in shared memory
anyway. Ashutosh has argued in favor of the approach taken by the
patch in the original thread when I've sent a version doing exactly
what you are saying now to not track a state in shmem.
--
Michael

On 20 Feb 2024, at 02:21, Michael Paquier <michael@paquier.xyz> wrote:

On Mon, Feb 19, 2024 at 11:54:20AM +0300, Andrey M. Borodin wrote:

1. injection_points_wake() will wake all of waiters. But it's not
suitable for complex tests. I think there must be a way to wake only
specific waiter by injection point name.

I don't disagree with that, but I don't have a strong argument for
implementing that until there is an explicit need for it in core. It
is also possible to plug in your own module, outside of core, if you
are looking for something more specific. The backend APIs allow that.

In [0]/messages/by-id/0925F9A9-4D53-4B27-A87E-3D83A757B0E0@yandex-team.ru I want to create a test for edge case of reading recent multixact. External module does not allow to have a test within repository.
In that test I need to sync backends in 3 steps (backend 1 starts to wait in injection point, backend 2 starts to sleep in other point, backend 1 is released and observe 3rd injection point). "wake them all" implementation allows only 2-step synchronization.
I will try to simplify test to 2-step, but it would be much easier to implement if injection points could be awaken independently.

2. Alexander Korotkov's stopevents could be used in isolation
tests. This kind of tests is perfect for describing complex race
conditions. (as a side note, I'd be happy if we could have
primary\standby in isolation tests too)

This requires plugging is more into src/test/isolation/, with multiple
connection strings. This has been suggested in the past.

I think standby isolation tests are just a sugar-on-top feature here.
Wrt injection points, I'd like to see a function to wait until some injection point is observed.
With this function at hand developer can implement race condition tests as an isolation test.

5. In many cases we need to have injection point under critical
section. I propose to have a "prepared injection point". See [0] for
example in v2-0003-Test-multixact-CV-sleep.patch
+   INJECTION_POINT_PREPARE("GetNewMultiXactId-done");
+
START_CRIT_SECTION();

+ INJECTION_POINT_RUN_PREPARED();

I don't see how that's different from a wait/wake logic? The only
thing you've changed is to stop a wait when a point is detached and
you want to make the stop conditional. Plugging in a condition
variable is more flexible than a hardcoded sleep in terms of wait,
while being more responsive.

No, "prepared injection point" is not about wait\wake logic. It's about having injection point in critical section.
Normal injection point will pstrdup(name) and fail. In [0]/messages/by-id/0925F9A9-4D53-4B27-A87E-3D83A757B0E0@yandex-team.ru I need a test that waits after multixact generation before WAL-logging it. It's only possible in a critical section.

Thanks!

Best regards, Andrey Borodin.

[0]: /messages/by-id/0925F9A9-4D53-4B27-A87E-3D83A757B0E0@yandex-team.ru

Hi,

On Tue, Feb 20, 2024 at 08:28:28AM +0900, Michael Paquier wrote:

On Mon, Feb 19, 2024 at 02:28:04PM +0000, Bertrand Drouvot wrote:

If slock_t protects "only" one counter, then what about using pg_atomic_uint64
or pg_atomic_uint32 instead? And btw do we need wait_counts at all? (see comment
number 4)

We could, indeed, even if we use more than one counter. Still, I
would be tempted to keep it in case more data is added to this
structure as that would make for less stuff to do while being normally
non-critical. This sentence may sound pedantic, though..

Okay, makes sense to keep this as it is as a "template" in case more stuff is
added.

+       /* Counter advancing when injection_points_wake() is called */
+       int                     wait_counts;

In that case what about using an unsigned instead? (Nit)

I'm wondering if this loop and wait_counts are needed, why not doing something
like (and completely get rid of wait_counts)?

"
ConditionVariablePrepareToSleep(&inj_state->wait_point);
ConditionVariableSleep(&inj_state->wait_point, injection_wait_event);
ConditionVariableCancelSleep();
"

It's true that the comment above ConditionVariableSleep() mentions that:

Perhaps not, but it encourages good practices around the use of
condition variables, and we need to track all that in shared memory
anyway. Ashutosh has argued in favor of the approach taken by the
patch in the original thread when I've sent a version doing exactly
what you are saying now to not track a state in shmem.

Oh okay I missed this previous discussion, let's keep it as it is then.

New comments:

1 ===

+void
+injection_wait(const char *name)

Looks like name is not used in the function. I guess the reason it is a parameter
is because that's the way the callback function is being called in
InjectionPointRun()?

2 ===

+PG_FUNCTION_INFO_V1(injection_points_wake);
+Datum
+injection_points_wake(PG_FUNCTION_ARGS)
+{

I think that This function will wake up all the "wait" injection points.
Would that make sense to implement some filtering based on the name? That could
be useful for tests that would need multiple wait injection points and that want
to wake them up "sequentially".

We could think about it if there is such a need in the future though.

3 ===

+# Register a injection point on the standby so as the follow-up

typo: "an injection"?

4 ===

+for (my $i = 0; $i < 3000; $i++)
+{

is 3000 due to?:

+checkpoint_timeout = 30s

If so, would that make sense to reduce the value for both?

Regards,

--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com

On Tue, Feb 20, 2024 at 03:55:08PM +0000, Bertrand Drouvot wrote:

Okay, makes sense to keep this as it is as a "template" in case more stuff is
added.

+       /* Counter advancing when injection_points_wake() is called */
+       int                     wait_counts;

In that case what about using an unsigned instead? (Nit)

uint32. Sure.

1 ===

+void
+injection_wait(const char *name)

Looks like name is not used in the function. I guess the reason it is a parameter
is because that's the way the callback function is being called in
InjectionPointRun()?

Right. The callback has to define this argument.

2 ===

+PG_FUNCTION_INFO_V1(injection_points_wake);
+Datum
+injection_points_wake(PG_FUNCTION_ARGS)
+{

I think that This function will wake up all the "wait" injection points.
Would that make sense to implement some filtering based on the name? That could
be useful for tests that would need multiple wait injection points and that want
to wake them up "sequentially".

We could think about it if there is such a need in the future though.

Well, both you and Andrey are asking for it now, so let's do it. The
implementation is simple:
- Store in InjectionPointSharedState an array of wait_counts and an
array of names. There is only one condition variable.
- When a point wants to wait, it takes the spinlock and looks within
the array of names until it finds a free slot, adds its name into the
array to reserve a wait counter at the same position, releases the
spinlock. Then it loops on the condition variable for an update of
the counter it has reserved. It is possible to make something more
efficient, but at a small size it would not really matter.
- The wakeup takes a point name in argument, acquires the spinlock,
and checks if it can find the point into the array, pinpoints the
location of the counter to update and updates it. Then it broadcasts
the change.
- The wait loop checks its counter, leaves its loop, cancels the
sleep, takes the spinlock to unregister from the array, and leaves.

I would just hardcode the number of points that can wait, say 5 of
them tracked in shmem? Does that look like what you are looking at?

+# Register a injection point on the standby so as the follow-up

typo: "an injection"?

Oops. Fixed locally.

+for (my $i = 0; $i < 3000; $i++)
+{

is 3000 due to?:

+checkpoint_timeout = 30s

If so, would that make sense to reduce the value for both?

That had better be based on PostgreSQL::Test::Utils::timeout_default,
actually, as in something like:
foreach my $i (0 .. 10 * $PostgreSQL::Test::Utils::timeout_default)
--
Michael

On Tue, Feb 20, 2024 at 05:32:38PM +0300, Andrey M. Borodin wrote:

I will try to simplify test to 2-step, but it would be much easier
to implement if injection points could be awaken independently.

I don't mind implementing something that wakes only a given point
name, that's just more state data to track in shmem for the module.

No, "prepared injection point" is not about wait\wake logic. It's
about having injection point in critical section.
Normal injection point will pstrdup(name) and fail. In [0] I need a
test that waits after multixact generation before WAL-logging
it. It's only possible in a critical section.

It took me some time to understand what you mean here. You are
referring to the allocations done in load_external_function() ->
expand_dynamic_library_name() -> substitute_libpath_macro(), which
is something that has to happen the first time a callback is loaded
into the local cache of a process. So what you want to achieve is to
preload the callback in its cache in a first step without running it,
then be able to run it, so your Prepare[d] layer is just a way to
split InjectionPointRun() into two. Fancy use case. You could
disable the critical section around the INJECTION_POINT() as one
solution, though having something to pre-warm the local cache for a
point name and avoid the allocations done in the external load would
be a second way to achieve that.

"Prepare" is not the best term I would have used, perhaps just have
one INJECTION_POINT_PRELOAD() macro to warm up the cache outside the
critical section with a wrapper routine? You could then leave
InjectionPointRun() as it is now. Having a check on CritSectionCount
in the injection point internals to disable temporarily a critical
section would not feel right as this is used as a safety check
anywhere else.
--
Michael

On Wed, Feb 21, 2024 at 07:08:03AM +0900, Michael Paquier wrote:

Well, both you and Andrey are asking for it now, so let's do it. The
implementation is simple:
- Store in InjectionPointSharedState an array of wait_counts and an
array of names. There is only one condition variable.
- When a point wants to wait, it takes the spinlock and looks within
the array of names until it finds a free slot, adds its name into the
array to reserve a wait counter at the same position, releases the
spinlock. Then it loops on the condition variable for an update of
the counter it has reserved. It is possible to make something more
efficient, but at a small size it would not really matter.
- The wakeup takes a point name in argument, acquires the spinlock,
and checks if it can find the point into the array, pinpoints the
location of the counter to update and updates it. Then it broadcasts
the change.
- The wait loop checks its counter, leaves its loop, cancels the
sleep, takes the spinlock to unregister from the array, and leaves.

I would just hardcode the number of points that can wait, say 5 of
them tracked in shmem? Does that look like what you are looking at?

I was looking at that, and it proves to work OK, so you can do stuff
like waits and wakeups for multiple processes in a controlled manner.
The attached patch authorizes up to 32 waiters. I have switched
things so as the information reported in pg_stat_activity is the name
of the injection point itself.

Comments and ideas are welcome.
--
Michael

Hi,

On Wed, Feb 21, 2024 at 07:08:03AM +0900, Michael Paquier wrote:

On Tue, Feb 20, 2024 at 03:55:08PM +0000, Bertrand Drouvot wrote:

+PG_FUNCTION_INFO_V1(injection_points_wake);
+Datum
+injection_points_wake(PG_FUNCTION_ARGS)
+{

I think that This function will wake up all the "wait" injection points.
Would that make sense to implement some filtering based on the name? That could
be useful for tests that would need multiple wait injection points and that want
to wake them up "sequentially".

We could think about it if there is such a need in the future though.

Well, both you and Andrey are asking for it now, so let's do it.

Thanks!

The implementation is simple:
- Store in InjectionPointSharedState an array of wait_counts and an
array of names. There is only one condition variable.
- When a point wants to wait, it takes the spinlock and looks within
the array of names until it finds a free slot, adds its name into the
array to reserve a wait counter at the same position, releases the
spinlock. Then it loops on the condition variable for an update of
the counter it has reserved. It is possible to make something more
efficient, but at a small size it would not really matter.
- The wakeup takes a point name in argument, acquires the spinlock,
and checks if it can find the point into the array, pinpoints the
location of the counter to update and updates it. Then it broadcasts
the change.
- The wait loop checks its counter, leaves its loop, cancels the
sleep, takes the spinlock to unregister from the array, and leaves.

I think that makes sense and now the "counter" makes more sense to me (thanks to
it we don't need multiple CV).

I would just hardcode the number of points that can wait, say 5 of
them tracked in shmem? Does that look like what you are looking at?

I think so yes and more than 5 points would look like a complicated test IMHO.

Regards,

--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com

Hi,

On Wed, Feb 21, 2024 at 04:46:00PM +0900, Michael Paquier wrote:

On Wed, Feb 21, 2024 at 07:08:03AM +0900, Michael Paquier wrote:

Well, both you and Andrey are asking for it now, so let's do it. The
implementation is simple:
- Store in InjectionPointSharedState an array of wait_counts and an
array of names. There is only one condition variable.
- When a point wants to wait, it takes the spinlock and looks within
the array of names until it finds a free slot, adds its name into the
array to reserve a wait counter at the same position, releases the
spinlock. Then it loops on the condition variable for an update of
the counter it has reserved. It is possible to make something more
efficient, but at a small size it would not really matter.
- The wakeup takes a point name in argument, acquires the spinlock,
and checks if it can find the point into the array, pinpoints the
location of the counter to update and updates it. Then it broadcasts
the change.
- The wait loop checks its counter, leaves its loop, cancels the
sleep, takes the spinlock to unregister from the array, and leaves.

I would just hardcode the number of points that can wait, say 5 of
them tracked in shmem? Does that look like what you are looking at?

I was looking at that, and it proves to work OK, so you can do stuff
like waits and wakeups for multiple processes in a controlled manner.
The attached patch authorizes up to 32 waiters. I have switched
things so as the information reported in pg_stat_activity is the name
of the injection point itself.

Thanks!

I think the approach is fine and the hardcoded value is "large" enough (it would
be surprising, at least to me, to write a test that would need more than 32
waiters).

A few comments:

1 ===

+-- Wakes a condition variable

I think "up" is missing at several places in the patch where "wake" is used.
I could be wrong as non native english speaker though.

2 ===

+       /* Counters advancing when injection_points_wakeup() is called */
+       int                     wait_counts[INJ_MAX_WAIT];

uint32? (here and other places where counter is manipulated).

3 ===

+ /* Remove us from the waiting list */

"Remove this injection wait name from the waiting list" instead?

4 ===

+ * SQL function for waking a condition variable.

s/a condition variable/an injection wait point/ ?

5 ===

+PG_FUNCTION_INFO_V1(injection_points_wakeup);
+Datum
+injection_points_wakeup(PG_FUNCTION_ARGS)

Empty line missing before "Datum"?

6 ===

Also maybe some comments are missing above injection_point_init_state(),
injection_init_shmem() but it's more a Nit.

7 ===

While at it, should we add a second injection wait point in
t/041_invalid_checkpoint_after_promote.pl to check that they are wake up
individually?

Regards,

--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com

On Wed, Feb 21, 2024 at 11:50:21AM +0000, Bertrand Drouvot wrote:

I think the approach is fine and the hardcoded value is "large" enough (it would
be surprising, at least to me, to write a test that would need more than 32
waiters).

This could use less. I've never used more than 3 of them in a single
test, and that was already very complex to follow.

A few comments:

1 ===
I think "up" is missing at several places in the patch where "wake" is used.
I could be wrong as non native english speaker though.

Patched up three places to be more consisten.

2 ===

+       /* Counters advancing when injection_points_wakeup() is called */
+       int                     wait_counts[INJ_MAX_WAIT];

uint32? (here and other places where counter is manipulated).

Okay, why not.

"Remove this injection wait name from the waiting list" instead?
s/a condition variable/an injection wait point/ ?

Okay.

5 ===

+PG_FUNCTION_INFO_V1(injection_points_wakeup);
+Datum
+injection_points_wakeup(PG_FUNCTION_ARGS)

Empty line missing before "Datum"?

I've used the same style as anywhere else.

Also maybe some comments are missing above injection_point_init_state(),
injection_init_shmem() but it's more a Nit.

Sure.

While at it, should we add a second injection wait point in
t/041_invalid_checkpoint_after_promote.pl to check that they are wake up
individually?

I'm not sure that's a good idea for the sake of this test, TBH, as
that would provide coverage outside the original scope of the
restartpoint/promote check.

I have also looked at if it would be possible to get an isolation test
out of that, but the arbitrary wait does not make that possible with
the existing isolation APIs, see GetSafeSnapshotBlockingPids() used in
pg_isolation_test_session_is_blocked(). isolation/README seems to be
a bit off here, actually, mentioning pg_locks.. We could use some
tricks with transactions or locks internally, but I'm not really
tempted to make the wait callback more complicated for the sake of
more coverage as I'd rather keep it generic for anybody who wants to
control the order of events across processes.

Attaching a v3 for reference with everything that has been mentioned
up to now.
--
Michael

Hi,

On Thu, Feb 22, 2024 at 12:02:01PM +0900, Michael Paquier wrote:

On Wed, Feb 21, 2024 at 11:50:21AM +0000, Bertrand Drouvot wrote:

A few comments:

1 ===
I think "up" is missing at several places in the patch where "wake" is used.
I could be wrong as non native english speaker though.

Patched up three places to be more consisten.

Thanks!

5 ===

+PG_FUNCTION_INFO_V1(injection_points_wakeup);
+Datum
+injection_points_wakeup(PG_FUNCTION_ARGS)

Empty line missing before "Datum"?

I've used the same style as anywhere else.

humm, looking at src/test/regress/regress.c for example, I can see an empty
line between PG_FUNCTION_INFO_V1 and Datum for all the "PG_FUNCTION_INFO_V1"
ones.

While at it, should we add a second injection wait point in
t/041_invalid_checkpoint_after_promote.pl to check that they are wake up
individually?

I'm not sure that's a good idea for the sake of this test, TBH, as
that would provide coverage outside the original scope of the
restartpoint/promote check.

Yeah, that was my doubt too.

I have also looked at if it would be possible to get an isolation test
out of that, but the arbitrary wait does not make that possible with
the existing isolation APIs, see GetSafeSnapshotBlockingPids() used in
pg_isolation_test_session_is_blocked(). isolation/README seems to be
a bit off here, actually, mentioning pg_locks.. We could use some
tricks with transactions or locks internally, but I'm not really
tempted to make the wait callback more complicated for the sake of
more coverage as I'd rather keep it generic for anybody who wants to
control the order of events across processes.

Makes sense to me, let's keep it as it is.

Attaching a v3 for reference with everything that has been mentioned
up to now.

Thanks!

Sorry, I missed those ones previously:

1 ===

+/* Maximum number of wait usable in injection points at once */

s/Maximum number of wait/Maximum number of waits/ ?

2 ===

+# Check the logs that the restart point has started on standby.  This is
+# optional, but let's be sure.
+my $log = slurp_file($node_standby->logfile, $logstart);
+my $checkpoint_start = 0;
+if ($log =~ m/restartpoint starting: immediate wait/)
+{
+       $checkpoint_start = 1;
+}
+is($checkpoint_start, 1, 'restartpoint has started');

what about?

ok( $node_standby->log_contains( "restartpoint starting: immediate wait", $logstart),
"restartpoint has started");

Except for the above, v3 looks good to me.

Regards,

--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com

On Thu, Feb 22, 2024 at 08:00:24AM +0000, Bertrand Drouvot wrote:

+/* Maximum number of wait usable in injection points at once */

s/Maximum number of wait/Maximum number of waits/ ?

Thanks. I've edited a few more places while scanning the whole.

2 ===

+# Check the logs that the restart point has started on standby.  This is
+# optional, but let's be sure.
+my $log = slurp_file($node_standby->logfile, $logstart);
+my $checkpoint_start = 0;
+if ($log =~ m/restartpoint starting: immediate wait/)
+{
+       $checkpoint_start = 1;
+}
+is($checkpoint_start, 1, 'restartpoint has started');

what about?

ok( $node_standby->log_contains( "restartpoint starting: immediate wait", $logstart),
"restartpoint has started");

And I'm behind the commit that introduced it (392ea0c78fdb). It is
possible to remove the dependency to slurp_file() entirely by
switching the second location checking the logs for the checkpoint
completion.

Except for the above, v3 looks good to me.

Thanks. I'm looking at applying that at the beginning of next week if
there are no more comments, to get something by the feature freeze.
We could be more flexible for all that as that's related to testing,
but let's be in the clear.

I've also renamed the test to 041_checkpoint_at_promote.pl, as now
that the original is fixed, the checkpoint is not invalid. That's
cleaner this way.
--
Michael

Hi,

On Mon, Feb 26, 2024 at 12:57:09PM +0900, Michael Paquier wrote:

On Thu, Feb 22, 2024 at 08:00:24AM +0000, Bertrand Drouvot wrote:

+/* Maximum number of wait usable in injection points at once */

s/Maximum number of wait/Maximum number of waits/ ?

Thanks. I've edited a few more places while scanning the whole.

Thanks!

2 ===

+# Check the logs that the restart point has started on standby.  This is
+# optional, but let's be sure.
+my $log = slurp_file($node_standby->logfile, $logstart);
+my $checkpoint_start = 0;
+if ($log =~ m/restartpoint starting: immediate wait/)
+{
+       $checkpoint_start = 1;
+}
+is($checkpoint_start, 1, 'restartpoint has started');

what about?

ok( $node_standby->log_contains( "restartpoint starting: immediate wait", $logstart),
"restartpoint has started");

And I'm behind the commit that introduced it (392ea0c78fdb).

;-)

It is
possible to remove the dependency to slurp_file() entirely by
switching the second location checking the logs for the checkpoint
completion.

Yeah right.

Except for the above, v3 looks good to me.

Thanks. I'm looking at applying that at the beginning of next week if
there are no more comments, to get something by the feature freeze.
We could be more flexible for all that as that's related to testing,
but let's be in the clear.

Sounds reasonable to me.

I've also renamed the test to 041_checkpoint_at_promote.pl, as now
that the original is fixed, the checkpoint is not invalid. That's
cleaner this way.

Agree.

I'll try to submit the POC patch in [1]/messages/by-id/ZdTNafYSxwnKNIhj@ip-10-97-1-34.eu-west-3.compute.internal before beginning of next week now that
we're "just waiting" if there is more comments on this current thread.

[1]: /messages/by-id/ZdTNafYSxwnKNIhj@ip-10-97-1-34.eu-west-3.compute.internal

Regards,

--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com

On 26 Feb 2024, at 08:57, Michael Paquier <michael@paquier.xyz> wrote:

<v4-0001-injection_points-Add-routines-to-wait-and-wake-pr.patch>

Would it be possible to have a helper function to check this:

+ok( $node_standby->poll_query_until(
+		'postgres',
+		qq[SELECT count(*) FROM pg_stat_activity
+           WHERE backend_type = 'checkpointer' AND wait_event = 'CreateRestartPoint' ;],
+		'1'),
+	'checkpointer is waiting in restart point'
+) or die "Timed out while waiting for checkpointer to run restart point”;

So that we could do something like

ok(node_standby->await_injection_point(“CreateRestartPoint”,”checkpointer"));

IMO, this could make many tests cleaner.
Or, perhaps, it’s a functionality for a future development?

Thanks!

Best regards, Andrey Borodin.

On Mon, Feb 26, 2024 at 02:10:49PM +0500, Andrey M. Borodin wrote:

So that we could do something like

ok(node_standby->await_injection_point(“CreateRestartPoint”,”checkpointer"));

It would be more flexible with a full string to describe the test
rather than a process name in the second argument.

IMO, this could make many tests cleaner.
Or, perhaps, it’s a functionality for a future development?

This could just happen as separate refactoring, I guess. But I'd wait
to see if more tests requiring scans to pg_stat_activity pop up. For
example, the test just posted here does not rely on that:
/messages/by-id/ZdyZya4YrNapWKqz@ip-10-97-1-34.eu-west-3.compute.internal
--
Michael

On 27 Feb 2024, at 04:29, Michael Paquier <michael@paquier.xyz> wrote:

For
example, the test just posted here does not rely on that:
/messages/by-id/ZdyZya4YrNapWKqz@ip-10-97-1-34.eu-west-3.compute.internal

Instead, that test is scanning logs

+ # Note: $node_primary->wait_for_replay_catchup($node_standby) would be
+ # hanging here due to the injection point, so check the log instead.+
+ my $terminated = 0;
+ for (my $i = 0; $i < 10 * $PostgreSQL::Test::Utils::timeout_default; $i++)
+ {
+ if ($node_standby->log_contains(
+ 'terminating process .* to release replication slot \"injection_activeslot\"', $logstart))
+ {
+ $terminated = 1;
+ last;
+ }
+ usleep(100_000);
+ }

But, AFAICS, the purpose is the same: wait until event happened.

Best regards, Andrey Borodin.