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
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 th
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 te
> On 10 May 2024, at 06:04, Michael Paquier 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 resultin
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 t
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 cau
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 102
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 optio
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
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,
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
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 no
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()
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
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
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 thi
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:
> >
> On 2 May 2024, at 13:43, Michael Paquier 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_p
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
> On 2 May 2024, at 12:27, 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: loc
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 C
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 Injec
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 injecti
On Mon, Apr 08, 2024 at 12:29:43PM +0300, Andrey M. Borodin wrote:
> On 8 Apr 2024, at 11:55, Michael Paquier 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 w
> On 8 Apr 2024, at 11:55, Michael Paquier 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 t
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.
--
Mich
> On 8 Apr 2024, at 10:33, Michael Paquier wrote:
>
> Thoughts?
As an alternative we can make local injection points mutually exclusive.
Best regards, Andrey Borodin.
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 t
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
> Injec
> On 5 Apr 2024, at 07:19, Michael Paquier 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
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
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 wo
> On 18 Mar 2024, at 06:04, Michael Paquier 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
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 re
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');" fo
On Sat, Mar 16, 2024 at 7:27 AM Tom Lane 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
Heikki Linnakangas 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 o
On 15/03/2024 16:00, Tom Lane wrote:
Heikki Linnakangas 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
Heikki Linnakangas 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.
M
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.b
On 15/03/2024 13:09, Heikki Linnakangas wrote:
On 15/03/2024 01:13, Tom Lane wrote:
Michael Paquier 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
On 15/03/2024 01:13, Tom Lane wrote:
Michael Paquier 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 curre
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 poi
On Thu, Mar 14, 2024 at 07:13:53PM -0400, Tom Lane wrote:
> Michael Paquier 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_INSTALL
Michael Paquier 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 InjectionPointAt
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 datab
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
Thomas Munro writes:
> On Fri, Mar 15, 2024 at 11:19 AM 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.
> Right, as mention
I wrote:
> Heikki Linnakangas 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?
On Fri, Mar 15, 2024 at 11:19 AM Tom Lane wrote:
> Heikki Linnakangas 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
Heikki Linnakangas 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 i
51 matches
Mail list logo
