Date: Tue, 20 Feb 2024 15:55:08 +0000
From: Bertrand Drouvot <bertranddrouvot.pg@gmail.com>
To: Michael Paquier <michael@paquier.xyz>
Cc: Postgres hackers <pgsql-hackers@lists.postgresql.org>,
	Ashutosh Bapat <ashutosh.bapat.oss@gmail.com>
Subject: Re: Injection points: some tools to wait and wake
Message-ID: <ZdTLXCnN0EV0e/KF@ip-10-97-1-34.eu-west-3.compute.internal>
References: <ZdLuxBk5hGpol91B@paquier.xyz>
 <ZdMIkfJnBj6rBQrL@paquier.xyz>
 <ZdNldOxVx4SL3+f0@ip-10-97-1-34.eu-west-3.compute.internal>
 <ZdPkHNMK8s4o0tV4@paquier.xyz>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <ZdPkHNMK8s4o0tV4@paquier.xyz>
Archived-At: <https://www.postgresql.org/message-id/ZdTLXCnN0EV0e/KF%40ip-10-97-1-34.eu-west-3.compute.internal>
Precedence: bulk

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