On Tue, Nov 26, 2024 at 04:47:07PM -0500, Andres Freund wrote:
> LGTM!
Committed.
--
nathan
On 2024-11-26 15:07:24 -0600, Nathan Bossart wrote:
> I've attempted to add all these details in v3.
LGTM!
On Tue, Nov 26, 2024 at 12:27:33PM -0500, Andres Freund wrote:
> On 2024-11-22 20:44:34 -0600, Nathan Bossart wrote:
>> D'oh, I missed that ProcNumber could be used as an index for the
>> BackendStatusArray. Is the attached more like what you are imagining?
>
> Yes.
>
> I'd probably add two func
Hi,
On 2024-11-22 20:44:34 -0600, Nathan Bossart wrote:
> On Fri, Nov 22, 2024 at 06:13:16PM -0500, Andres Freund wrote:
> >> - if (procStatus && procStatus->st_backendType ==
> >> B_AUTOVAC_WORKER)
> >> + if (pgstat_get_backend_type(pid) == B_AUTOVAC_WORKER)
> >
> > Because we
On Sat, 23 Nov 2024, 07:44 Nathan Bossart, wrote:
> On Fri, Nov 22, 2024 at 06:13:16PM -0500, Andres Freund wrote:
> >> -if (procStatus && procStatus->st_backendType ==
> B_AUTOVAC_WORKER)
> >> +if (pgstat_get_backend_type(pid) == B_AUTOVAC_WORKER)
> >
> > Because we alrea
On Fri, Nov 22, 2024 at 06:13:16PM -0500, Andres Freund wrote:
>> -if (procStatus && procStatus->st_backendType ==
>> B_AUTOVAC_WORKER)
>> +if (pgstat_get_backend_type(pid) == B_AUTOVAC_WORKER)
>
> Because we already mapped the pid to a ProcNumber, it'd be cheaper to acces
Hi,
On 2024-11-22 13:21:53 -0600, Nathan Bossart wrote:
> Here is a draft-grade patch for this one. It seems pretty
> straightforward...
Thanks for looking into it quickly.
> diff --git a/src/backend/storage/ipc/signalfuncs.c
> b/src/backend/storage/ipc/signalfuncs.c
> index aa729a36e3..8e165
On Fri, Nov 22, 2024 at 12:13:49PM -0500, Andres Freund wrote:
> I justed ended up looking at this code for some boring reason. One thing that
> has me worried a bit is that pg_signal_backend() now does
> pgstat_get_beentry_by_proc_number(), triggering a pgstat_read_current_status()
> further down.
On Fri, 22 Nov 2024 at 22:13, Andres Freund wrote:
>
> Hi,
>
> On 2024-07-09 13:12:59 -0500, Nathan Bossart wrote:
> > I've committed 0001.
>
> I justed ended up looking at this code for some boring reason. One thing that
> has me worried a bit is that pg_signal_backend() now does
> pgstat_get_bee
Hi,
On 2024-07-09 13:12:59 -0500, Nathan Bossart wrote:
> I've committed 0001.
I justed ended up looking at this code for some boring reason. One thing that
has me worried a bit is that pg_signal_backend() now does
pgstat_get_beentry_by_proc_number(), triggering a pgstat_read_current_status()
fur
On Mon, Jul 15, 2024 at 09:54:43AM +0900, Michael Paquier wrote:
> Thanks. I'll see about stressing the buildfarm tomorrow or so, after
> looking at how the CI reacts.
There were a few more things here:
1) The new test was missing from test_misc/meson.build.
2) With 1) fixed, the CI has been comp
On Fri, Jul 12, 2024 at 11:19:05AM -0500, Nathan Bossart wrote:
> I suppose it would be silly to allow even lower values for
> autovacuum_naptime (e.g., by moving it to ConfigureNamesReal and setting
> the minimum to 0.1).
I've thought about that as well, and did not mention it as this would
encou
On Fri, Jul 12, 2024 at 02:21:09PM +0900, Michael Paquier wrote:
> On Thu, Jul 11, 2024 at 08:50:57PM -0500, Nathan Bossart wrote:
>> I'm not following how this is guaranteed to trigger an autovacuum quickly.
>> Shouldn't we set autovacuum_vacuum_insert_threshold to 1 so that it is
>> eligible for
On Thu, Jul 11, 2024 at 08:50:57PM -0500, Nathan Bossart wrote:
> I'm not following how this is guaranteed to trigger an autovacuum quickly.
> Shouldn't we set autovacuum_vacuum_insert_threshold to 1 so that it is
> eligible for autovacuum?
You are right, this is not going to influence a faster st
On Wed, Jul 10, 2024 at 02:14:55PM +0900, Michael Paquier wrote:
> +# Only non-superuser roles granted pg_signal_autovacuum_worker are allowed
> +# to signal autovacuum workers. This test uses an injection point located
> +# at the beginning of the autovacuum worker startup.
nitpick: Superuser ro
On Wed, Jul 10, 2024 at 10:57:45PM +0500, Kirill Reshke wrote:
> Hi, that's for digging into this. Turns out I completely missed one of
> your emails today morning.
Don't worry. Using this domain tends to put my emails in one's spam
folder.
--
Michael
signature.asc
Description: PGP signature
Hi, that's for digging into this. Turns out I completely missed one of
your emails today morning.
On Wed, 10 Jul 2024 at 10:15, Michael Paquier wrote:
> And then the timestamp of the tests:
> [12:48:23.277](0.058s) not ok 2 - autovacuum worker signaled with
> pg_signal_autovacuum_worker granted
>
> On 10 Jul 2024, at 11:27, Kirill Reshke wrote:
>
> That's very strange, because the test works fine on my virtual
> machine. Also, it seems that it works in Cirrus [0], as there is this
> line:
So far I could not reproduce that failure.
I’ve checkouted 6edec53 from CFbot repository, but it
> The script has two tests, and the CI is failing for the second test
> where we expect the signal to be processed:
> [12:48:23.370] # Failed test 'autovacuum worker signaled with
> pg_signal_autovacuum_worker granted'
> [12:48:23.370] # at t/006_signal_autovacuum.pl line 90.
> --
> Michael
T
On Wed, Jul 10, 2024 at 10:03:04AM +0500, Kirill Reshke wrote:
> The problem is the error message has been changed.
>
> # DETAIL: Only roles with privileges of the
> "pg_signal_autovacuum_worker" role may terminate autovacuum workers.'
> # doesn't match '(?^:ERROR: permission denied to termi
On Tue, Jul 09, 2024 at 01:12:59PM -0500, Nathan Bossart wrote:
> I've committed 0001. It looks like 0002 failed CI testing [0], but I
> haven't investigated why.
>
> [0] https://cirrus-ci.com/task/5668467599212544
Nice catch by the CI. This looks like a race condition to me. I
think that we s
Hi
On Tue, 9 Jul 2024 at 23:13, Nathan Bossart wrote:
>
> I've committed 0001. It looks like 0002 failed CI testing [0], but I
> haven't investigated why.
>
> [0] https://cirrus-ci.com/task/5668467599212544
>
> --
> nathan
The problem is the error message has been changed.
# DETAIL: Only role
I've committed 0001. It looks like 0002 failed CI testing [0], but I
haven't investigated why.
[0] https://cirrus-ci.com/task/5668467599212544
--
nathan
> On 21 Jun 2024, at 10:36, Michael Paquier wrote:
>
> On Fri, Jun 14, 2024 at 03:12:50PM -0500, Nathan Bossart wrote:
>> On Fri, Jun 14, 2024 at 12:06:36PM +0500, Andrey M. Borodin wrote:
>>> This patch looks good to me.
>>
>> Thanks for looking.
>
> While double-checking the whole, where I
On Fri, Jun 21, 2024 at 10:31:30AM +0500, Andrey M. Borodin wrote:
> Thanks for the pointer, I’ll try this approach!
Thanks. FWIW, I've put my mind into it, and fixed the thing a few
minutes ago:
https://www.postgresql.org/message-id/ZnURUaujl39wSoEW%40paquier.xyz
--
Michael
signature.asc
Descr
On Fri, Jun 14, 2024 at 03:12:50PM -0500, Nathan Bossart wrote:
> On Fri, Jun 14, 2024 at 12:06:36PM +0500, Andrey M. Borodin wrote:
> > This patch looks good to me.
>
> Thanks for looking.
While double-checking the whole, where I don't have much to say about
0001, I have fixed a few issues with
> On 21 Jun 2024, at 09:01, Michael Paquier wrote:
>
> On Fri, Jun 14, 2024 at 12:06:36PM +0500, Andrey M. Borodin wrote:
>> I’ve tried to dig into the test.
>> The problem is CV is allocated in
>>
>> inj_state = GetNamedDSMSegment("injection_points”,
>>
>> which seems to be destroyed in
>>
On Fri, Jun 14, 2024 at 12:06:36PM +0500, Andrey M. Borodin wrote:
> I’ve tried to dig into the test.
> The problem is CV is allocated in
>
> inj_state = GetNamedDSMSegment("injection_points”,
>
> which seems to be destroyed in
>
> shmem_exit() calling dsm_backend_shutdown()
>
> This happens be
On Fri, Jun 14, 2024 at 12:06:36PM +0500, Andrey M. Borodin wrote:
> This patch looks good to me.
Thanks for looking.
> Spellchecker is complaining about "signaling" instead of "signalling",
> but ISTM it´s OK.
I think this is an en-US versus en-GB thing. We've standardized on en-US
for "cancel
> On 13 Jun 2024, at 02:04, Nathan Bossart wrote:
>
> I adjusted 0001 based on my upthread feedback.
This patch looks good to me. Spellchecker is complaining about “signaling”
instead of “signalling”, but ISTM it’s OK.
I’ve tried to dig into the test.
The problem is CV is allocated in
inj_
I adjusted 0001 based on my upthread feedback.
--
nathan
>From 118f95346fcf8099ab28d2f9186185171e3b88af Mon Sep 17 00:00:00 2001
From: Nathan Bossart
Date: Wed, 12 Jun 2024 15:38:14 -0500
Subject: [PATCH v5 1/1] Introduce pg_signal_autovacuum_worker.
Since commit 3a9b18b309, roles with privileg
On Fri, Apr 12, 2024 at 01:32:42PM +0500, Kirill Reshke wrote:
> +#
> +# Portions Copyright (c) 1996-2024, PostgreSQL Global Development Group
> +# Portions Copyright (c) 1994, Regents of the University of California
> +#
> (Like in src/test/signals/Makefile)
>
> at the beginning of each added fil
On Thu, 11 Apr 2024 at 16:55, Kirill Reshke wrote:
> 7)
>
> > +# Copyright (c) 2022-2024, PostgreSQL Global Development Group
> > [...]
> > +# Copyright (c) 2024-2024, PostgreSQL Global Development Group
>
> > These need to be cleaned up.
>
> > +# Makefile for src/test/recovery
> > +#
> > +# src/
At Fri, 12 Apr 2024 09:10:35 +0900, Michael Paquier wrote
in
> On Thu, Apr 11, 2024 at 04:55:59PM +0500, Kirill Reshke wrote:
> > The test doesn't fail because pg_terminate_backend actually meets his
> > point: autovac is killed. But while dying, autovac also receives
> > segfault. Thats because
On Thu, Apr 11, 2024 at 04:55:59PM +0500, Kirill Reshke wrote:
> The test doesn't fail because pg_terminate_backend actually meets his
> point: autovac is killed. But while dying, autovac also receives
> segfault. Thats because of injections points:
>
> (gdb) bt
> #0 0x56361c3379ea in tas (lo
On Thu, 11 Apr 2024 at 19:07, Nathan Bossart wrote:
>
> On Thu, Apr 11, 2024 at 04:55:59PM +0500, Kirill Reshke wrote:
> > Posting updated version of this patch with comments above addressed.
>
> I look for a commitfest entry for this one, but couldn't find it. Would
> you mind either creating on
On Thu, Apr 11, 2024 at 04:55:59PM +0500, Kirill Reshke wrote:
>> It's feeling more natural here to check that we have a superuser-owned
>> backend first, and then do a lookup of the process type.
>
>> I figured since there's no reason to rely on that behavior, we might as
>> well do a bit of futu
On Thu, Apr 11, 2024 at 04:55:59PM +0500, Kirill Reshke wrote:
> Posting updated version of this patch with comments above addressed.
I look for a commitfest entry for this one, but couldn't find it. Would
you mind either creating one or, if I've somehow missed it, pointing me to
the existing ent
Posting updated version of this patch with comments above addressed.
1) pg_signal_autovacuum -> pg_signal_autovacuum_worker, as there seems
to be no objections to that.
2)
There are comments on how to write if statement:
> In core, do_autovacuum() is only called in a process without
> a role spe
On Wed, Apr 10, 2024 at 10:00:34AM -0500, Nathan Bossart wrote:
> Isn't it relatively easy to discover this same information today via
> pg_stat_progress_vacuum? That has the following code:
>
> /* Value available to all callers */
> values[0] = Int32GetDatum(beentry->
On Wed, Apr 10, 2024 at 07:58:39AM +0900, Michael Paquier wrote:
> On Wed, Apr 10, 2024 at 12:52:19AM +0300, Kirill Reshke wrote:
>> On Tue, 9 Apr 2024 at 08:53, Michael Paquier wrote:
>>> The thing is that you cannot rely on a lookup of the backend type for
>>> the error information, or you open
On Wed, Apr 10, 2024 at 12:52:19AM +0300, Kirill Reshke wrote:
> On Tue, 9 Apr 2024 at 08:53, Michael Paquier wrote:
>> The thing is that you cannot rely on a lookup of the backend type for
>> the error information, or you open yourself to letting the caller of
>> pg_cancel_backend or pg_terminate
Hi, thanks for looking into this.
On Tue, 9 Apr 2024 at 08:53, Michael Paquier wrote:
> On Mon, Apr 08, 2024 at 05:42:05PM +, Leung, Anthony wrote:
> > Are you suggesting that we check if the backend is B_AUTOVAC in
> > pg_cancel/ terminate_backend? That seems a bit unclean to me since
> > p
On Mon, Apr 08, 2024 at 05:42:05PM +, Leung, Anthony wrote:
> Are you suggesting that we check if the backend is B_AUTOVAC in
> pg_cancel/ terminate_backend? That seems a bit unclean to me since
> pg_cancel_backend & pg_cancel_backend does not access to the
> procNumber to check the type of the
>>> There is pg_read_all_stats as well, so I don't see a big issue in
>>> requiring to be a member of this role as well for the sake of what's
>>> proposing here.
>>
>> Well, that tells you quite a bit more than just which PIDs correspond to
>> autovacuum workers, but maybe that's good enough for n
On Fri, Apr 05, 2024 at 08:07:51PM -0500, Nathan Bossart wrote:
> On Sat, Apr 06, 2024 at 08:56:04AM +0900, Michael Paquier wrote:
>> There is pg_read_all_stats as well, so I don't see a big issue in
>> requiring to be a member of this role as well for the sake of what's
>> proposing here.
>
> Wel
On Sat, Apr 06, 2024 at 08:56:04AM +0900, Michael Paquier wrote:
> There is pg_read_all_stats as well, so I don't see a big issue in
> requiring to be a member of this role as well for the sake of what's
> proposing here.
Well, that tells you quite a bit more than just which PIDs correspond to
aut
On Fri, Apr 05, 2024 at 07:56:56AM -0500, Nathan Bossart wrote:
> On Fri, Apr 05, 2024 at 02:39:05PM +0900, Michael Paquier wrote:
>> One thing that we should definitely not do is letting any user calling
>> pg_signal_backend() know that a given PID maps to an autovacuum
>> worker. This informatio
On Fri, Apr 05, 2024 at 02:39:05PM +0900, Michael Paquier wrote:
> + /*
> + * If the backend is autovacuum worker, allow user with the privileges
> of
> + * pg_signal_autovacuum role to signal the backend.
> + */
> + if (pgstat_get_backend_type(GetNumberFromPGProc(proc)) ==
On Fri, Apr 05, 2024 at 12:03:05AM +, Leung, Anthony wrote:
> Adding tap test for pg_signal_autovacuum using injection points as a
> separate patch. I also made a minor change on the original patch.
+ ret = pgstat_get_beentry_by_proc_number(procNumber);
+
+ if (!ret)
+
> On 5 Apr 2024, at 05:03, Leung, Anthony wrote:
>
> Adding tap test for pg_signal_autovacuum using injection points as a separate
> patch. I also made a minor change on the original patch.
The test looks good, but:
1. remove references to passcheck :)
2. detach injection point when it's not
Adding tap test for pg_signal_autovacuum using injection points as a separate
patch. I also made a minor change on the original patch.
Thanks.
--
Anthony
Amazon Web Services: https://aws.amazon.com
v3-0001-Introduce-pg_signal_autovacuum-role-to-signal-autovacuum-worker.patch
Description: v3-
I made some updates based on the feedbacks in v2. This patch only contains the
code change for allowing the signaling to av worker with pg_signal_autovacuum.
I will send a separate patch for the tap test shortly.
Thanks
--
Anthony Leung
Amazon Web Services: https://aws.amazon.com
v2-0001-In
On Thu, Apr 04, 2024 at 12:30:51AM +, Leung, Anthony wrote:
>> if (pg_stat_is_backend_autovac_worker(proc->backendId) &&
>> !has_privs_of_role(GetUserId(), ROLE_PG_SIGNAL_AUTOVACUUM))
>> return SIGNAL_BACKEND_NOAUTOVACUUM;
>
> I tried to add them above the existing code.
On Tue, Apr 02, 2024 at 04:35:28PM +0500, Andrey M. Borodin wrote:
> We can add tests just like [0] with injection points.
> I mean replace that "sleep 1" with something like
> "$node->wait_for_event('autovacuum worker', 'autocauum-runing');".
> Currently we have no infrastructure to wait for autov
Update - the condition should be &&
if (pgstat_get_backend_type(proc->backendId) == B_AUTOVAC_WORKER)
{
if (!has_privs_of_role(GetUserId(), ROLE_PG_SIGNAL_AUTOVACUUM)
&& !superuser())
return SIGNAL_BACKEND_NOAUTOVACUUM;
}
Thanks
--
> I don't think we should rely on !OidIsValid(proc->roleId) for signaling
> autovacuum workers. That might not always be true, and I don't see any
> need to rely on that otherwise. IMHO we should just add a few lines before
> the existing code, which doesn't need to be changed at all:
>
>
> On 2 Apr 2024, at 01:21, Nathan Bossart wrote:
>
> I haven't looked too closely, but I'm pretty skeptical that the test suite
> in your patch would be stable. Unfortunately, I don't have any better
> ideas at the moment besides not adding a test for this new role.
We can add tests just lik
On Mon, Apr 01, 2024 at 02:29:29PM +, Leung, Anthony wrote:
> I've attached the updated patch with some improvements.
Thanks!
+
+ pg_signal_autovacuum
+ Allow terminating backend running autovacuum
+
I think we should be more precise here by calling out the exact type
I took the liberty of continuing to work on this after chatting with Nathan.
I've attached the updated patch with some improvements.
Thanks.
--
Anthony Leung
Amazon Web Services: https://aws.amazon.com
v1-0001-Introduce-pg_signal_autovacuum-role-to-allow-non-sup.patch
Description: v1-0001
Another comment that I forgot to mention is that we should also make the
documentation change in doc/src/sgml/user-manag.sgml for this new predefined
role
Thanks.
--
Anthony Leung
Amazon Web Services: https://aws.amazon.com
Hi,
I'm new to reviewing postgres patches, but I took an interest in reviewing this
patch as recommended by Nathan.
I have the following comments:
> if (!superuser()) {
> if (!OidIsValid(proc->roleId)) {
> LocalPgBackendStatus *local_beentry;
>
On Tue, Feb 27, 2024 at 11:59:00PM +0500, Kirill Reshke wrote:
> Do we need to test the pg_cancel_backend vs autovacuum case at all?
> I think we do. Would it be better to split work into 2 patches: first one
> with tests against current logic, and second
> one with some changes/enhancements which
On Tue, Feb 27, 2024 at 01:22:31AM +0500, Kirill Reshke wrote:
> Also, tap tests for functionality added. I'm not sure where to place them,
> so I placed them in a separate directory in `src/test/`
> Seems that regression tests for this feature are not possible, am i right?
It might be difficult t
On Tue, 27 Feb 2024 at 01:22, Kirill Reshke wrote:
>
>
> On Mon, 26 Feb 2024 at 20:10, Nathan Bossart
> wrote:
>
>> On Mon, Feb 26, 2024 at 12:38:40PM +0500, Kirill Reshke wrote:
>> > I see 2 possible ways to implement this. The first one is to have hool
>> in
>> > pg_signal_backend, and define
On Mon, 26 Feb 2024 at 20:10, Nathan Bossart
wrote:
> On Mon, Feb 26, 2024 at 12:38:40PM +0500, Kirill Reshke wrote:
> > I see 2 possible ways to implement this. The first one is to have hool in
> > pg_signal_backend, and define a hook in extension which can do the thing.
> > The second one is to
On Mon, Feb 26, 2024 at 12:38:40PM +0500, Kirill Reshke wrote:
> I see 2 possible ways to implement this. The first one is to have hool in
> pg_signal_backend, and define a hook in extension which can do the thing.
> The second one is to have a predefined role. Something like a
> `pg_signal_autovac
Hi hackers!
In our Cloud we have a patch, which allows non-superuser role ('mdb_admin')
to do some superuser things.
In particular, we have a patch that allows mdb admin to cancel the
autovacuum process and some other processes (processes with
application_name = 'MDB'), see the attachment.
This is
68 matches
Mail list logo
