Thread: Allow placeholders in ALTER ROLE w/o superuser
Hello hackers,
Using placeholders for application variables allows the use of RLS for application users as shown in this blog post https://www.2ndquadrant.com/en/blog/application-users-vs-row-level-security/.
SET my.username = 'tomas'
CREATE POLICY chat_policy ON chat
USING (current_setting('my.username') IN (message_from, message_to))
WITH CHECK (message_from = current_setting('my.username'))
CREATE POLICY chat_policy ON chat
USING (current_setting('my.username') IN (message_from, message_to))
WITH CHECK (message_from = current_setting('my.username'))
This technique has enabled postgres sidecar services(PostgREST, PostGraphQL, etc) to keep the application security at the database level, which has worked great.
However, defining placeholders at the role level require superuser:
alter role myrole set my.username to 'tomas';
ERROR: permission denied to set parameter "my.username"
Which is inconsistent and surprising behavior. I think it doesn't make sense since you can already set them at the session or transaction level(SET LOCAL my.username = 'tomas'). Enabling this would allow sidecar services to store metadata scoped to its pertaining role.
I've attached a patch that removes this restriction. From my testing, this doesn't affect permission checking when an extension defines its custom GUC variables.
DefineCustomStringVariable("my.custom", NULL, NULL, &my_custom, NULL,
PGC_SUSET, ..);
PGC_SUSET, ..);
Using PGC_SUSET or PGC_SIGHUP will fail accordingly. Also no tests fail when doing "make installcheck".
Attachment
On Sun, Jun 05, 2022 at 11:20:38PM -0500, Steve Chavez wrote:
> However, defining placeholders at the role level require superuser:
>
> alter role myrole set my.username to 'tomas';
> ERROR: permission denied to set parameter "my.username"
>
> Which is inconsistent and surprising behavior. I think it doesn't make
> sense since you can already set them at the session or transaction
> level(SET LOCAL my.username = 'tomas'). Enabling this would allow sidecar
> services to store metadata scoped to its pertaining role.
>
> I've attached a patch that removes this restriction. From my testing, this
> doesn't affect permission checking when an extension defines its custom GUC
> variables.
>
> DefineCustomStringVariable("my.custom", NULL, NULL, &my_custom, NULL,
> PGC_SUSET, ..);
>
> Using PGC_SUSET or PGC_SIGHUP will fail accordingly. Also no tests fail
> when doing "make installcheck".
IIUC you are basically proposing to revert a6dcd19 [0], but it is not clear
to me why that is safe. Am I missing something?
[0] https://www.postgresql.org/message-id/flat/4090.1258042387%40sss.pgh.pa.us
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
On Fri, Jul 01, 2022 at 04:40:27PM -0700, Nathan Bossart wrote:
> On Sun, Jun 05, 2022 at 11:20:38PM -0500, Steve Chavez wrote:
>> However, defining placeholders at the role level require superuser:
>>
>> alter role myrole set my.username to 'tomas';
>> ERROR: permission denied to set parameter "my.username"
>>
>> Which is inconsistent and surprising behavior. I think it doesn't make
>> sense since you can already set them at the session or transaction
>> level(SET LOCAL my.username = 'tomas'). Enabling this would allow sidecar
>> services to store metadata scoped to its pertaining role.
>>
>> I've attached a patch that removes this restriction. From my testing, this
>> doesn't affect permission checking when an extension defines its custom GUC
>> variables.
>>
>> DefineCustomStringVariable("my.custom", NULL, NULL, &my_custom, NULL,
>> PGC_SUSET, ..);
>>
>> Using PGC_SUSET or PGC_SIGHUP will fail accordingly. Also no tests fail
>> when doing "make installcheck".
>
> IIUC you are basically proposing to revert a6dcd19 [0], but it is not clear
> to me why that is safe. Am I missing something?
I spent some more time looking into this, and I think I've constructed a
simple example that demonstrates the problem with removing this
restriction.
postgres=# CREATE ROLE test CREATEROLE;
CREATE ROLE
postgres=# CREATE ROLE other LOGIN;
CREATE ROLE
postgres=# GRANT CREATE ON DATABASE postgres TO other;
GRANT
postgres=# SET ROLE test;
SET
postgres=> ALTER ROLE other SET plperl.on_plperl_init = 'test';
ALTER ROLE
postgres=> \c postgres other
You are now connected to database "postgres" as user "other".
postgres=> CREATE EXTENSION plperl;
CREATE EXTENSION
postgres=> SHOW plperl.on_plperl_init;
plperl.on_plperl_init
-----------------------
test
(1 row)
In this example, the non-superuser role sets a placeholder GUC for another
role. This GUC becomes a PGC_SUSET GUC when plperl is loaded, so a
non-superuser role will have successfully set a PGC_SUSET GUC. If we had a
record of who ran ALTER ROLE, we might be able to apply appropriate
permissions checking when the module is loaded, but this information
doesn't exist in pg_db_role_setting. IIUC we have the following options:
1. Store information about who ran ALTER ROLE. I think there are a
couple of problems with this. For example, what happens if the
grantor was dropped or its privileges were altered? Should we
instead store the context of the user (i.e., PGC_USERSET or
PGC_SUSET)? Do we need to add entries to pg_depend?
2. Ignore or ERROR for any ALTER ROLE settings for custom GUCs. Since
we don't know who ran ALTER ROLE, we can't trust the value.
3. Require superuser to use ALTER ROLE for a placeholder. This is what
we do today. Since we know a superuser set the value, we can always
apply it when the custom GUC is finally defined.
If this is an accurate representation of the options, it seems clear why
the superuser restriction is in place.
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
Thanks a lot for the feedback Nathan.
Taking your options into consideration, for me the correct behaviour should be:
- The ALTER ROLE placeholder should always be stored with a PGC_USERSET GucContext. It's a placeholder anyway, so it should be the less restrictive one. If the user wants to define it as PGC_SUSET or other this should be done through a custom extension.
- When an extension claims the placeholder, we should check the DefineCustomXXXVariable GucContext with PGC_USERSET. If there's a match, then the value gets applied, otherwise WARN or ERR.
The role GUCs get applied at login time right? So at this point we can WARN or ERR about the defined role GUCs.
What do you think?
On Mon, 18 Jul 2022 at 19:03, Nathan Bossart <nathandbossart@gmail.com> wrote:
On Fri, Jul 01, 2022 at 04:40:27PM -0700, Nathan Bossart wrote:
> On Sun, Jun 05, 2022 at 11:20:38PM -0500, Steve Chavez wrote:
>> However, defining placeholders at the role level require superuser:
>>
>> alter role myrole set my.username to 'tomas';
>> ERROR: permission denied to set parameter "my.username"
>>
>> Which is inconsistent and surprising behavior. I think it doesn't make
>> sense since you can already set them at the session or transaction
>> level(SET LOCAL my.username = 'tomas'). Enabling this would allow sidecar
>> services to store metadata scoped to its pertaining role.
>>
>> I've attached a patch that removes this restriction. From my testing, this
>> doesn't affect permission checking when an extension defines its custom GUC
>> variables.
>>
>> DefineCustomStringVariable("my.custom", NULL, NULL, &my_custom, NULL,
>> PGC_SUSET, ..);
>>
>> Using PGC_SUSET or PGC_SIGHUP will fail accordingly. Also no tests fail
>> when doing "make installcheck".
>
> IIUC you are basically proposing to revert a6dcd19 [0], but it is not clear
> to me why that is safe. Am I missing something?
I spent some more time looking into this, and I think I've constructed a
simple example that demonstrates the problem with removing this
restriction.
postgres=# CREATE ROLE test CREATEROLE;
CREATE ROLE
postgres=# CREATE ROLE other LOGIN;
CREATE ROLE
postgres=# GRANT CREATE ON DATABASE postgres TO other;
GRANT
postgres=# SET ROLE test;
SET
postgres=> ALTER ROLE other SET plperl.on_plperl_init = 'test';
ALTER ROLE
postgres=> \c postgres other
You are now connected to database "postgres" as user "other".
postgres=> CREATE EXTENSION plperl;
CREATE EXTENSION
postgres=> SHOW plperl.on_plperl_init;
plperl.on_plperl_init
-----------------------
test
(1 row)
In this example, the non-superuser role sets a placeholder GUC for another
role. This GUC becomes a PGC_SUSET GUC when plperl is loaded, so a
non-superuser role will have successfully set a PGC_SUSET GUC. If we had a
record of who ran ALTER ROLE, we might be able to apply appropriate
permissions checking when the module is loaded, but this information
doesn't exist in pg_db_role_setting. IIUC we have the following options:
1. Store information about who ran ALTER ROLE. I think there are a
couple of problems with this. For example, what happens if the
grantor was dropped or its privileges were altered? Should we
instead store the context of the user (i.e., PGC_USERSET or
PGC_SUSET)? Do we need to add entries to pg_depend?
2. Ignore or ERROR for any ALTER ROLE settings for custom GUCs. Since
we don't know who ran ALTER ROLE, we can't trust the value.
3. Require superuser to use ALTER ROLE for a placeholder. This is what
we do today. Since we know a superuser set the value, we can always
apply it when the custom GUC is finally defined.
If this is an accurate representation of the options, it seems clear why
the superuser restriction is in place.
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
On Tue, Jul 19, 2022 at 12:55:14AM -0500, Steve Chavez wrote: > Taking your options into consideration, for me the correct behaviour should > be: > > - The ALTER ROLE placeholder should always be stored with a PGC_USERSET > GucContext. It's a placeholder anyway, so it should be the less restrictive > one. If the user wants to define it as PGC_SUSET or other this should be > done through a custom extension. > - When an extension claims the placeholder, we should check the > DefineCustomXXXVariable GucContext with PGC_USERSET. If there's a match, > then the value gets applied, otherwise WARN or ERR. > The role GUCs get applied at login time right? So at this point we can > WARN or ERR about the defined role GUCs. > > What do you think? Hm. I would expect ALTER ROLE to store the PGC_SUSET context when executed by a superuser or a role with privileges via pg_parameter_acl. Storing all placeholder GUC settings as PGC_USERSET would make things more restrictive than they are today. For example, it would no longer be possible to apply any ALTER ROLE settings from superusers for placeholders that later become custom GUCS. -- Nathan Bossart Amazon Web Services: https://aws.amazon.com
At Tue, 19 Jul 2022 09:53:39 -0700, Nathan Bossart <nathandbossart@gmail.com> wrote in > On Tue, Jul 19, 2022 at 12:55:14AM -0500, Steve Chavez wrote: > > Taking your options into consideration, for me the correct behaviour should > > be: > > > > - The ALTER ROLE placeholder should always be stored with a PGC_USERSET > > GucContext. It's a placeholder anyway, so it should be the less restrictive > > one. If the user wants to define it as PGC_SUSET or other this should be > > done through a custom extension. > > - When an extension claims the placeholder, we should check the > > DefineCustomXXXVariable GucContext with PGC_USERSET. If there's a match, > > then the value gets applied, otherwise WARN or ERR. > > The role GUCs get applied at login time right? So at this point we can > > WARN or ERR about the defined role GUCs. > > > > What do you think? > > Hm. I would expect ALTER ROLE to store the PGC_SUSET context when executed > by a superuser or a role with privileges via pg_parameter_acl. Storing all > placeholder GUC settings as PGC_USERSET would make things more restrictive > than they are today. For example, it would no longer be possible to apply > any ALTER ROLE settings from superusers for placeholders that later become > custom GUCS. Currently placehoders are always created PGC_USERSET, thus non-superuser can set it. But if loaded module defines the custom variable as PGC_SUSET, the value set by the user is refused then the value from ALTER-ROLE-SET or otherwise the default value from DefineCustom*Variable is used. If the module defines it as PGC_USERSET, the last value is accepted. If a placehoders were created PGC_SUSET, non-superusers cannot set it on-session. But that behavior is not needed since loadable modules reject PGC_USERSET values as above. Returning to the topic, that operation can be allowed in PG15, having being granted by superuser using the GRANT SET ON PARMETER command. =# GRANT SET ON PARAMETER my.username TO r1; r1=> ALTER ROLE r1 SET my.username = 'hoge_user_x'; <success> r1=> \c r1=> => show my.username; my.username ------------- hoge_user_x (1 row) regards. -- Kyotaro Horiguchi NTT Open Source Software Center
Kyotaro Horiguchi <horikyota.ntt@gmail.com> writes:
> At Tue, 19 Jul 2022 09:53:39 -0700, Nathan Bossart <nathandbossart@gmail.com> wrote in
>> Hm. I would expect ALTER ROLE to store the PGC_SUSET context when executed
>> by a superuser or a role with privileges via pg_parameter_acl. Storing all
>> placeholder GUC settings as PGC_USERSET would make things more restrictive
>> than they are today. For example, it would no longer be possible to apply
>> any ALTER ROLE settings from superusers for placeholders that later become
>> custom GUCS.
> Returning to the topic, that operation can be allowed in PG15, having
> being granted by superuser using the GRANT SET ON PARMETER command.
I think that 13d838815 has completely changed the terms that this
discussion needs to be conducted under. It seems clear to me now
that if you want to relax this only-superusers restriction, what
you have to do is store the OID of the role that issued ALTER ROLE/DB SET,
and then apply the same checks that would be used in the ordinary case
where a placeholder is being filled in after being set intra-session.
That is, we'd no longer assume that a value coming from pg_db_role_setting
was set with superuser privileges, but we'd know exactly who did set it.
This might also tie into Nathan's question in another thread about
exactly what permissions should be required to issue ALTER ROLE/DB SET.
In particular I'm wondering if different permissions should be needed to
override an existing entry than if there is no existing entry. If not,
we could find ourselves downgrading a superuser-set entry to a
non-superuser-set entry, which might have bad consequences later
(eg, by rendering the entry nonfunctional because when we actually
load the extension we find out the GUC is SUSET).
Possibly related to this: I felt while working on 13d838815 that
PGC_SUSET and PGC_SU_BACKEND should be usable as GucContext
values for GUC variables, indicating that the GUC requires special
privileges to be set, but we should no longer use them as passed-in
GucContext values. That is, we should remove privilege tests from
the call sites, like this:
(void) set_config_option(stmt->name,
ExtractSetVariableArgs(stmt),
- (superuser() ? PGC_SUSET : PGC_USERSET),
+ PGC_USERSET,
PGC_S_SESSION,
action, true, 0, false);
and instead put that behavior inside set_config_option_ext, which
would want to look at superuser_arg(srole) instead, and indeed might
not need to do anything because pg_parameter_aclcheck would subsume
the test. I didn't pursue this further because it wasn't essential
to fixing the bug. But it seems relevant here, because that line of
thought leads to the conclusion that storing PGC_SUSET vs PGC_USERSET
is entirely the wrong approach.
There is a bunch of infrastructure work that has to be done if anyone
wants to make this happen:
* redesign physical representation of pg_db_role_setting
* be sure to clean up if a role mentioned in pg_db_role_setting is dropped
* pg_dump would need to be taught to dump the state of affairs correctly.
regards, tom lane
On Wed, Jul 20, 2022 at 11:50:10AM -0400, Tom Lane wrote: > I think that 13d838815 has completely changed the terms that this > discussion needs to be conducted under. It seems clear to me now > that if you want to relax this only-superusers restriction, what > you have to do is store the OID of the role that issued ALTER ROLE/DB SET, > and then apply the same checks that would be used in the ordinary case > where a placeholder is being filled in after being set intra-session. > That is, we'd no longer assume that a value coming from pg_db_role_setting > was set with superuser privileges, but we'd know exactly who did set it. I was imagining that the permissions checks would apply at ALTER ROLE/DB SET time, not at login time. Otherwise, changing a role's privileges might impact other roles' parameters, and it's not clear (at least to me) what should happen when the role is dropped. Another reason I imagined it this way is because that's basically how it works today. We assume that the pg_db_role_setting entry was added by a superuser, but we don't check that the user that ran ALTER ROLE/DB SET is still superuser every time you log in. > This might also tie into Nathan's question in another thread about > exactly what permissions should be required to issue ALTER ROLE/DB SET. > In particular I'm wondering if different permissions should be needed to > override an existing entry than if there is no existing entry. If not, > we could find ourselves downgrading a superuser-set entry to a > non-superuser-set entry, which might have bad consequences later > (eg, by rendering the entry nonfunctional because when we actually > load the extension we find out the GUC is SUSET). Yeah, this is why I suggested storing something that equates to PGC_SUSET any time a role is superuser or has grantable GUC permissions. > Possibly related to this: I felt while working on 13d838815 that > PGC_SUSET and PGC_SU_BACKEND should be usable as GucContext > values for GUC variables, indicating that the GUC requires special > privileges to be set, but we should no longer use them as passed-in > GucContext values. That is, we should remove privilege tests from > the call sites, like this: > > (void) set_config_option(stmt->name, > ExtractSetVariableArgs(stmt), > - (superuser() ? PGC_SUSET : PGC_USERSET), > + PGC_USERSET, > PGC_S_SESSION, > action, true, 0, false); > > and instead put that behavior inside set_config_option_ext, which > would want to look at superuser_arg(srole) instead, and indeed might > not need to do anything because pg_parameter_aclcheck would subsume > the test. I didn't pursue this further because it wasn't essential > to fixing the bug. But it seems relevant here, because that line of > thought leads to the conclusion that storing PGC_SUSET vs PGC_USERSET > is entirely the wrong approach. Couldn't ProcessGUCArray() use set_config_option_ext() with the context indicated by pg_db_role_setting? Also, instead of using PGC_USERSET in all the set_config_option() call sites, shouldn't we remove the "context" argument altogether? I am likely misunderstanding your proposal, but while I think simplifying set_config_option() is worthwhile, I don't see why it would preclude storing the context in pg_db_role_setting. -- Nathan Bossart Amazon Web Services: https://aws.amazon.com
On Thu, Jul 21, 2022 at 10:56:41AM -0700, Nathan Bossart wrote: > Couldn't ProcessGUCArray() use set_config_option_ext() with the context > indicated by pg_db_role_setting? Also, instead of using PGC_USERSET in all > the set_config_option() call sites, shouldn't we remove the "context" > argument altogether? I am likely misunderstanding your proposal, but while > I think simplifying set_config_option() is worthwhile, I don't see why it > would preclude storing the context in pg_db_role_setting. This thread has remained idle for a bit more than two months, so I have marked its CF entry as returned with feedback. -- Michael
Attachment
Hi! I'd like to resume this discussion. On Wed, Jul 20, 2022 at 6:50 PM Tom Lane <tgl@sss.pgh.pa.us> wrote: > Kyotaro Horiguchi <horikyota.ntt@gmail.com> writes: > > At Tue, 19 Jul 2022 09:53:39 -0700, Nathan Bossart <nathandbossart@gmail.com> wrote in > >> Hm. I would expect ALTER ROLE to store the PGC_SUSET context when executed > >> by a superuser or a role with privileges via pg_parameter_acl. Storing all > >> placeholder GUC settings as PGC_USERSET would make things more restrictive > >> than they are today. For example, it would no longer be possible to apply > >> any ALTER ROLE settings from superusers for placeholders that later become > >> custom GUCS. > > > Returning to the topic, that operation can be allowed in PG15, having > > being granted by superuser using the GRANT SET ON PARMETER command. > > I think that 13d838815 has completely changed the terms that this > discussion needs to be conducted under. It seems clear to me now > that if you want to relax this only-superusers restriction, what > you have to do is store the OID of the role that issued ALTER ROLE/DB SET, > and then apply the same checks that would be used in the ordinary case > where a placeholder is being filled in after being set intra-session. > That is, we'd no longer assume that a value coming from pg_db_role_setting > was set with superuser privileges, but we'd know exactly who did set it. This makes sense. But do we really need to store the OID of the role? validate_option_array_item() already checks if the placeholder option passes validation for PGC_SUSET. So, we can just save a flag indicating that this check was not successful. If so, then the value stored can be only used for PGC_USERSET. Do you think this would be correct? ------ Regards, Alexander Korotkov
Alexander Korotkov <aekorotkov@gmail.com> writes:
> This makes sense. But do we really need to store the OID of the role?
> validate_option_array_item() already checks if the placeholder option
> passes validation for PGC_SUSET. So, we can just save a flag
> indicating that this check was not successful. If so, then the value
> stored can be only used for PGC_USERSET. Do you think this would be
> correct?
Meh ... doesn't seem like much of an improvement. You still need
to store something that's not there now. This also seems to require
some shaky assumptions about decisions having been made when storing
still being valid later on. Given the possibility of granting or
revoking permissions for SET, I think we don't really want it to act
that way.
regards, tom lane
... BTW, re-reading the commit message for a0ffa885e:
One caveat is that PGC_USERSET GUCs are unaffected by the SET privilege
--- one could wish that those were handled by a revocable grant to
PUBLIC, but they are not, because we couldn't make it robust enough
for GUCs defined by extensions.
it suddenly struck me to wonder if the later 13d838815 changed the
situation enough to allow revisiting that problem, and/or if storing
the source role's OID in pg_db_role_setting would help.
I don't immediately recall all the problems that led us to leave USERSET
GUCs out of the feature, so maybe this is nuts; but maybe it isn't.
It'd be worth considering if we're trying to improve matters here.
regards, tom lane
On Sat, Nov 19, 2022 at 12:33 AM Tom Lane <tgl@sss.pgh.pa.us> wrote: > Alexander Korotkov <aekorotkov@gmail.com> writes: > > This makes sense. But do we really need to store the OID of the role? > > validate_option_array_item() already checks if the placeholder option > > passes validation for PGC_SUSET. So, we can just save a flag > > indicating that this check was not successful. If so, then the value > > stored can be only used for PGC_USERSET. Do you think this would be > > correct? > > Meh ... doesn't seem like much of an improvement. You still need > to store something that's not there now. Yes, but it wouldn't be needed to track dependencies of pg_role mentions in pg_db_role_setting. That seems to be a significant simplification. > This also seems to require > some shaky assumptions about decisions having been made when storing > still being valid later on. Given the possibility of granting or > revoking permissions for SET, I think we don't really want it to act > that way. Yes, it might be shaky. Consider user sets parameter pg_db_role_setting, and that appears to be capable only for PGC_USERSET. Next this user gets the SET permissions. Then this parameter needs to be set again in order for the new permission to take effect. But consider the other side. How should we handle stored OID of a role? Should the privilege checking be moved from "set time" to "run time"? Therefore, revoking SET permission from role may affect existing parameters in pg_db_role_setting. It feels like revoke of SET permission also aborts changes previously made with that permission. This is not how we normally do, and that seems confusing. I think if we implement the flag and make it user-visible, e.g. implement something like "ALTER ROLE ... SET ... USERSET;", then it might be the lesser confusing option. Thoughts? ------ Regards, Alexander Korotkov
On Sat, Nov 19, 2022 at 12:41 AM Tom Lane <tgl@sss.pgh.pa.us> wrote: > ... BTW, re-reading the commit message for a0ffa885e: > > One caveat is that PGC_USERSET GUCs are unaffected by the SET privilege > --- one could wish that those were handled by a revocable grant to > PUBLIC, but they are not, because we couldn't make it robust enough > for GUCs defined by extensions. > > it suddenly struck me to wonder if the later 13d838815 changed the > situation enough to allow revisiting that problem, and/or if storing > the source role's OID in pg_db_role_setting would help. > > I don't immediately recall all the problems that led us to leave USERSET > GUCs out of the feature, so maybe this is nuts; but maybe it isn't. > It'd be worth considering if we're trying to improve matters here. I think if we implement the user-visible USERSET flag for ALTER ROLE, then we might just check permissions for such parameters from the target role. ------ Regards, Alexander Korotkov
On Sat, Nov 19, 2022 at 4:02 AM Alexander Korotkov <aekorotkov@gmail.com> wrote: > On Sat, Nov 19, 2022 at 12:41 AM Tom Lane <tgl@sss.pgh.pa.us> wrote: > > ... BTW, re-reading the commit message for a0ffa885e: > > > > One caveat is that PGC_USERSET GUCs are unaffected by the SET privilege > > --- one could wish that those were handled by a revocable grant to > > PUBLIC, but they are not, because we couldn't make it robust enough > > for GUCs defined by extensions. > > > > it suddenly struck me to wonder if the later 13d838815 changed the > > situation enough to allow revisiting that problem, and/or if storing > > the source role's OID in pg_db_role_setting would help. > > > > I don't immediately recall all the problems that led us to leave USERSET > > GUCs out of the feature, so maybe this is nuts; but maybe it isn't. > > It'd be worth considering if we're trying to improve matters here. > > I think if we implement the user-visible USERSET flag for ALTER ROLE, > then we might just check permissions for such parameters from the > target role. I've drafted a patch implementing ALTER ROLE ... SET ... TO ... USER SET syntax. These options are working only for USERSET GUC variables, but require less privileges to set. I think there is no problem to implement Also it seems that this approach doesn't conflict with future privileges for USERSET GUCs [1]. I expect that USERSET GUCs should be available unless explicitly REVOKEd. That mean we should be able to check those privileges during ALTER ROLE. Opinions on the patch draft? Links 1. https://mail.google.com/mail/u/0/?ik=a20b091faa&view=om&permmsgid=msg-f%3A1749871710745577015 ------ Regards, Alexander Korotkov
Attachment
.On Sun, Nov 20, 2022 at 8:48 PM Alexander Korotkov <aekorotkov@gmail.com> wrote: > I've drafted a patch implementing ALTER ROLE ... SET ... TO ... USER SET syntax. > > These options are working only for USERSET GUC variables, but require > less privileges to set. I think there is no problem to implement > > Also it seems that this approach doesn't conflict with future > privileges for USERSET GUCs [1]. I expect that USERSET GUCs should be > available unless explicitly REVOKEd. That mean we should be able to > check those privileges during ALTER ROLE. > > Opinions on the patch draft? > > Links > 1. https://mail.google.com/mail/u/0/?ik=a20b091faa&view=om&permmsgid=msg-f%3A1749871710745577015 Uh, sorry for the wrong link. I meant https://www.postgresql.org/message-id/2271988.1668807706@sss.pgh.pa.us ------ Regards, Alexander Korotkov
Hey Alexander,
Looks like your latest patch addresses the original issue I posted!
So now I can create a placeholder with the USERSET modifier without a superuser, while non-USERSET placeholders still require superuser:
```sql
create role foo noinherit;
set role to foo;
alter role foo set prefix.bar to true user set;
ALTER ROLE
alter role foo set prefix.baz to true;
ERROR: permission denied to set parameter "prefix.baz"
set role to postgres;
alter role foo set prefix.baz to true;
ALTER ROLE
alter role foo set prefix.baz to true;
ALTER ROLE
```
Also USERSET gucs are marked(`(u)`) on `pg_db_role_setting`:
```sql
select * from pg_db_role_setting ;
setdatabase | setrole | setconfig
-------------+---------+--------------------------------------
0 | 16384 | {prefix.bar(u)=true,prefix.baz=true}
setdatabase | setrole | setconfig
-------------+---------+--------------------------------------
0 | 16384 | {prefix.bar(u)=true,prefix.baz=true}
```
Which I guess avoids the need for adding columns to `pg_catalog` and makes the "fix" simpler.
So from my side this all looks good!
Best regards,
Steve
On Sun, 20 Nov 2022 at 12:50, Alexander Korotkov <aekorotkov@gmail.com> wrote:
.On Sun, Nov 20, 2022 at 8:48 PM Alexander Korotkov
<aekorotkov@gmail.com> wrote:
> I've drafted a patch implementing ALTER ROLE ... SET ... TO ... USER SET syntax.
>
> These options are working only for USERSET GUC variables, but require
> less privileges to set. I think there is no problem to implement
>
> Also it seems that this approach doesn't conflict with future
> privileges for USERSET GUCs [1]. I expect that USERSET GUCs should be
> available unless explicitly REVOKEd. That mean we should be able to
> check those privileges during ALTER ROLE.
>
> Opinions on the patch draft?
>
> Links
> 1. https://mail.google.com/mail/u/0/?ik=a20b091faa&view=om&permmsgid=msg-f%3A1749871710745577015
Uh, sorry for the wrong link. I meant
https://www.postgresql.org/message-id/2271988.1668807706@sss.pgh.pa.us
------
Regards,
Alexander Korotkov
On Wed, Nov 23, 2022 at 1:53 AM Steve Chavez <steve@supabase.io> wrote: > So from my side this all looks good! Thank you for your feedback. The next revision of the patch is attached. It contains code improvements, comments and documentation. I'm going to also write sode tests. pg_db_role_setting doesn't seem to be well-covered with tests. I will probably need to write a new module into src/tests/modules to check now placeholders interacts with dynamically defined GUCs. ------ Regards, Alexander Korotkov
Attachment
On Thu, Dec 1, 2022 at 6:14 AM Alexander Korotkov <aekorotkov@gmail.com> wrote: > On Wed, Nov 23, 2022 at 1:53 AM Steve Chavez <steve@supabase.io> wrote: > > So from my side this all looks good! > > Thank you for your feedback. > > The next revision of the patch is attached. It contains code > improvements, comments and documentation. I'm going to also write > sode tests. pg_db_role_setting doesn't seem to be well-covered with > tests. I will probably need to write a new module into > src/tests/modules to check now placeholders interacts with dynamically > defined GUCs. Another revision of patch is attached. It's fixed now that USER SET values can't be used for PGC_SUSET parameters. Tests are added. That require new module test_pg_db_role_setting to check dynamically defined GUCs. ------ Regards, Alexander Korotkov
Attachment
> On Thu, Dec 1, 2022 at 6:14 AM Alexander Korotkov <aekorotkov@gmail.com> wrote: > > On Wed, Nov 23, 2022 at 1:53 AM Steve Chavez <steve@supabase.io> wrote: > > > So from my side this all looks good! > > > > Thank you for your feedback. > > > > The next revision of the patch is attached. It contains code > > improvements, comments and documentation. I'm going to also write > > sode tests. pg_db_role_setting doesn't seem to be well-covered with > > tests. I will probably need to write a new module into > > src/tests/modules to check now placeholders interacts with dynamically > > defined GUCs. > > Another revision of patch is attached. It's fixed now that USER SET > values can't be used for PGC_SUSET parameters. Tests are added. That > require new module test_pg_db_role_setting to check dynamically > defined GUCs. I've looked through the last version of a patch. The tests in v3 failed due to naming mismatches. I fixed this in v4 (PFA). The other thing that may seem unexpected: is whether the value should apply to the ordinary user only, encoded in the parameter name. The pro of this is that it doesn't break catalog compatibility by a separate field for GUC permissions a concept that doesn't exist today (and maybe not needed at all). Also, it makes the patch more minimalistic in the code. This is also fully compatible with the previous parameters naming due to parentheses being an unsupported symbol for the parameter name. I've also tried to revise the comments and docs a little bit to reflect the changes. The CI-enabled build of patch v4 for reference is at https://github.com/pashkinelfe/postgres/tree/placeholders-in-alter-role-v4 Overall the patch looks useful and good enough to be committed. Kind regards, Pavel Borisov, Supabase
Attachment
After posting the patch I've found my own typo in docs. So corrected it in v5 (PFA). Regards, Pavel.
Attachment
On Mon, Dec 5, 2022 at 2:27 PM Pavel Borisov <pashkin.elfe@gmail.com> wrote: > After posting the patch I've found my own typo in docs. So corrected > it in v5 (PFA). The new revision of the patch is attached. I've removed the mention of "(s)" suffix from the "Server Configuration" docs section. I think it might be confusing since this suffix isn't a part of the variable name. It is only used for storage. Instead, I've added the description of this suffix to the catalog structure description and psql documentation. Also, I've added psql tab completion for the USER SET flag, and made some enhancements to comments, tests, and commit message. ------ Regards, Alexander Korotkov
Attachment
Hi, Alexander! On Mon, 5 Dec 2022 at 17:51, Alexander Korotkov <aekorotkov@gmail.com> wrote: > > On Mon, Dec 5, 2022 at 2:27 PM Pavel Borisov <pashkin.elfe@gmail.com> wrote: > > After posting the patch I've found my own typo in docs. So corrected > > it in v5 (PFA). > > The new revision of the patch is attached. > > I've removed the mention of "(s)" suffix from the "Server > Configuration" docs section. I think it might be confusing since this > suffix isn't a part of the variable name. It is only used for storage. > Instead, I've added the description of this suffix to the catalog > structure description and psql documentation. > > Also, I've added psql tab completion for the USER SET flag, and made > some enhancements to comments, tests, and commit message. The changes in expected test results are somehow lost in v6, I've corrected them in v7. Otherwise, I've looked through the updated patch and it is good. Regards, Pavel.
Attachment
I couldn't find any discussion of the idea of adding "(s)" to the variable name in order to mark the variable userset in the catalog, and I have to admit I find it a bit strange. Are we really agreed that that's the way to proceed? -- Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/ "Having your biases confirmed independently is how scientific progress is made, and hence made our great society what it is today" (Mary Gardiner)
Alvaro Herrera <alvherre@alvh.no-ip.org> writes:
> I couldn't find any discussion of the idea of adding "(s)" to the
> variable name in order to mark the variable userset in the catalog, and
> I have to admit I find it a bit strange. Are we really agreed that
> that's the way to proceed?
I hadn't been paying close attention to this thread, sorry.
I agree that that seems like a very regrettable choice,
especially if you anticipate having to bump catversion anyway.
Better to add a bool column to the catalog.
regards, tom lane
On Mon, Dec 5, 2022 at 8:18 PM Tom Lane <tgl@sss.pgh.pa.us> wrote: > Alvaro Herrera <alvherre@alvh.no-ip.org> writes: > > I couldn't find any discussion of the idea of adding "(s)" to the > > variable name in order to mark the variable userset in the catalog, and > > I have to admit I find it a bit strange. Are we really agreed that > > that's the way to proceed? > > I hadn't been paying close attention to this thread, sorry. > > I agree that that seems like a very regrettable choice, > especially if you anticipate having to bump catversion anyway. I totally understand that this change requires a catversion bump. I've reflected this in the commit message. > Better to add a bool column to the catalog. What about adding a boolean array to the pg_db_role_setting? So, pg_db_role_setting would have the following columns. * setdatabase oid * setrole oid * setconfig text[] * setuser bool[] ------ Regards, Alexander Korotkov
On Mon, Dec 5, 2022 at 10:32 PM Alexander Korotkov <aekorotkov@gmail.com> wrote: > On Mon, Dec 5, 2022 at 8:18 PM Tom Lane <tgl@sss.pgh.pa.us> wrote: > > Alvaro Herrera <alvherre@alvh.no-ip.org> writes: > > > I couldn't find any discussion of the idea of adding "(s)" to the > > > variable name in order to mark the variable userset in the catalog, and > > > I have to admit I find it a bit strange. Are we really agreed that > > > that's the way to proceed? > > > > I hadn't been paying close attention to this thread, sorry. > > > > I agree that that seems like a very regrettable choice, > > especially if you anticipate having to bump catversion anyway. > > I totally understand that this change requires a catversion bump. > I've reflected this in the commit message. > > > Better to add a bool column to the catalog. > > What about adding a boolean array to the pg_db_role_setting? So, > pg_db_role_setting would have the following columns. > * setdatabase oid > * setrole oid > * setconfig text[] > * setuser bool[] The revised patch implements this way for storage USER SET flag. I think it really became more structured and less cumbersome. ------ Regards, Alexander Korotkov
Attachment
Hi, Alexander! On Tue, 6 Dec 2022 at 19:01, Alexander Korotkov <aekorotkov@gmail.com> wrote: > > On Mon, Dec 5, 2022 at 10:32 PM Alexander Korotkov <aekorotkov@gmail.com> wrote: > > On Mon, Dec 5, 2022 at 8:18 PM Tom Lane <tgl@sss.pgh.pa.us> wrote: > > > Alvaro Herrera <alvherre@alvh.no-ip.org> writes: > > > > I couldn't find any discussion of the idea of adding "(s)" to the > > > > variable name in order to mark the variable userset in the catalog, and > > > > I have to admit I find it a bit strange. Are we really agreed that > > > > that's the way to proceed? > > > > > > I hadn't been paying close attention to this thread, sorry. > > > > > > I agree that that seems like a very regrettable choice, > > > especially if you anticipate having to bump catversion anyway. > > > > I totally understand that this change requires a catversion bump. > > I've reflected this in the commit message. > > > > > Better to add a bool column to the catalog. > > > > What about adding a boolean array to the pg_db_role_setting? So, > > pg_db_role_setting would have the following columns. > > * setdatabase oid > > * setrole oid > > * setconfig text[] > > * setuser bool[] > > The revised patch implements this way for storage USER SET flag. > think it really became more structured and less cumbersome. I agree that the patch became more structured and the complications for string parameter suffixing have gone away. I've looked it through and don't see problems with it. The only two-lines fix regarding variable initializing may be relevant (see v9). Tests pass and CI is also happy with it. I'd like to set it ready for committer if no objections. Regards, Pavel Borisov, Supabase.
Attachment
On Wed, Dec 7, 2022 at 1:28 AM Pavel Borisov <pashkin.elfe@gmail.com> wrote: > On Tue, 6 Dec 2022 at 19:01, Alexander Korotkov <aekorotkov@gmail.com> wrote: > > > > On Mon, Dec 5, 2022 at 10:32 PM Alexander Korotkov <aekorotkov@gmail.com> wrote: > > > On Mon, Dec 5, 2022 at 8:18 PM Tom Lane <tgl@sss.pgh.pa.us> wrote: > > > > Alvaro Herrera <alvherre@alvh.no-ip.org> writes: > > > > > I couldn't find any discussion of the idea of adding "(s)" to the > > > > > variable name in order to mark the variable userset in the catalog, and > > > > > I have to admit I find it a bit strange. Are we really agreed that > > > > > that's the way to proceed? > > > > > > > > I hadn't been paying close attention to this thread, sorry. > > > > > > > > I agree that that seems like a very regrettable choice, > > > > especially if you anticipate having to bump catversion anyway. > > > > > > I totally understand that this change requires a catversion bump. > > > I've reflected this in the commit message. > > > > > > > Better to add a bool column to the catalog. > > > > > > What about adding a boolean array to the pg_db_role_setting? So, > > > pg_db_role_setting would have the following columns. > > > * setdatabase oid > > > * setrole oid > > > * setconfig text[] > > > * setuser bool[] > > > > The revised patch implements this way for storage USER SET flag. > > think it really became more structured and less cumbersome. > > I agree that the patch became more structured and the complications > for string parameter suffixing have gone away. I've looked it through > and don't see problems with it. The only two-lines fix regarding > variable initializing may be relevant (see v9). Tests pass and CI is > also happy with it. I'd like to set it ready for committer if no > objections. Thank you, Pavel. I've made few minor improvements in the docs and comments. I'm going to push this if no objections. ------ Regards, Alexander Korotkov
Attachment
On Wed, Dec 7, 2022 at 4:36 PM Alexander Korotkov <aekorotkov@gmail.com> wrote: > On Wed, Dec 7, 2022 at 1:28 AM Pavel Borisov <pashkin.elfe@gmail.com> wrote: > > On Tue, 6 Dec 2022 at 19:01, Alexander Korotkov <aekorotkov@gmail.com> wrote: > > > > > > On Mon, Dec 5, 2022 at 10:32 PM Alexander Korotkov <aekorotkov@gmail.com> wrote: > > > > On Mon, Dec 5, 2022 at 8:18 PM Tom Lane <tgl@sss.pgh.pa.us> wrote: > > > > > Alvaro Herrera <alvherre@alvh.no-ip.org> writes: > > > > > > I couldn't find any discussion of the idea of adding "(s)" to the > > > > > > variable name in order to mark the variable userset in the catalog, and > > > > > > I have to admit I find it a bit strange. Are we really agreed that > > > > > > that's the way to proceed? > > > > > > > > > > I hadn't been paying close attention to this thread, sorry. > > > > > > > > > > I agree that that seems like a very regrettable choice, > > > > > especially if you anticipate having to bump catversion anyway. > > > > > > > > I totally understand that this change requires a catversion bump. > > > > I've reflected this in the commit message. > > > > > > > > > Better to add a bool column to the catalog. > > > > > > > > What about adding a boolean array to the pg_db_role_setting? So, > > > > pg_db_role_setting would have the following columns. > > > > * setdatabase oid > > > > * setrole oid > > > > * setconfig text[] > > > > * setuser bool[] > > > > > > The revised patch implements this way for storage USER SET flag. > > > think it really became more structured and less cumbersome. > > > > I agree that the patch became more structured and the complications > > for string parameter suffixing have gone away. I've looked it through > > and don't see problems with it. The only two-lines fix regarding > > variable initializing may be relevant (see v9). Tests pass and CI is > > also happy with it. I'd like to set it ready for committer if no > > objections. > > Thank you, Pavel. > I've made few minor improvements in the docs and comments. > I'm going to push this if no objections. Pushed, thanks to everyone! ------ Regards, Alexander Korotkov
Hi, Alexander! > Pushed, thanks to everyone! Thank you! I've found a minor thing that makes the new test fail on sifaka and longfin build farm animals. If role names in regression don't start with "regress_" this invokes a warning. I've consulted in other modules regression tests e.g. in test_rls_hooks and changed the role naming accordingly. In essence, a fix is just a batch replace in test SQL and expected results. Regards, Pavel.
Attachment
Hi, Alexander! > Hi, Alexander! > > Pushed, thanks to everyone! > > Thank you! > I've found a minor thing that makes the new test fail on sifaka and > longfin build farm animals. If role names in regression don't start > with "regress_" this invokes a warning. I've consulted in other > modules regression tests e.g. in test_rls_hooks and changed the role > naming accordingly. In essence, a fix is just a batch replace in test > SQL and expected results. I see you already pushed the fix for this in beecbe8e5001. So no worries, it it not needed anymore. Regards, Pavel.
On Fri, Dec 9, 2022 at 2:57 PM Pavel Borisov <pashkin.elfe@gmail.com> wrote: > > > Pushed, thanks to everyone! > > > > Thank you! > > I've found a minor thing that makes the new test fail on sifaka and > > longfin build farm animals. If role names in regression don't start > > with "regress_" this invokes a warning. I've consulted in other > > modules regression tests e.g. in test_rls_hooks and changed the role > > naming accordingly. In essence, a fix is just a batch replace in test > > SQL and expected results. > > I see you already pushed the fix for this in beecbe8e5001. So no > worries, it it not needed anymore. OK. Thank you for keeping eyes on buildfarm. ------ Regards, Alexander Korotkov
On Fri, Dec 09, 2022 at 01:23:56PM +0300, Alexander Korotkov wrote:
> Pushed, thanks to everyone!
FYI: this causes meson test running ("installcheck") fail when run
twice. I guess that's expected to work, per:
b62303794efd97f2afb55f1e1b82fffae2cf8a2d
f31111bbe81db0e84fb486c6423a234c47091b30
6928484bda454f9ab2456d385b2d317f18b6bf1a
072710dff3eef4540f1c64d07890eb128535e212
b22b770683806db0a1c0a52a4601a3b6755891e0
--
Justin
Justin Pryzby <pryzby@telsasoft.com> writes:
> FYI: this causes meson test running ("installcheck") fail when run
> twice. I guess that's expected to work, per:
We do indeed expect that to work ... but I don't see any problem
with repeat "make installcheck" on HEAD. Can you provide more
detail about what you're seeing?
regards, tom lane
On Tue, Dec 27, 2022 at 01:58:14AM -0500, Tom Lane wrote:
> Justin Pryzby <pryzby@telsasoft.com> writes:
> > FYI: this causes meson test running ("installcheck") fail when run
> > twice. I guess that's expected to work, per:
>
> We do indeed expect that to work ... but I don't see any problem
> with repeat "make installcheck" on HEAD. Can you provide more
> detail about what you're seeing?
This fails when run more than once:
time meson test --setup running --print test_pg_db_role_setting-running/regress
@@ -1,12 +1,13 @@
CREATE EXTENSION test_pg_db_role_setting;
CREATE USER regress_super_user SUPERUSER;
+ERROR: role "regress_super_user" already exists
CREATE USER regress_regular_user;
+ERROR: role "regress_regular_user" already exists
...
It didn't fail for you because it says:
./src/test/modules/test_pg_db_role_setting/Makefile
+# disable installcheck for now
+NO_INSTALLCHECK = 1
It also says:
+# and also for now force NO_LOCALE and UTF8
+ENCODING = UTF8
+NO_LOCALE = 1
which was evidently copied from the "oat" tests, which have said that
since March (5b29a9f77, 7c51b7f7c).
It fails the same way with "make" if you change it to not disable
installcheck:
EXTRA_REGRESS_OPTS="--bindir=`pwd`/tmp_install/usr/local/pgsql/bin" PGHOST=/tmp make installcheck -C
src/test/modules/test_pg_db_role_setting
--
Justin
Justin Pryzby <pryzby@telsasoft.com> writes:
> This fails when run more than once:
> time meson test --setup running --print test_pg_db_role_setting-running/regress
Ah.
> It didn't fail for you because it says:
> ./src/test/modules/test_pg_db_role_setting/Makefile
> +# disable installcheck for now
> +NO_INSTALLCHECK = 1
So ... exactly why is the meson infrastructure failing to honor that?
This test looks sufficiently careless about its side-effects that
I completely agree with the decision to not run it in pre-existing
installations. Failing to drop a created superuser is just one of
its risk factors --- it also leaves around pg_db_role_setting entries.
regards, tom lane
On Tue, Dec 27, 2022 at 11:29:40PM -0500, Tom Lane wrote: > Justin Pryzby <pryzby@telsasoft.com> writes: > > This fails when run more than once: > > time meson test --setup running --print test_pg_db_role_setting-running/regress > > Ah. > > > It didn't fail for you because it says: > > ./src/test/modules/test_pg_db_role_setting/Makefile > > +# disable installcheck for now > > +NO_INSTALLCHECK = 1 > > So ... exactly why is the meson infrastructure failing to honor that? > This test looks sufficiently careless about its side-effects that > I completely agree with the decision to not run it in pre-existing > installations. Failing to drop a created superuser is just one of > its risk factors --- it also leaves around pg_db_role_setting entries. Meson doesn't try to parse the Makefiles (like the MSVC scripts) but (since 3f0e786ccb) has its own implementation, which involves setting 'runningcheck': false. 096dd80f3c seems to have copied the NO_INSTALLCHECK from oat's makefile, but didn't copy "runningcheck" from oat's meson.build (but did copy its regress_args). -- Justin
Hi, Justin! On Wed, 28 Dec 2022 at 21:28, Justin Pryzby <pryzby@telsasoft.com> wrote: > > On Tue, Dec 27, 2022 at 11:29:40PM -0500, Tom Lane wrote: > > Justin Pryzby <pryzby@telsasoft.com> writes: > > > This fails when run more than once: > > > time meson test --setup running --print test_pg_db_role_setting-running/regress > > > > Ah. > > > > > It didn't fail for you because it says: > > > ./src/test/modules/test_pg_db_role_setting/Makefile > > > +# disable installcheck for now > > > +NO_INSTALLCHECK = 1 > > > > So ... exactly why is the meson infrastructure failing to honor that? > > This test looks sufficiently careless about its side-effects that > > I completely agree with the decision to not run it in pre-existing > > installations. Failing to drop a created superuser is just one of > > its risk factors --- it also leaves around pg_db_role_setting entries. > > Meson doesn't try to parse the Makefiles (like the MSVC scripts) but > (since 3f0e786ccb) has its own implementation, which involves setting > 'runningcheck': false. > > 096dd80f3c seems to have copied the NO_INSTALLCHECK from oat's makefile, > but didn't copy "runningcheck" from oat's meson.build (but did copy its > regress_args). I completely agree with your analysis. Fixes by 3f0e786ccbf5 to oat and the other modules tests came just a couple of days before committing the main pg_db_role_setting commit 096dd80f3c so they were forgotten to be included into it. I support committing the same fix to pg_db_role_setting test and added this minor fix as a patch to this thread. Kind regards, and happy New year! Pavel Borisov, Supabase.
Attachment
Justin, Tom, Pavel, thank you for catching this. On Mon, Jan 2, 2023 at 11:54 AM Pavel Borisov <pashkin.elfe@gmail.com> wrote: > I completely agree with your analysis. Fixes by 3f0e786ccbf5 to oat > and the other modules tests came just a couple of days before > committing the main pg_db_role_setting commit 096dd80f3c so they were > forgotten to be included into it. > > I support committing the same fix to pg_db_role_setting test and added > this minor fix as a patch to this thread. I'm going to push this if no objections. > Kind regards, and happy New year! Thanks, and happy New Year to you! ------ Regards, Alexander Korotkov
On Mon, Jan 02, 2023 at 06:14:48PM +0300, Alexander Korotkov wrote: > I'm going to push this if no objections. I also suggest that meson.build should not copy regress_args. -- Justin
On Mon, Jan 2, 2023 at 6:42 PM Justin Pryzby <pryzby@telsasoft.com> wrote: > On Mon, Jan 02, 2023 at 06:14:48PM +0300, Alexander Korotkov wrote: > > I'm going to push this if no objections. > > I also suggest that meson.build should not copy regress_args. Good point, thanks. ------ Regards, Alexander Korotkov
Attachment
On Tue, 3 Jan 2023 at 09:29, Alexander Korotkov <aekorotkov@gmail.com> wrote: > > On Mon, Jan 2, 2023 at 6:42 PM Justin Pryzby <pryzby@telsasoft.com> wrote: > > On Mon, Jan 02, 2023 at 06:14:48PM +0300, Alexander Korotkov wrote: > > > I'm going to push this if no objections. > > > > I also suggest that meson.build should not copy regress_args. > > Good point, thanks. Nice, thanks! Isn't there the same reason to remove regress_args from meson.build in oat's test and possibly from other modules with runningcheck=false? Regards, Pavel Borisov
On Tue, Jan 3, 2023 at 11:51 AM Pavel Borisov <pashkin.elfe@gmail.com> wrote: > On Tue, 3 Jan 2023 at 09:29, Alexander Korotkov <aekorotkov@gmail.com> wrote: > > > > On Mon, Jan 2, 2023 at 6:42 PM Justin Pryzby <pryzby@telsasoft.com> wrote: > > > On Mon, Jan 02, 2023 at 06:14:48PM +0300, Alexander Korotkov wrote: > > > > I'm going to push this if no objections. > > > > > > I also suggest that meson.build should not copy regress_args. > > > > Good point, thanks. > Nice, thanks! > Isn't there the same reason to remove regress_args from meson.build in > oat's test and possibly from other modules with runningcheck=false? This makes sense to me too. I don't see anything specific in oat's regression test that requires setting regress_args. ------ Regards, Alexander Korotkov
Hi, Alexander! On Tue, 3 Jan 2023 at 13:48, Alexander Korotkov <aekorotkov@gmail.com> wrote: > > On Tue, Jan 3, 2023 at 11:51 AM Pavel Borisov <pashkin.elfe@gmail.com> wrote: > > On Tue, 3 Jan 2023 at 09:29, Alexander Korotkov <aekorotkov@gmail.com> wrote: > > > > > > On Mon, Jan 2, 2023 at 6:42 PM Justin Pryzby <pryzby@telsasoft.com> wrote: > > > > On Mon, Jan 02, 2023 at 06:14:48PM +0300, Alexander Korotkov wrote: > > > > > I'm going to push this if no objections. > > > > > > > > I also suggest that meson.build should not copy regress_args. > > > > > > Good point, thanks. > > Nice, thanks! > > Isn't there the same reason to remove regress_args from meson.build in > > oat's test and possibly from other modules with runningcheck=false? > > This makes sense to me too. I don't see anything specific in oat's > regression test that requires setting regress_args. Yes, it seems so. Regress args in oat's Makefile are added as a response to a buildfarm issues by 7c51b7f7cc08. They seem unneeded to be copied into meson.build with runningcheck=false. I may mistake but it seems to me that removing regress_args from meson.build with runningcheck=false is just to make it neat, not for functionality. So I consider fixing it in pg_db_role_setting, oat, or both of them optional. Regards, Pavel Borisov.
On Tue, Jan 03, 2023 at 02:20:38PM +0300, Pavel Borisov wrote: > Hi, Alexander! > > On Tue, 3 Jan 2023 at 13:48, Alexander Korotkov <aekorotkov@gmail.com> wrote: > > > > On Tue, Jan 3, 2023 at 11:51 AM Pavel Borisov <pashkin.elfe@gmail.com> wrote: > > > On Tue, 3 Jan 2023 at 09:29, Alexander Korotkov <aekorotkov@gmail.com> wrote: > > > > > > > > On Mon, Jan 2, 2023 at 6:42 PM Justin Pryzby <pryzby@telsasoft.com> wrote: > > > > > On Mon, Jan 02, 2023 at 06:14:48PM +0300, Alexander Korotkov wrote: > > > > > > I'm going to push this if no objections. > > > > > > > > > > I also suggest that meson.build should not copy regress_args. > > > > > > > > Good point, thanks. > > > Nice, thanks! > > > Isn't there the same reason to remove regress_args from meson.build in > > > oat's test and possibly from other modules with runningcheck=false? > > > > This makes sense to me too. I don't see anything specific in oat's > > regression test that requires setting regress_args. > Yes, it seems so. > Regress args in oat's Makefile are added as a response to a buildfarm > issues by 7c51b7f7cc08. They seem unneeded to be copied into > meson.build with runningcheck=false. I may mistake but it seems to me > that removing regress_args from meson.build with runningcheck=false is > just to make it neat, not for functionality. So I consider fixing it > in pg_db_role_setting, oat, or both of them optional. Right - my suggestion is to "uncopy" them from pg_db_role_setting, where they serve no purpose, since they shouldn't have been copied originally. On Tue, Jan 03, 2023 at 09:29:00AM +0300, Alexander Korotkov wrote: > On Mon, Jan 2, 2023 at 6:42 PM Justin Pryzby <pryzby@telsasoft.com> wrote: > > On Mon, Jan 02, 2023 at 06:14:48PM +0300, Alexander Korotkov wrote: > > > I'm going to push this if no objections. > > > > I also suggest that meson.build should not copy regress_args. > > Good point, thanks. I should've mentioned that the same things should be removed from Makefile, too... -- Justin
On Tue, 3 Jan 2023 at 17:28, Justin Pryzby <pryzby@telsasoft.com> wrote: > > On Tue, Jan 03, 2023 at 02:20:38PM +0300, Pavel Borisov wrote: > > Hi, Alexander! > > > > On Tue, 3 Jan 2023 at 13:48, Alexander Korotkov <aekorotkov@gmail.com> wrote: > > > > > > On Tue, Jan 3, 2023 at 11:51 AM Pavel Borisov <pashkin.elfe@gmail.com> wrote: > > > > On Tue, 3 Jan 2023 at 09:29, Alexander Korotkov <aekorotkov@gmail.com> wrote: > > > > > > > > > > On Mon, Jan 2, 2023 at 6:42 PM Justin Pryzby <pryzby@telsasoft.com> wrote: > > > > > > On Mon, Jan 02, 2023 at 06:14:48PM +0300, Alexander Korotkov wrote: > > > > > > > I'm going to push this if no objections. > > > > > > > > > > > > I also suggest that meson.build should not copy regress_args. > > > > > > > > > > Good point, thanks. > > > > Nice, thanks! > > > > Isn't there the same reason to remove regress_args from meson.build in > > > > oat's test and possibly from other modules with runningcheck=false? > > > > > > This makes sense to me too. I don't see anything specific in oat's > > > regression test that requires setting regress_args. > > Yes, it seems so. > > Regress args in oat's Makefile are added as a response to a buildfarm > > issues by 7c51b7f7cc08. They seem unneeded to be copied into > > meson.build with runningcheck=false. I may mistake but it seems to me > > that removing regress_args from meson.build with runningcheck=false is > > just to make it neat, not for functionality. So I consider fixing it > > in pg_db_role_setting, oat, or both of them optional. > > Right - my suggestion is to "uncopy" them from pg_db_role_setting, where > they serve no purpose, since they shouldn't have been copied originally. > > On Tue, Jan 03, 2023 at 09:29:00AM +0300, Alexander Korotkov wrote: > > On Mon, Jan 2, 2023 at 6:42 PM Justin Pryzby <pryzby@telsasoft.com> wrote: > > > On Mon, Jan 02, 2023 at 06:14:48PM +0300, Alexander Korotkov wrote: > > > > I'm going to push this if no objections. > > > > > > I also suggest that meson.build should not copy regress_args. > > > > Good point, thanks. > > I should've mentioned that the same things should be removed from > Makefile, too... > > -- Thanks, Justin! Attached is a new patch accordingly. Regards, Pavel Borisov!
Attachment
On Tue, Jan 3, 2023 at 5:28 PM Justin Pryzby <pryzby@telsasoft.com> wrote: > On Tue, Jan 03, 2023 at 09:29:00AM +0300, Alexander Korotkov wrote: > > On Mon, Jan 2, 2023 at 6:42 PM Justin Pryzby <pryzby@telsasoft.com> wrote: > > > I also suggest that meson.build should not copy regress_args. > > > > Good point, thanks. > > I should've mentioned that the same things should be removed from > Makefile, too... This makes sense too. See the attached patchset. ------ Regards, Alexander Korotkov
Attachment
On Tue, Jan 3, 2023 at 5:41 PM Pavel Borisov <pashkin.elfe@gmail.com> wrote: > On Tue, 3 Jan 2023 at 17:28, Justin Pryzby <pryzby@telsasoft.com> wrote: > > I should've mentioned that the same things should be removed from > > Makefile, too... > > > Thanks, Justin! > Attached is a new patch accordingly. Thank you. I've pushed my version, which is split into two commits. ------ Regards, Alexander Korotkov