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
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
> > +#
> > +#
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
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
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 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
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
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
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] =
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
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
> >
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
>>> 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
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.
>
>
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
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
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
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:
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
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
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
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
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
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:
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
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
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
>
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
37 matches
Mail list logo
