AlterUserStmt anmd RoleSpec rules in grammar.y

Started by Pavel Golubover 8 years ago9 messages
#1Pavel Golub
pavel@microolap.com

Hello, hackers.

I need someone to throw some light on grammar (gram.y).
I'm investigating beta2 regression tests, and found new statement

`ALTER USER ALL SET application_name to 'SLAP';`
^^^

I know for sure that in beta1 this operator fails. So I decided to recheck gram.y:

AlterUserStmt:
ALTER USER RoleSpec SetResetClause;
....
RoleSpec: NonReservedWord
| CURRENT_USER
| SESSION_USER;

But *ALL is reserved word*! Thus "ALTER ROLE\USER ALL" should fail.

OK, I checked in Pg10 beta2, installer provided by EDB. It worked.

Then I asked someone to check this against fresh built server from
'master'. It failed.

So, the situation is:

1. Docs say this is correct statement:
https://www.postgresql.org/docs/devel/static/sql-alterrole.html

2. The sources in master don't support such production:
https://git.postgresql.org/gitweb/?p=postgresql.git;a=blob;f=src/backend/parser/gram.y;h=4b1ce09c445a5ee249a965ec0953b122df71eb6f;hb=refs/heads/master
Line 1179 for AlterUserSetStmt rule;
Line 14515 for RoleSpec rule;

3. EDB 10beta2 server supports it.

What's going on?

--
With best wishes,
Pavel mailto:pavel@gf.microolap.com

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

#2Tom Lane
tgl@sss.pgh.pa.us
In reply to: Pavel Golub (#1)
Re: AlterUserStmt anmd RoleSpec rules in grammar.y

Pavel Golub <pavel@microolap.com> writes:

I need someone to throw some light on grammar (gram.y).
I'm investigating beta2 regression tests, and found new statement

`ALTER USER ALL SET application_name to 'SLAP';`
^^^

You'll notice that that statement fails in the regression tests:

ALTER USER ALL SET application_name to 'SLAP';
ERROR: syntax error at or near "ALL"

The one that works is

ALTER ROLE ALL SET application_name to 'SLAP';

and the reason is that AlterRoleSetStmt has a separate production
for ALL, but AlterUserSetStmt doesn't. This seems a tad bizarre
though. Peter, you added that production (in commit 9475db3a4);
is this difference intentional or just an oversight? If it's
intentional, what's the reasoning?

BTW, I'm quite confused as to why these test cases (in rolenames.sql)
seem to predate that commit, and yet it did not change their results.

regards, tom lane

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

#3Pavel Golub
pavel@microolap.com
In reply to: Tom Lane (#2)
Re: AlterUserStmt anmd RoleSpec rules in grammar.y

Hello, Tom.

You wrote:

TL> Pavel Golub <pavel@microolap.com> writes:

I need someone to throw some light on grammar (gram.y).
I'm investigating beta2 regression tests, and found new statement

`ALTER USER ALL SET application_name to 'SLAP';`
^^^

TL> You'll notice that that statement fails in the regression tests:

TL> ALTER USER ALL SET application_name to 'SLAP';
TL> ERROR: syntax error at or near "ALL"

Oops! My bad!

TL> The one that works is

TL> ALTER ROLE ALL SET application_name to 'SLAP';

TL> and the reason is that AlterRoleSetStmt has a separate production
TL> for ALL, but AlterUserSetStmt doesn't.

Yeap, I see now separate rule for ALL in AlterRoleSetStmt.

TL> This seems a tad bizarre
TL> though. Peter, you added that production (in commit 9475db3a4);
TL> is this difference intentional or just an oversight? If it's
TL> intentional, what's the reasoning?

TL> BTW, I'm quite confused as to why these test cases (in rolenames.sql)
TL> seem to predate that commit, and yet it did not change their results.

TL> regards, tom lane

--
With best wishes,
Pavel mailto:pavel@gf.microolap.com

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

#4Pavel Golub
pavel@microolap.com
In reply to: Tom Lane (#2)
Re: AlterUserStmt anmd RoleSpec rules in grammar.y

Hello, Tom.

You wrote:

TL> Pavel Golub <pavel@microolap.com> writes:

I need someone to throw some light on grammar (gram.y).
I'm investigating beta2 regression tests, and found new statement

`ALTER USER ALL SET application_name to 'SLAP';`
^^^

TL> You'll notice that that statement fails in the regression tests:

TL> ALTER USER ALL SET application_name to 'SLAP';
TL> ERROR: syntax error at or near "ALL"

One more notice. ALTER USER ALL works in EnterpriseDB 10beta2
installer. That's weird. I thought EnterpriseDB uses official sources.

TL> The one that works is

TL> ALTER ROLE ALL SET application_name to 'SLAP';

TL> and the reason is that AlterRoleSetStmt has a separate production
TL> for ALL, but AlterUserSetStmt doesn't. This seems a tad bizarre
TL> though. Peter, you added that production (in commit 9475db3a4);
TL> is this difference intentional or just an oversight? If it's
TL> intentional, what's the reasoning?

TL> BTW, I'm quite confused as to why these test cases (in rolenames.sql)
TL> seem to predate that commit, and yet it did not change their results.

TL> regards, tom lane

--
With best wishes,
Pavel mailto:pavel@gf.microolap.com

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

#5Robert Haas
robertmhaas@gmail.com
In reply to: Pavel Golub (#4)
Re: AlterUserStmt anmd RoleSpec rules in grammar.y

On Thu, Jul 27, 2017 at 2:52 AM, Pavel Golub <pavel@microolap.com> wrote:

One more notice. ALTER USER ALL works in EnterpriseDB 10beta2
installer. That's weird. I thought EnterpriseDB uses official sources.

I find it really hard to believe that we're doing anything else. It
wouldn't make any sense to patch the PostgreSQL source code and then
release the installers as PostgreSQL installers. And if we *were*
going to do that, wouldn't we patch something more interesting than
the ALTER USER command? I don't know what's going on here but I have
a feeling that EnterpriseDB secretly maintaining patch sets that we
inject into our PostgreSQL installers is not that thing.

Adding a few EDB people.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

#6Peter Eisentraut
peter.eisentraut@2ndquadrant.com
In reply to: Tom Lane (#2)
1 attachment(s)
Re: AlterUserStmt anmd RoleSpec rules in grammar.y

On 7/26/17 11:29, Tom Lane wrote:

You'll notice that that statement fails in the regression tests:

ALTER USER ALL SET application_name to 'SLAP';
ERROR: syntax error at or near "ALL"

The one that works is

ALTER ROLE ALL SET application_name to 'SLAP';

and the reason is that AlterRoleSetStmt has a separate production
for ALL, but AlterUserSetStmt doesn't. This seems a tad bizarre
though. Peter, you added that production (in commit 9475db3a4);
is this difference intentional or just an oversight? If it's
intentional, what's the reasoning?

That looks like a bug to me. ALTER USER also does not support the IN
DATABASE clause, so the code deviation might have started there already.

I propose the attached patch to clean this up.

For backpatching, I could develop some less invasive versions.

--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Attachments:

0001-Further-unify-ROLE-and-USER-command-grammar-rules.patchtext/plain; charset=UTF-8; name=0001-Further-unify-ROLE-and-USER-command-grammar-rules.patch; x-mac-creator=0; x-mac-type=0Download
From 48a7c95b7c8243626362c7845a45cdb036028f7e Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <peter_e@gmx.net>
Date: Mon, 31 Jul 2017 20:36:32 -0400
Subject: [PATCH] Further unify ROLE and USER command grammar rules

ALTER USER ... SET did not support all the syntax variants of ALTER ROLE
...  SET.  Fix that, and to avoid further deviations of this kind, unify
many the grammar rules for ROLE/USER/GROUP commands.

Reported-by: Pavel Golub <pavel@microolap.com>
---
 doc/src/sgml/ref/alter_user.sgml        |   8 +--
 src/backend/parser/gram.y               | 105 +++++++++++---------------------
 src/test/regress/expected/rolenames.out |  10 +--
 3 files changed, 43 insertions(+), 80 deletions(-)

diff --git a/doc/src/sgml/ref/alter_user.sgml b/doc/src/sgml/ref/alter_user.sgml
index 9b8a39b376..411a6dcc38 100644
--- a/doc/src/sgml/ref/alter_user.sgml
+++ b/doc/src/sgml/ref/alter_user.sgml
@@ -38,10 +38,10 @@
 
 ALTER USER <replaceable class="PARAMETER">name</replaceable> RENAME TO <replaceable>new_name</replaceable>
 
-ALTER USER <replaceable class="PARAMETER">role_specification</replaceable> SET <replaceable>configuration_parameter</replaceable> { TO | = } { <replaceable>value</replaceable> | DEFAULT }
-ALTER USER <replaceable class="PARAMETER">role_specification</replaceable> SET <replaceable>configuration_parameter</replaceable> FROM CURRENT
-ALTER USER <replaceable class="PARAMETER">role_specification</replaceable> RESET <replaceable>configuration_parameter</replaceable>
-ALTER USER <replaceable class="PARAMETER">role_specification</replaceable> RESET ALL
+ALTER USER { <replaceable class="PARAMETER">role_specification</replaceable> | ALL } [ IN DATABASE <replaceable class="PARAMETER">database_name</replaceable> ] SET <replaceable>configuration_parameter</replaceable> { TO | = } { <replaceable>value</replaceable> | DEFAULT }
+ALTER USER { <replaceable class="PARAMETER">role_specification</replaceable> | ALL } [ IN DATABASE <replaceable class="PARAMETER">database_name</replaceable> ] SET <replaceable>configuration_parameter</replaceable> FROM CURRENT
+ALTER USER { <replaceable class="PARAMETER">role_specification</replaceable> | ALL } [ IN DATABASE <replaceable class="PARAMETER">database_name</replaceable> ] RESET <replaceable>configuration_parameter</replaceable>
+ALTER USER { <replaceable class="PARAMETER">role_specification</replaceable> | ALL } [ IN DATABASE <replaceable class="PARAMETER">database_name</replaceable> ] RESET ALL
 
 <phrase>where <replaceable class="PARAMETER">role_specification</replaceable> can be:</phrase>
 
diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y
index 4b1ce09c44..e20bf5ec55 100644
--- a/src/backend/parser/gram.y
+++ b/src/backend/parser/gram.y
@@ -250,7 +250,7 @@ static Node *makeRecursiveViewSelect(char *relname, List *aliases, Node *query);
 		AlterObjectDependsStmt AlterObjectSchemaStmt AlterOwnerStmt
 		AlterOperatorStmt AlterSeqStmt AlterSystemStmt AlterTableStmt
 		AlterTblSpcStmt AlterExtensionStmt AlterExtensionContentsStmt AlterForeignTableStmt
-		AlterCompositeTypeStmt AlterUserStmt AlterUserMappingStmt AlterUserSetStmt
+		AlterCompositeTypeStmt AlterUserMappingStmt
 		AlterRoleStmt AlterRoleSetStmt AlterPolicyStmt
 		AlterDefaultPrivilegesStmt DefACLAction
 		AnalyzeStmt ClosePortalStmt ClusterStmt CommentStmt
@@ -262,9 +262,9 @@ static Node *makeRecursiveViewSelect(char *relname, List *aliases, Node *query);
 		CreateAssertStmt CreateTransformStmt CreateTrigStmt CreateEventTrigStmt
 		CreateUserStmt CreateUserMappingStmt CreateRoleStmt CreatePolicyStmt
 		CreatedbStmt DeclareCursorStmt DefineStmt DeleteStmt DiscardStmt DoStmt
-		DropGroupStmt DropOpClassStmt DropOpFamilyStmt DropPLangStmt DropStmt
+		DropOpClassStmt DropOpFamilyStmt DropPLangStmt DropStmt
 		DropAssertStmt DropCastStmt DropRoleStmt
-		DropUserStmt DropdbStmt DropTableSpaceStmt
+		DropdbStmt DropTableSpaceStmt
 		DropTransformStmt
 		DropUserMappingStmt ExplainStmt FetchStmt
 		GrantStmt GrantRoleStmt ImportForeignSchemaStmt IndexStmt InsertStmt
@@ -841,8 +841,6 @@ stmt :
 			| AlterTSConfigurationStmt
 			| AlterTSDictionaryStmt
 			| AlterUserMappingStmt
-			| AlterUserSetStmt
-			| AlterUserStmt
 			| AnalyzeStmt
 			| CheckPointStmt
 			| ClosePortalStmt
@@ -890,7 +888,6 @@ stmt :
 			| DoStmt
 			| DropAssertStmt
 			| DropCastStmt
-			| DropGroupStmt
 			| DropOpClassStmt
 			| DropOpFamilyStmt
 			| DropOwnedStmt
@@ -900,7 +897,6 @@ stmt :
 			| DropTableSpaceStmt
 			| DropTransformStmt
 			| DropRoleStmt
-			| DropUserStmt
 			| DropUserMappingStmt
 			| DropdbStmt
 			| ExecuteStmt
@@ -1130,6 +1126,14 @@ AlterRoleStmt:
 					n->options = $5;
 					$$ = (Node *)n;
 				 }
+			| ALTER USER RoleSpec opt_with AlterOptRoleList
+				 {
+					AlterRoleStmt *n = makeNode(AlterRoleStmt);
+					n->role = $3;
+					n->action = +1;	/* add, if there are members */
+					n->options = $5;
+					$$ = (Node *)n;
+				 }
 		;
 
 opt_in_database:
@@ -1154,37 +1158,23 @@ AlterRoleSetStmt:
 					n->setstmt = $5;
 					$$ = (Node *)n;
 				}
-		;
-
-
-/*****************************************************************************
- *
- * Alter a postgresql DBMS user
- *
- *****************************************************************************/
-
-AlterUserStmt:
-			ALTER USER RoleSpec opt_with AlterOptRoleList
-				 {
-					AlterRoleStmt *n = makeNode(AlterRoleStmt);
+			| ALTER USER RoleSpec opt_in_database SetResetClause
+				{
+					AlterRoleSetStmt *n = makeNode(AlterRoleSetStmt);
 					n->role = $3;
-					n->action = +1;	/* add, if there are members */
-					n->options = $5;
+					n->database = $4;
+					n->setstmt = $5;
 					$$ = (Node *)n;
-				 }
-		;
-
-
-AlterUserSetStmt:
-			ALTER USER RoleSpec SetResetClause
+				}
+			| ALTER USER ALL opt_in_database SetResetClause
 				{
 					AlterRoleSetStmt *n = makeNode(AlterRoleSetStmt);
-					n->role = $3;
-					n->database = NULL;
-					n->setstmt = $4;
+					n->role = NULL;
+					n->database = $4;
+					n->setstmt = $5;
 					$$ = (Node *)n;
 				}
-			;
+		;
 
 
 /*****************************************************************************
@@ -1211,17 +1201,7 @@ DropRoleStmt:
 					n->roles = $5;
 					$$ = (Node *)n;
 				}
-			;
-
-/*****************************************************************************
- *
- * Drop a postgresql DBMS user
- *
- * XXX As with DROP ROLE, no CASCADE/RESTRICT here.
- *****************************************************************************/
-
-DropUserStmt:
-			DROP USER role_list
+			| DROP USER role_list
 				{
 					DropRoleStmt *n = makeNode(DropRoleStmt);
 					n->missing_ok = FALSE;
@@ -1235,6 +1215,20 @@ DropUserStmt:
 					n->missing_ok = TRUE;
 					$$ = (Node *)n;
 				}
+			| DROP GROUP_P role_list
+				{
+					DropRoleStmt *n = makeNode(DropRoleStmt);
+					n->missing_ok = FALSE;
+					n->roles = $3;
+					$$ = (Node *)n;
+				}
+			| DROP GROUP_P IF_P EXISTS role_list
+				{
+					DropRoleStmt *n = makeNode(DropRoleStmt);
+					n->missing_ok = TRUE;
+					n->roles = $5;
+					$$ = (Node *)n;
+				}
 			;
 
 
@@ -1281,31 +1275,6 @@ add_drop:	ADD_P									{ $$ = +1; }
 
 /*****************************************************************************
  *
- * Drop a postgresql group
- *
- * XXX As with DROP ROLE, no CASCADE/RESTRICT here.
- *****************************************************************************/
-
-DropGroupStmt:
-			DROP GROUP_P role_list
-				{
-					DropRoleStmt *n = makeNode(DropRoleStmt);
-					n->missing_ok = FALSE;
-					n->roles = $3;
-					$$ = (Node *)n;
-				}
-			| DROP GROUP_P IF_P EXISTS role_list
-				{
-					DropRoleStmt *n = makeNode(DropRoleStmt);
-					n->missing_ok = TRUE;
-					n->roles = $5;
-					$$ = (Node *)n;
-				}
-		;
-
-
-/*****************************************************************************
- *
  * Manipulate a schema
  *
  *****************************************************************************/
diff --git a/src/test/regress/expected/rolenames.out b/src/test/regress/expected/rolenames.out
index fd058e4f7d..dce82f5de7 100644
--- a/src/test/regress/expected/rolenames.out
+++ b/src/test/regress/expected/rolenames.out
@@ -310,9 +310,9 @@ ERROR:  syntax error at or near "CURRENT_ROLE"
 LINE 1: ALTER USER CURRENT_ROLE WITH LOGIN;
                    ^
 ALTER USER ALL WITH REPLICATION; -- error
-ERROR:  syntax error at or near "ALL"
+ERROR:  syntax error at or near "WITH"
 LINE 1: ALTER USER ALL WITH REPLICATION;
-                   ^
+                       ^
 ALTER USER SESSION_ROLE WITH NOREPLICATION; -- error
 ERROR:  role "session_role" does not exist
 ALTER USER PUBLIC WITH NOREPLICATION; -- error
@@ -392,9 +392,6 @@ ALTER USER SESSION_USER SET application_name to 'BAR';
 ALTER USER "current_user" SET application_name to 'FOOFOO';
 ALTER USER "Public" SET application_name to 'BARBAR';
 ALTER USER ALL SET application_name to 'SLAP';
-ERROR:  syntax error at or near "ALL"
-LINE 1: ALTER USER ALL SET application_name to 'SLAP';
-                   ^
 SELECT * FROM chksetconfig();
  db  |       role       |  rolkeyword  |         setconfig         
 -----+------------------+--------------+---------------------------
@@ -419,9 +416,6 @@ ALTER USER SESSION_USER RESET application_name;
 ALTER USER "current_user" RESET application_name;
 ALTER USER "Public" RESET application_name;
 ALTER USER ALL RESET application_name;
-ERROR:  syntax error at or near "ALL"
-LINE 1: ALTER USER ALL RESET application_name;
-                   ^
 SELECT * FROM chksetconfig();
  db | role | rolkeyword | setconfig 
 ----+------+------------+-----------
-- 
2.13.3

#7Pavel Golub
pavel@microolap.com
In reply to: Robert Haas (#5)
Re: AlterUserStmt anmd RoleSpec rules in grammar.y

Hello, Robert.

Sorry, if I was rough. My English is not so excellent. My point is
that I was trying to distinguish behavior of EDB installer and
"build from source" PG.

And the result is that EDB executes ALTER USER and I don't know why.

You wrote:

RH> On Thu, Jul 27, 2017 at 2:52 AM, Pavel Golub <pavel@microolap.com> wrote:

One more notice. ALTER USER ALL works in EnterpriseDB 10beta2
installer. That's weird. I thought EnterpriseDB uses official sources.

RH> I find it really hard to believe that we're doing anything else. It
RH> wouldn't make any sense to patch the PostgreSQL source code and then
RH> release the installers as PostgreSQL installers. And if we *were*
RH> going to do that, wouldn't we patch something more interesting than
RH> the ALTER USER command? I don't know what's going on here but I have
RH> a feeling that EnterpriseDB secretly maintaining patch sets that we
RH> inject into our PostgreSQL installers is not that thing.

RH> Adding a few EDB people.

--
With best wishes,
Pavel mailto:pavel@gf.microolap.com

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

#8Robert Haas
robertmhaas@gmail.com
In reply to: Pavel Golub (#7)
Re: AlterUserStmt anmd RoleSpec rules in grammar.y

On Tue, Aug 1, 2017 at 8:42 AM, Pavel Golub <pavel@microolap.com> wrote:

Sorry, if I was rough. My English is not so excellent. My point is
that I was trying to distinguish behavior of EDB installer and
"build from source" PG.

And the result is that EDB executes ALTER USER and I don't know why.

I don't know why, either. Are you sure you haven't made some mistake
in the testing?

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

#9Peter Eisentraut
peter.eisentraut@2ndquadrant.com
In reply to: Peter Eisentraut (#6)
Re: AlterUserStmt anmd RoleSpec rules in grammar.y

On 7/31/17 20:42, Peter Eisentraut wrote:

That looks like a bug to me. ALTER USER also does not support the IN
DATABASE clause, so the code deviation might have started there already.

I propose the attached patch to clean this up.

For backpatching, I could develop some less invasive versions.

done and done

--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers