Re: Possible regression setting GUCs on \connect

2023-05-18 Thread Tom Lane
Robert Haas writes: > This discussion made me go back and look at the commit in question. My > opinion is that the feature as it was committed is quite hard to > understand. The documentation for it said this: "Specifies that > variable should be set on behalf of ordinary role." But what does

Re: Possible regression setting GUCs on \connect

2023-05-18 Thread Robert Haas
On Wed, May 17, 2023 at 1:31 PM Alexander Korotkov wrote: > I have carefully noted your concerns regarding the USER SET patch that > I've committed. It's clear that you have strong convictions about > this, particularly in relation to your plan of storing the setter role > OID in

Re: Possible regression setting GUCs on \connect

2023-05-17 Thread Jonathan S. Katz
On 5/17/23 1:30 PM, Alexander Korotkov wrote: Tom, On Wed, May 17, 2023 at 3:08 PM Tom Lane wrote: Amit Kapila writes: Tom/Nathan, do you have any further suggestions here? My recommendation is to revert this feature. I do not see any way that we won't regret it as a poor design. I

Re: Possible regression setting GUCs on \connect

2023-05-17 Thread Alexander Korotkov
Tom, On Wed, May 17, 2023 at 3:08 PM Tom Lane wrote: > Amit Kapila writes: > > Tom/Nathan, do you have any further suggestions here? > > My recommendation is to revert this feature. I do not see any > way that we won't regret it as a poor design. I have carefully noted your concerns regarding

Re: Possible regression setting GUCs on \connect

2023-05-17 Thread Jonathan S. Katz
On 5/17/23 12:47 PM, Nathan Bossart wrote: On Wed, May 17, 2023 at 08:08:36AM -0400, Tom Lane wrote: Amit Kapila writes: Tom/Nathan, do you have any further suggestions here? My recommendation is to revert this feature. I do not see any way that we won't regret it as a poor design. I

Re: Possible regression setting GUCs on \connect

2023-05-17 Thread Nathan Bossart
On Wed, May 17, 2023 at 08:08:36AM -0400, Tom Lane wrote: > Amit Kapila writes: >> Tom/Nathan, do you have any further suggestions here? > > My recommendation is to revert this feature. I do not see any > way that we won't regret it as a poor design. I agree. The problem seems worth solving,

Re: Possible regression setting GUCs on \connect

2023-05-17 Thread Tom Lane
Amit Kapila writes: > Tom/Nathan, do you have any further suggestions here? My recommendation is to revert this feature. I do not see any way that we won't regret it as a poor design. regards, tom lane

Re: Possible regression setting GUCs on \connect

2023-05-17 Thread Alexander Korotkov
Hi, Amit. On Wed, May 17, 2023 at 9:47 AM Amit Kapila wrote: > I see there are mainly three concerns (a) Avoid adding the new option > USER SET, (b) The behavior of this feature varies from the precedents > set by a0ffa885e and 13d838815, (c) As per discussion, not following > 13d838815 could

Re: Possible regression setting GUCs on \connect

2023-05-17 Thread Amit Kapila
On Fri, Apr 28, 2023 at 5:01 AM Alexander Korotkov wrote: > > On Fri, Apr 28, 2023 at 1:38 AM Tom Lane wrote: > > Nathan Bossart writes: > > > On Thu, Apr 27, 2023 at 03:22:04PM -0400, Tom Lane wrote: > > >> The right way to do this was not to add some > > >> poorly-explained option to ALTER

Re: Possible regression setting GUCs on \connect

2023-05-17 Thread Michael Paquier
On Sun, Apr 30, 2023 at 12:25:20PM -0400, Jonathan S. Katz wrote: > [RMT hat] > > I read through the original thread[1] to understand the use case and also > the concerns, but I need to study [1] and this thread a bit more before I > can form an opinion. > > The argument that there is "demand

Re: Possible regression setting GUCs on \connect

2023-04-30 Thread Jonathan S. Katz
On 4/28/23 12:29 PM, Pavel Borisov wrote: Hi! On Fri, 28 Apr 2023 at 17:42, Jonathan S. Katz wrote: On 4/27/23 8:04 PM, Alexander Korotkov wrote: On Fri, Apr 28, 2023 at 2:30 AM Alexander Korotkov wrote: Additionally, I think if we start recording role OID, then we need a full set of

Re: Possible regression setting GUCs on \connect

2023-04-28 Thread Pavel Borisov
Hi! On Fri, 28 Apr 2023 at 17:42, Jonathan S. Katz wrote: > > On 4/27/23 8:04 PM, Alexander Korotkov wrote: > > On Fri, Apr 28, 2023 at 2:30 AM Alexander Korotkov > > wrote: > >> Additionally, I think if we start recording role OID, then we need a > >> full set of management clauses for each

Re: Possible regression setting GUCs on \connect

2023-04-28 Thread Jonathan S. Katz
On 4/27/23 8:04 PM, Alexander Korotkov wrote: On Fri, Apr 28, 2023 at 2:30 AM Alexander Korotkov wrote: Additionally, I think if we start recording role OID, then we need a full set of management clauses for each individual option ownership. Otherwise, we would leave this new role OID without

Re: Possible regression setting GUCs on \connect

2023-04-27 Thread Alexander Korotkov
On Fri, Apr 28, 2023 at 2:30 AM Alexander Korotkov wrote: > Additionally, I think if we start recording role OID, then we need a > full set of management clauses for each individual option ownership. > Otherwise, we would leave this new role OID without necessarily > management facilities. But

Re: Possible regression setting GUCs on \connect

2023-04-27 Thread Alexander Korotkov
On Fri, Apr 28, 2023 at 1:38 AM Tom Lane wrote: > Nathan Bossart writes: > > On Thu, Apr 27, 2023 at 03:22:04PM -0400, Tom Lane wrote: > >> The right way to do this was not to add some > >> poorly-explained option to ALTER ROLE, but to record the role OID that > >> issued the ALTER ROLE, and

Re: Possible regression setting GUCs on \connect

2023-04-27 Thread Tom Lane
Nathan Bossart writes: > On Thu, Apr 27, 2023 at 03:22:04PM -0400, Tom Lane wrote: >> The right way to do this was not to add some >> poorly-explained option to ALTER ROLE, but to record the role OID that >> issued the ALTER ROLE, and then to check when loading the ALTER ROLE >> setting whether

Re: Possible regression setting GUCs on \connect

2023-04-27 Thread Nathan Bossart
On Thu, Apr 27, 2023 at 03:22:04PM -0400, Tom Lane wrote: > The right way to do this was not to add some > poorly-explained option to ALTER ROLE, but to record the role OID that > issued the ALTER ROLE, and then to check when loading the ALTER ROLE > setting whether that role (still) has the right

Re: Possible regression setting GUCs on \connect

2023-04-27 Thread Tom Lane
Nathan Bossart writes: > I suspect the problem is that GUCArrayDelete() is using the wrong Datum: > -newUsersetArray = construct_array_builtin(, 1, BOOLOID); > +newUsersetArray = construct_array_builtin(, 1, > BOOLOID); Ah, should have checked my mail earlier.

Re: Possible regression setting GUCs on \connect

2023-04-27 Thread Tom Lane
David Steele writes: > On 4/27/23 21:16, Tom Lane wrote: >> I tried to replicate this per that recipe, but it works for me: > I included the errors in the expect log so I could link to them. So test > success means the error is happening. Ah, got it. I added some debug output to the test, and

Re: Possible regression setting GUCs on \connect

2023-04-27 Thread Alexander Korotkov
.On Thu, Apr 27, 2023 at 9:55 PM Nathan Bossart wrote: > On Thu, Apr 27, 2023 at 09:53:23PM +0300, Alexander Korotkov wrote: > > Thanks to everybody for catching and investigating this. > > Nathan, I'd like to push it myself. I'm also going to check the code > > for similar errors. > > Sounds

Re: Possible regression setting GUCs on \connect

2023-04-27 Thread Nathan Bossart
On Thu, Apr 27, 2023 at 09:53:23PM +0300, Alexander Korotkov wrote: > Thanks to everybody for catching and investigating this. > Nathan, I'd like to push it myself. I'm also going to check the code > for similar errors. Sounds good! -- Nathan Bossart Amazon Web Services: https://aws.amazon.com

Re: Possible regression setting GUCs on \connect

2023-04-27 Thread Alexander Korotkov
Hi! On Thu, Apr 27, 2023 at 9:51 PM Nathan Bossart wrote: > On Thu, Apr 27, 2023 at 09:47:33PM +0300, David Steele wrote: > > That seems to work. The errors are now gone. > > Great. Barring objections, I'll plan on committing this shortly. Thanks to everybody for catching and investigating

Re: Possible regression setting GUCs on \connect

2023-04-27 Thread Nathan Bossart
On Thu, Apr 27, 2023 at 09:47:33PM +0300, David Steele wrote: > That seems to work. The errors are now gone. Great. Barring objections, I'll plan on committing this shortly. -- Nathan Bossart Amazon Web Services: https://aws.amazon.com

Re: Possible regression setting GUCs on \connect

2023-04-27 Thread David Steele
On 4/27/23 21:43, Nathan Bossart wrote: I suspect the problem is that GUCArrayDelete() is using the wrong Datum: diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c index 9dd624b3ae..ee9f87e7f2 100644 --- a/src/backend/utils/misc/guc.c +++ b/src/backend/utils/misc/guc.c @@

Re: Possible regression setting GUCs on \connect

2023-04-27 Thread Nathan Bossart
I suspect the problem is that GUCArrayDelete() is using the wrong Datum: diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c index 9dd624b3ae..ee9f87e7f2 100644 --- a/src/backend/utils/misc/guc.c +++ b/src/backend/utils/misc/guc.c @@ -6496,7 +6496,7 @@

Re: Possible regression setting GUCs on \connect

2023-04-27 Thread David Steele
On 4/27/23 21:16, Tom Lane wrote: David Steele writes: Seems plausible. This can be reproduced by cloning [1] into contrib and running `make check`. I can work out another test case but it may not end up being simpler. [1] https://github.com/pgaudit/pgaudit/tree/dev-pg16-ci I tried to

Re: Possible regression setting GUCs on \connect

2023-04-27 Thread Tom Lane
David Steele writes: > Seems plausible. This can be reproduced by cloning [1] into contrib and > running `make check`. I can work out another test case but it may not > end up being simpler. > [1] https://github.com/pgaudit/pgaudit/tree/dev-pg16-ci I tried to replicate this per that recipe,

Re: Possible regression setting GUCs on \connect

2023-04-27 Thread David Steele
On 4/27/23 19:13, Tom Lane wrote: David Steele writes: I have been updating pgAudit for PG16 and ran into the following issue in the regression tests: \connect - user1 WARNING: permission denied to set parameter "pgaudit.log_level" This happens after switching back and forth a few times

Re: Possible regression setting GUCs on \connect

2023-04-27 Thread Tom Lane
David Steele writes: > I have been updating pgAudit for PG16 and ran into the following issue > in the regression tests: > \connect - user1 > WARNING: permission denied to set parameter "pgaudit.log_level" > This happens after switching back and forth a few times between the > current user

Possible regression setting GUCs on \connect

2023-04-27 Thread David Steele
Hackers, I have been updating pgAudit for PG16 and ran into the following issue in the regression tests: \connect - user1 WARNING: permission denied to set parameter "pgaudit.log_level" This happens after switching back and forth a few times between the current user when the regression