CREATE ROLE IF NOT EXISTS

david.christensen@crunchydata.com

about 4 years ago

In reply to: Isaac Morland (#2)

Re: CREATE ROLE IF NOT EXISTS

On Tue, Oct 19, 2021 at 4:29 PM Isaac Morland <isaac.morland@gmail.com>
wrote:

On Tue, 19 Oct 2021 at 16:12, David Christensen <
david.christensen@crunchydata.com> wrote:

Greetings -hackers,

Enclosed is a patch that implements CREATE ROLE IF NOT EXISTS (along with
the same support for USER/GROUP). This is a fairly straightforward
approach in that we do no validation of anything other than existence, with
the user needing to ensure that permissions/grants are set up in the proper
way.

One little tricky aspect that occurs to me is the ALTER ROLE to set the
role flag options: it really needs to mention *all* the available options
if it is to leave the role in a specific state regardless of how it started
out. For example, if the existing role has BYPASSRLS but you want the
default NOBYPASSRLS you have to say so explicitly.

Because of this, I think my preference, based just on thinking about
setting the flag options, would be for CREATE OR REPLACE.

However, I'm wondering about the role name options: IN ROLE, ROLE, ADMIN.
With OR REPLACE should they replace the set of memberships or augment it?
Either seems potentially problematic to me. By contrast it’s absolutely
clear what IF NOT EXISTS should do with these.

So I’m not sure what I think overall.

Sure, the ambiguity here for merging options was exactly the reason I went
with the IF NOT EXISTS route. Whatever concerns with merging already exist
with ALTER ROLE, so nothing new is introduced by this functionality, at
least that was my original thought.

David

Daniel Gustafsson

daniel@yesql.se

about 4 years ago

In reply to: David Christensen (#1)

Re: CREATE ROLE IF NOT EXISTS

On 19 Oct 2021, at 22:12, David Christensen <david.christensen@crunchydata.com> wrote:

Greetings -hackers,

Enclosed is a patch that implements CREATE ROLE IF NOT EXISTS (along with the same support for USER/GROUP). This is a fairly straightforward approach in that we do no validation of anything other than existence, with the user needing to ensure that permissions/grants are set up in the proper way.

Comments?

This fails the roleattributes test in "make check", with what seems to be a
trivial change in the output. Can you please submit a rebased version fixing
the test?

--
Daniel Gustafsson https://vmware.com/

david.christensen@crunchydata.com

about 4 years ago

In reply to: Daniel Gustafsson (#4)

1 attachment(s)

Re: CREATE ROLE IF NOT EXISTS

This fails the roleattributes test in "make check", with what seems to be a
trivial change in the output. Can you please submit a rebased version
fixing
the test?

Updated version attached.

David

Attachments:

CREATE-ROLE-IF-NOT-EXISTS-v2.patchapplication/octet-stream; name=CREATE-ROLE-IF-NOT-EXISTS-v2.patchDownload

From c9a859017ddd69152b25cbe7e9c127f12a33ef78 Mon Sep 17 00:00:00 2001
From: David Christensen <david.christensen@crunchydata.com>
Date: Tue, 19 Oct 2021 12:23:50 -0500
Subject: [PATCH] Support CREATE ROLE IF NOT EXISTS for roles, users, and
 groups

By choosing to implement CREATE ROLE IF NOT EXISTS instead of CREATE OR REPLACE ROLE, we can return
an existing oid while not throwing an error when using this form of the CREATE ROLE statement.  This
does mean that if writing code which expects a specific state for the ROLE with specific attributes,
you will likely want to separate these out into two separate statements:

    CREATE ROLE IF NOT EXISTS my_role;
    ALTER ROLE my_role WITH INHERIT SUPERUSER CREATEDB;

Instead of:

    CREATE ROLE IF NOT EXISTS my_role WITH INHERIT SUPERUSER CREATEDB;

While the second form will DWYM in cases where the role does not exist, it will not change the
existing object to reflect the role permissions passed in, and will instead keep the existing
permissions.  This is a conscious choice at this point.

What this *does* mean is that you can avoid issues caused by needing to (e.g.) DROP and immediately
CREATE a user/role when the intent is just "make sure this role exists".
---
 doc/src/sgml/ref/create_group.sgml           |  2 +-
 doc/src/sgml/ref/create_role.sgml            | 13 ++++-
 doc/src/sgml/ref/create_user.sgml            |  2 +-
 src/backend/commands/user.c                  | 23 +++++++--
 src/backend/nodes/copyfuncs.c                |  1 +
 src/backend/nodes/equalfuncs.c               |  1 +
 src/backend/parser/gram.y                    | 36 +++++++++++--
 src/include/nodes/parsenodes.h               |  1 +
 src/test/regress/expected/roleattributes.out | 53 ++++++++++++++++++++
 src/test/regress/sql/roleattributes.sql      | 28 +++++++++++
 10 files changed, 149 insertions(+), 11 deletions(-)

diff --git a/doc/src/sgml/ref/create_group.sgml b/doc/src/sgml/ref/create_group.sgml
index d124c98eb5..299873bb71 100644
--- a/doc/src/sgml/ref/create_group.sgml
+++ b/doc/src/sgml/ref/create_group.sgml
@@ -21,7 +21,7 @@ PostgreSQL documentation
 
  <refsynopsisdiv>
 <synopsis>
-CREATE GROUP <replaceable class="parameter">name</replaceable> [ [ WITH ] <replaceable class="parameter">option</replaceable> [ ... ] ]
+CREATE GROUP [ IF NOT EXISTS ] <replaceable class="parameter">name</replaceable> [ [ WITH ] <replaceable class="parameter">option</replaceable> [ ... ] ]
 
 <phrase>where <replaceable class="parameter">option</replaceable> can be:</phrase>
 
diff --git a/doc/src/sgml/ref/create_role.sgml b/doc/src/sgml/ref/create_role.sgml
index b6a4ea1f72..a21e2d6fe6 100644
--- a/doc/src/sgml/ref/create_role.sgml
+++ b/doc/src/sgml/ref/create_role.sgml
@@ -21,7 +21,7 @@ PostgreSQL documentation
 
  <refsynopsisdiv>
 <synopsis>
-CREATE ROLE <replaceable class="parameter">name</replaceable> [ [ WITH ] <replaceable class="parameter">option</replaceable> [ ... ] ]
+CREATE ROLE [ IF NOT EXISTS ] <replaceable class="parameter">name</replaceable> [ [ WITH ] <replaceable class="parameter">option</replaceable> [ ... ] ]
 
 <phrase>where <replaceable class="parameter">option</replaceable> can be:</phrase>
 
@@ -74,6 +74,17 @@ in sync when changing the above synopsis!
   <title>Parameters</title>
 
     <variablelist>
+     <varlistentry>
+      <term><literal>IF NOT EXISTS</literal></term>
+      <listitem>
+       <para>
+        Do not throw an error if the role with the same name already exists. A notice
+        is issued in this case.  Note that there is no guarantee that the existing role
+        is anything like the one that would have been created.
+       </para>
+      </listitem>
+     </varlistentry>
+
      <varlistentry>
       <term><replaceable class="parameter">name</replaceable></term>
       <listitem>
diff --git a/doc/src/sgml/ref/create_user.sgml b/doc/src/sgml/ref/create_user.sgml
index 48d2089238..9749db8233 100644
--- a/doc/src/sgml/ref/create_user.sgml
+++ b/doc/src/sgml/ref/create_user.sgml
@@ -21,7 +21,7 @@ PostgreSQL documentation
 
  <refsynopsisdiv>
 <synopsis>
-CREATE USER <replaceable class="parameter">name</replaceable> [ [ WITH ] <replaceable class="parameter">option</replaceable> [ ... ] ]
+CREATE USER [ IF NOT EXISTS ] <replaceable class="parameter">name</replaceable> [ [ WITH ] <replaceable class="parameter">option</replaceable> [ ... ] ]
 
 <phrase>where <replaceable class="parameter">option</replaceable> can be:</phrase>
 
diff --git a/src/backend/commands/user.c b/src/backend/commands/user.c
index aa69821be4..044fc8107d 100644
--- a/src/backend/commands/user.c
+++ b/src/backend/commands/user.c
@@ -304,11 +304,24 @@ CreateRole(ParseState *pstate, CreateRoleStmt *stmt)
 	pg_authid_rel = table_open(AuthIdRelationId, RowExclusiveLock);
 	pg_authid_dsc = RelationGetDescr(pg_authid_rel);
 
-	if (OidIsValid(get_role_oid(stmt->role, true)))
-		ereport(ERROR,
-				(errcode(ERRCODE_DUPLICATE_OBJECT),
-				 errmsg("role \"%s\" already exists",
-						stmt->role)));
+	roleid = get_role_oid(stmt->role, true);
+
+	if (OidIsValid(roleid))
+	{
+		if (stmt->if_not_exists)
+		{
+			table_close(pg_authid_rel, NoLock);
+			ereport(NOTICE,
+					(errmsg("role \"%s\" already exists, skipping",
+							stmt->role)));
+			return roleid;
+		}
+		else
+			ereport(ERROR,
+					(errcode(ERRCODE_DUPLICATE_OBJECT),
+					 errmsg("role \"%s\" already exists",
+							stmt->role)));
+	}
 
 	/* Convert validuntil to internal form */
 	if (validUntil)
diff --git a/src/backend/nodes/copyfuncs.c b/src/backend/nodes/copyfuncs.c
index 82464c9889..39b4811002 100644
--- a/src/backend/nodes/copyfuncs.c
+++ b/src/backend/nodes/copyfuncs.c
@@ -4515,6 +4515,7 @@ _copyCreateRoleStmt(const CreateRoleStmt *from)
 	COPY_SCALAR_FIELD(stmt_type);
 	COPY_STRING_FIELD(role);
 	COPY_NODE_FIELD(options);
+	COPY_SCALAR_FIELD(if_not_exists);
 
 	return newnode;
 }
diff --git a/src/backend/nodes/equalfuncs.c b/src/backend/nodes/equalfuncs.c
index f537d3eb96..582846e6a3 100644
--- a/src/backend/nodes/equalfuncs.c
+++ b/src/backend/nodes/equalfuncs.c
@@ -2129,6 +2129,7 @@ _equalCreateRoleStmt(const CreateRoleStmt *a, const CreateRoleStmt *b)
 	COMPARE_SCALAR_FIELD(stmt_type);
 	COMPARE_STRING_FIELD(role);
 	COMPARE_NODE_FIELD(options);
+	COMPARE_SCALAR_FIELD(if_not_exists);
 
 	return true;
 }
diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y
index d0eb80e69c..c3b41441df 100644
--- a/src/backend/parser/gram.y
+++ b/src/backend/parser/gram.y
@@ -1055,12 +1055,22 @@ CallStmt:	CALL func_application
  *****************************************************************************/
 
 CreateRoleStmt:
-			CREATE ROLE RoleId opt_with OptRoleList
+			CREATE ROLE IF_P NOT EXISTS RoleId opt_with OptRoleList
+				{
+					CreateRoleStmt *n = makeNode(CreateRoleStmt);
+					n->stmt_type = ROLESTMT_ROLE;
+					n->role = $6;
+					n->options = $8;
+					n->if_not_exists = true;
+					$$ = (Node *)n;
+				}
+			| CREATE ROLE RoleId opt_with OptRoleList
 				{
 					CreateRoleStmt *n = makeNode(CreateRoleStmt);
 					n->stmt_type = ROLESTMT_ROLE;
 					n->role = $3;
 					n->options = $5;
+					n->if_not_exists = false;
 					$$ = (Node *)n;
 				}
 		;
@@ -1211,12 +1221,22 @@ CreateOptRoleElem:
  *****************************************************************************/
 
 CreateUserStmt:
-			CREATE USER RoleId opt_with OptRoleList
+			CREATE USER IF_P NOT EXISTS RoleId opt_with OptRoleList
+				{
+					CreateRoleStmt *n = makeNode(CreateRoleStmt);
+					n->stmt_type = ROLESTMT_USER;
+					n->role = $6;
+					n->options = $8;
+					n->if_not_exists = true;
+					$$ = (Node *)n;
+				}
+			| CREATE USER RoleId opt_with OptRoleList
 				{
 					CreateRoleStmt *n = makeNode(CreateRoleStmt);
 					n->stmt_type = ROLESTMT_USER;
 					n->role = $3;
 					n->options = $5;
+					n->if_not_exists = false;
 					$$ = (Node *)n;
 				}
 		;
@@ -1350,12 +1370,22 @@ DropRoleStmt:
  *****************************************************************************/
 
 CreateGroupStmt:
-			CREATE GROUP_P RoleId opt_with OptRoleList
+			CREATE GROUP_P IF_P NOT EXISTS RoleId opt_with OptRoleList
+				{
+					CreateRoleStmt *n = makeNode(CreateRoleStmt);
+					n->stmt_type = ROLESTMT_GROUP;
+					n->role = $6;
+					n->options = $8;
+					n->if_not_exists = true;
+					$$ = (Node *)n;
+				}
+			| CREATE GROUP_P RoleId opt_with OptRoleList
 				{
 					CreateRoleStmt *n = makeNode(CreateRoleStmt);
 					n->stmt_type = ROLESTMT_GROUP;
 					n->role = $3;
 					n->options = $5;
+					n->if_not_exists = false;
 					$$ = (Node *)n;
 				}
 		;
diff --git a/src/include/nodes/parsenodes.h b/src/include/nodes/parsenodes.h
index 49123e28a4..7f20e1ca93 100644
--- a/src/include/nodes/parsenodes.h
+++ b/src/include/nodes/parsenodes.h
@@ -2622,6 +2622,7 @@ typedef struct CreateRoleStmt
 	RoleStmtType stmt_type;		/* ROLE/USER/GROUP */
 	char	   *role;			/* role name */
 	List	   *options;		/* List of DefElem nodes */
+	bool       if_not_exists;	/* Skip role creation if exists */
 } CreateRoleStmt;
 
 typedef struct AlterRoleStmt
diff --git a/src/test/regress/expected/roleattributes.out b/src/test/regress/expected/roleattributes.out
index 5e6969b173..1a0fc13b22 100644
--- a/src/test/regress/expected/roleattributes.out
+++ b/src/test/regress/expected/roleattributes.out
@@ -247,3 +247,56 @@ DROP ROLE regress_test_def_replication;
 DROP ROLE regress_test_replication;
 DROP ROLE regress_test_def_bypassrls;
 DROP ROLE regress_test_bypassrls;
+-- CREATE ROLE IF NOT EXISTS
+CREATE ROLE regress_test_exists_role;
+CREATE ROLE regress_test_exists_role;
+ERROR:  role "regress_test_exists_role" already exists
+CREATE ROLE IF NOT EXISTS regress_test_exists_role;
+NOTICE:  role "regress_test_exists_role" already exists, skipping
+DROP ROLE regress_test_exists_role;
+CREATE ROLE regress_test_exists_role WITH NOINHERIT;
+CREATE ROLE IF NOT EXISTS regress_test_exists_role WITH INHERIT;
+NOTICE:  role "regress_test_exists_role" already exists, skipping
+SELECT rolname, rolinherit FROM pg_authid WHERE rolname = 'regress_test_exists_role';
+         rolname          | rolinherit 
+--------------------------+------------
+ regress_test_exists_role | f
+(1 row)
+
+ALTER ROLE regress_test_exists_role WITH INHERIT;
+SELECT rolname, rolinherit FROM pg_authid WHERE rolname = 'regress_test_exists_role';
+         rolname          | rolinherit 
+--------------------------+------------
+ regress_test_exists_role | t
+(1 row)
+
+DROP ROLE regress_test_exists_role;
+CREATE USER regress_test_exists_user;
+CREATE USER regress_test_exists_user;
+ERROR:  role "regress_test_exists_user" already exists
+CREATE USER IF NOT EXISTS regress_test_exists_user;
+NOTICE:  role "regress_test_exists_user" already exists, skipping
+DROP USER regress_test_exists_user;
+CREATE USER regress_test_exists_user WITH NOINHERIT;
+CREATE USER IF NOT EXISTS regress_test_exists_user WITH INHERIT;
+NOTICE:  role "regress_test_exists_user" already exists, skipping
+SELECT rolname, rolinherit FROM pg_authid WHERE rolname = 'regress_test_exists_user';
+         rolname          | rolinherit 
+--------------------------+------------
+ regress_test_exists_user | f
+(1 row)
+
+ALTER USER regress_test_exists_user WITH INHERIT;
+SELECT rolname, rolinherit FROM pg_authid WHERE rolname = 'regress_test_exists_user';
+         rolname          | rolinherit 
+--------------------------+------------
+ regress_test_exists_user | t
+(1 row)
+
+DROP USER regress_test_exists_user;
+CREATE GROUP regress_test_exists_group;
+CREATE GROUP regress_test_exists_group;
+ERROR:  role "regress_test_exists_group" already exists
+CREATE GROUP IF NOT EXISTS regress_test_exists_group;
+NOTICE:  role "regress_test_exists_group" already exists, skipping
+DROP GROUP regress_test_exists_group;
diff --git a/src/test/regress/sql/roleattributes.sql b/src/test/regress/sql/roleattributes.sql
index c961b2d730..b6f79c25cd 100644
--- a/src/test/regress/sql/roleattributes.sql
+++ b/src/test/regress/sql/roleattributes.sql
@@ -96,3 +96,31 @@ DROP ROLE regress_test_def_replication;
 DROP ROLE regress_test_replication;
 DROP ROLE regress_test_def_bypassrls;
 DROP ROLE regress_test_bypassrls;
+
+-- CREATE ROLE IF NOT EXISTS
+CREATE ROLE regress_test_exists_role;
+CREATE ROLE regress_test_exists_role;
+CREATE ROLE IF NOT EXISTS regress_test_exists_role;
+DROP ROLE regress_test_exists_role;
+CREATE ROLE regress_test_exists_role WITH NOINHERIT;
+CREATE ROLE IF NOT EXISTS regress_test_exists_role WITH INHERIT;
+SELECT rolname, rolinherit FROM pg_authid WHERE rolname = 'regress_test_exists_role';
+ALTER ROLE regress_test_exists_role WITH INHERIT;
+SELECT rolname, rolinherit FROM pg_authid WHERE rolname = 'regress_test_exists_role';
+DROP ROLE regress_test_exists_role;
+
+CREATE USER regress_test_exists_user;
+CREATE USER regress_test_exists_user;
+CREATE USER IF NOT EXISTS regress_test_exists_user;
+DROP USER regress_test_exists_user;
+CREATE USER regress_test_exists_user WITH NOINHERIT;
+CREATE USER IF NOT EXISTS regress_test_exists_user WITH INHERIT;
+SELECT rolname, rolinherit FROM pg_authid WHERE rolname = 'regress_test_exists_user';
+ALTER USER regress_test_exists_user WITH INHERIT;
+SELECT rolname, rolinherit FROM pg_authid WHERE rolname = 'regress_test_exists_user';
+DROP USER regress_test_exists_user;
+
+CREATE GROUP regress_test_exists_group;
+CREATE GROUP regress_test_exists_group;
+CREATE GROUP IF NOT EXISTS regress_test_exists_group;
+DROP GROUP regress_test_exists_group;
-- 
2.30.1 (Apple Git-130)

Tom Lane

tgl@sss.pgh.pa.us

about 4 years ago

In reply to: David Christensen (#5)

Re: CREATE ROLE IF NOT EXISTS

David Christensen <david.christensen@crunchydata.com> writes:

Updated version attached.

I'm generally pretty down on IF NOT EXISTS semantics in all cases,
but it seems particularly dangerous for something as fundamental
to privilege checks as a role. It's not hard at all to conjure up
scenarios in which this permits privilege escalation. That is,
Alice wants to create role Bob and give it some privileges, but
she's lazy and writes a quick-and-dirty script using CREATE ROLE
IF NOT EXISTS. Meanwhile Charlie sneaks in and creates Bob first,
and then grants it to himself. Now Alice's script is giving away
all sorts of privilege to Charlie. (Admittedly, Charlie must have
CREATEROLE privilege already, but that doesn't mean he has every
privilege that Alice has --- especially not as we continue working
to slice the superuser salami ever more finely.)

Do we really need this?

regards, tom lane

Daniel Gustafsson

daniel@yesql.se

about 4 years ago

In reply to: Tom Lane (#6)

Re: CREATE ROLE IF NOT EXISTS

On 3 Nov 2021, at 23:18, Tom Lane <tgl@sss.pgh.pa.us> wrote:

I'm generally pretty down on IF NOT EXISTS semantics in all cases,
but it seems particularly dangerous for something as fundamental
to privilege checks as a role. It's not hard at all to conjure up
scenarios in which this permits privilege escalation. That is,
Alice wants to create role Bob and give it some privileges, but
she's lazy and writes a quick-and-dirty script using CREATE ROLE
IF NOT EXISTS. Meanwhile Charlie sneaks in and creates Bob first,
and then grants it to himself. Now Alice's script is giving away
all sorts of privilege to Charlie. (Admittedly, Charlie must have
CREATEROLE privilege already, but that doesn't mean he has every
privilege that Alice has --- especially not as we continue working
to slice the superuser salami ever more finely.)

I agree with this take, I don't think the convenience outweighs the risk in
this case.

--
Daniel Gustafsson https://vmware.com/

sfrost@snowman.net

about 4 years ago

In reply to: Daniel Gustafsson (#7)

Re: CREATE ROLE IF NOT EXISTS

Greetings,

* Daniel Gustafsson (daniel@yesql.se) wrote:

On 3 Nov 2021, at 23:18, Tom Lane <tgl@sss.pgh.pa.us> wrote:
I'm generally pretty down on IF NOT EXISTS semantics in all cases,
but it seems particularly dangerous for something as fundamental
to privilege checks as a role. It's not hard at all to conjure up
scenarios in which this permits privilege escalation. That is,
Alice wants to create role Bob and give it some privileges, but
she's lazy and writes a quick-and-dirty script using CREATE ROLE
IF NOT EXISTS. Meanwhile Charlie sneaks in and creates Bob first,
and then grants it to himself. Now Alice's script is giving away
all sorts of privilege to Charlie. (Admittedly, Charlie must have
CREATEROLE privilege already, but that doesn't mean he has every
privilege that Alice has --- especially not as we continue working
to slice the superuser salami ever more finely.)

I agree with this take, I don't think the convenience outweighs the risk in
this case.

I don't quite follow this. The entire point of Alice writing a script
that uses IF NOT EXISTS is to have that command not fail if, indeed,
that role already exists, but for the rest of the script to be run.
That there's some potential attacker with CREATEROLE running around
creating roles that they think someone *else* might create is really
stretching things to a very questionable level- especially with
CREATEROLE where Charlie could just CREATE a new role which is a member
of Bob anyway after the fact and then GRANT that role to themselves.

The reason this thread was started is that it's a pretty clearly useful
thing to be able to use IF NOT EXISTS for CREATE ROLE and I don't agree
with the justification that we shouldn't allow it because someone might
use it carelessly. For one, I really doubt that's actually a risk at
all, but more importantly there's a lot of very good use-cases where
it'll be used correctly and not having it means having to do other ugly
things like write a pl/pgsql function which checks pg_roles and would
end up having the exact same risk but be a lot more clunky. And, yes,
people are already doing that. Let's give them useful tools and
document that they be careful with them, not make them jump through
hoops.

Thanks,

Stephen

Tom Lane

tgl@sss.pgh.pa.us

about 4 years ago

In reply to: Stephen Frost (#8)

Re: CREATE ROLE IF NOT EXISTS

Stephen Frost <sfrost@snowman.net> writes:

I don't quite follow this. The entire point of Alice writing a script
that uses IF NOT EXISTS is to have that command not fail if, indeed,
that role already exists, but for the rest of the script to be run.
That there's some potential attacker with CREATEROLE running around
creating roles that they think someone *else* might create is really
stretching things to a very questionable level- especially with
CREATEROLE where Charlie could just CREATE a new role which is a member
of Bob anyway after the fact and then GRANT that role to themselves.

I agree that as things stand, CREATEROLE is powerful enough that Charlie
doesn't need any subterfuge to become a member of the Bob role. However,
in view of other work that's going on, I think we shouldn't design the
system on the assumption that it'll always be that way. As soon as
there exist roles that can create roles but cannot make arbitrary
privilege grants, this becomes an interesting security question.
Do you really think that's never going to happen?

My concern here is basically that the semantics of CINE --- ie, that
you don't really know the initial properties of the target object ---
seem far more dangerous for a role than for any other sort of object.
The possibility of unexpected grants on or to that role means
that you may be giving away privileges unintentionally.

The reason this thread was started is that it's a pretty clearly useful
thing to be able to use IF NOT EXISTS for CREATE ROLE and I don't agree
with the justification that we shouldn't allow it because someone might
use it carelessly.

I'm not buying the argument that it's a "clearly useful thing".
I think it's a foot-gun, and I repeat the point that nobody's
actually provided a concrete use-case.

regards, tom lane

#10

mark.dilger@enterprisedb.com

about 4 years ago

In reply to: Stephen Frost (#8)

Re: CREATE ROLE IF NOT EXISTS

On Nov 8, 2021, at 10:38 AM, Stephen Frost <sfrost@snowman.net> wrote:

I don't quite follow this. The entire point of Alice writing a script
that uses IF NOT EXISTS is to have that command not fail if, indeed,
that role already exists, but for the rest of the script to be run.
That there's some potential attacker with CREATEROLE running around
creating roles that they think someone *else* might create is really
stretching things to a very questionable level- especially with
CREATEROLE where Charlie could just CREATE a new role which is a member
of Bob anyway after the fact and then GRANT that role to themselves.

I don't see why this is "stretching things to a very questionable level". It might help this discussion if you could provide pseudo-code or similar for adding roles which is well-written and secure, and which benefits from this syntax. I would expect the amount of locking and checking for pre-existing roles that such logic would require would make the IF NOT EXIST option useless. Perhaps I'm wrong?

—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

#11

david.christensen@crunchydata.com

about 4 years ago

In reply to: Mark Dilger (#10)

Re: CREATE ROLE IF NOT EXISTS

On Mon, Nov 8, 2021 at 1:22 PM Mark Dilger <mark.dilger@enterprisedb.com>
wrote:

On Nov 8, 2021, at 10:38 AM, Stephen Frost <sfrost@snowman.net> wrote:

I don't quite follow this. The entire point of Alice writing a script
that uses IF NOT EXISTS is to have that command not fail if, indeed,
that role already exists, but for the rest of the script to be run.
That there's some potential attacker with CREATEROLE running around
creating roles that they think someone *else* might create is really
stretching things to a very questionable level- especially with
CREATEROLE where Charlie could just CREATE a new role which is a member
of Bob anyway after the fact and then GRANT that role to themselves.

I don't see why this is "stretching things to a very questionable level".
It might help this discussion if you could provide pseudo-code or similar
for adding roles which is well-written and secure, and which benefits from
this syntax. I would expect the amount of locking and checking for
pre-existing roles that such logic would require would make the IF NOT
EXIST option useless. Perhaps I'm wrong?

The main motivator for me writing this was trying to increase idempotency
for things like scripting, where you want to be able to minimize the effort
required to get things into a particular state. I agree with Stephen that
whether or not this is a best practices approach, this is something that is
being done in the wild via DO blocks or similar, so providing a tool to
handle this better seems useful on its own.

This originally came from me looking into the failures to load certain
`pg_dump` or `pg_dumpall` output when generated with the `--clean` flag,
which necessarily cannot work, as it fails with the error `current user
cannot be dropped`. Not that I am promoting the use of `pg_dumpall
--clean`, as there are clearly better solutions here, but something which
generates unusable output does not seem that useful. Instead, you could
generate `CREATE ROLE IF NOT EXISTS username` statements and emit `ALTER
ROLE ...`, which is what it is already doing (modulo `IF NOT EXISTS`).

This seems to introduce no further security vectors compared to field work
and increases utility in some cases, so seems generally useful to me.

If CINE semantics are at issue, what about the CREATE OR REPLACE semantics
with some sort of merge into the existing role? I don't care strongly
about which approach is taken, just think the overall "make this role exist
in this form" without an error is useful in my own work, and CINE was
easier to implement as a first pass.

Best,

David

#12

mark.dilger@enterprisedb.com

about 4 years ago

In reply to: David Christensen (#11)

Re: CREATE ROLE IF NOT EXISTS

On Nov 9, 2021, at 7:36 AM, David Christensen <david.christensen@crunchydata.com> wrote:

If CINE semantics are at issue, what about the CREATE OR REPLACE semantics with some sort of merge into the existing role? I don't care strongly about which approach is taken, just think the overall "make this role exist in this form" without an error is useful in my own work, and CINE was easier to implement as a first pass.

CREATE OR REPLACE might be a better option, not with the "merge into the existing role" part, but rather as drop+create. If a malicious actor has already added other roles to the role, or created a table with a malicious trigger definition, the drop part will fail, which is good from a security viewpoint. Of course, the drop portion will also fail under other conditions which don't entail any security concerns, but maybe they could be addressed in a series of follow-on patches?

I understand this idea is not as useful for creating idempotent scripts, but maybe it gets you part of the way there?

—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

#13

sfrost@snowman.net

about 4 years ago

In reply to: David Christensen (#11)

Re: CREATE ROLE IF NOT EXISTS

Greetings,

* David Christensen (david.christensen@crunchydata.com) wrote:

On Mon, Nov 8, 2021 at 1:22 PM Mark Dilger <mark.dilger@enterprisedb.com>
wrote:

On Nov 8, 2021, at 10:38 AM, Stephen Frost <sfrost@snowman.net> wrote:

I don't quite follow this. The entire point of Alice writing a script
that uses IF NOT EXISTS is to have that command not fail if, indeed,
that role already exists, but for the rest of the script to be run.
That there's some potential attacker with CREATEROLE running around
creating roles that they think someone *else* might create is really
stretching things to a very questionable level- especially with
CREATEROLE where Charlie could just CREATE a new role which is a member
of Bob anyway after the fact and then GRANT that role to themselves.

I don't see why this is "stretching things to a very questionable level".
It might help this discussion if you could provide pseudo-code or similar
for adding roles which is well-written and secure, and which benefits from
this syntax. I would expect the amount of locking and checking for
pre-existing roles that such logic would require would make the IF NOT
EXIST option useless. Perhaps I'm wrong?

The main motivator for me writing this was trying to increase idempotency
for things like scripting, where you want to be able to minimize the effort
required to get things into a particular state. I agree with Stephen that
whether or not this is a best practices approach, this is something that is
being done in the wild via DO blocks or similar, so providing a tool to
handle this better seems useful on its own.

Agreed.

This originally came from me looking into the failures to load certain
`pg_dump` or `pg_dumpall` output when generated with the `--clean` flag,
which necessarily cannot work, as it fails with the error `current user
cannot be dropped`. Not that I am promoting the use of `pg_dumpall
--clean`, as there are clearly better solutions here, but something which
generates unusable output does not seem that useful. Instead, you could
generate `CREATE ROLE IF NOT EXISTS username` statements and emit `ALTER
ROLE ...`, which is what it is already doing (modulo `IF NOT EXISTS`).

The other very common case that I've seen is where the role ends up
owning objects and therefore can't be dropped without also dropping
those objects- possibly just GRANTs but may also be tables or other
things. In other words, a script like this:

DROP ROLE IF EXISTS r1;
CREATE ROLE r1;
CREATE SCHEMA IF NOT EXISTS r1 AUTHORIZATION r1;

isn't able to be re-run, while this is able to be:

CREATE ROLE IF NOT EXISTS r1;
CREATE SCHEMA IF NOT EXISTS r1 AUTHORIZATION r1;

This seems to introduce no further security vectors compared to field work
and increases utility in some cases, so seems generally useful to me.

Yeah.

If CINE semantics are at issue, what about the CREATE OR REPLACE semantics
with some sort of merge into the existing role? I don't care strongly
about which approach is taken, just think the overall "make this role exist
in this form" without an error is useful in my own work, and CINE was
easier to implement as a first pass.

I don't really see how we could do CREATE OR REPLACE here, at least for
the cases that I'm thinking about. How would that work with existing
GRANTs, for example? Perhaps it'd be alright if we limited it to just
what can be specified in the CREATE ROLE and then left anything else in
place. I do generally like the idea of being able to explicitly say how
the role should look in one shot.

Thanks,

Stephen

#14

sfrost@snowman.net

about 4 years ago

In reply to: Mark Dilger (#12)

Re: CREATE ROLE IF NOT EXISTS

Greetings,

* Mark Dilger (mark.dilger@enterprisedb.com) wrote:

On Nov 9, 2021, at 7:36 AM, David Christensen <david.christensen@crunchydata.com> wrote:
If CINE semantics are at issue, what about the CREATE OR REPLACE semantics with some sort of merge into the existing role? I don't care strongly about which approach is taken, just think the overall "make this role exist in this form" without an error is useful in my own work, and CINE was easier to implement as a first pass.

CREATE OR REPLACE might be a better option, not with the "merge into the existing role" part, but rather as drop+create. If a malicious actor has already added other roles to the role, or created a table with a malicious trigger definition, the drop part will fail, which is good from a security viewpoint. Of course, the drop portion will also fail under other conditions which don't entail any security concerns, but maybe they could be addressed in a series of follow-on patches?

I understand this idea is not as useful for creating idempotent scripts, but maybe it gets you part of the way there?

If it's actually drop+create then, no, that isn't really useful because
it'll fail when that role owns objects (see my other email). If we can
avoid that issue then CREATE OR REPLACE might work, we just need to make
sure that we document what is, and isn't, done in such a case.

Thanks,

Stephen

#15

david.christensen@crunchydata.com

about 4 years ago

In reply to: Mark Dilger (#12)

Re: CREATE ROLE IF NOT EXISTS

On Tue, Nov 9, 2021 at 9:55 AM Mark Dilger <mark.dilger@enterprisedb.com>
wrote:

On Nov 9, 2021, at 7:36 AM, David Christensen <

david.christensen@crunchydata.com> wrote:

If CINE semantics are at issue, what about the CREATE OR REPLACE

semantics with some sort of merge into the existing role? I don't care
strongly about which approach is taken, just think the overall "make this
role exist in this form" without an error is useful in my own work, and
CINE was easier to implement as a first pass.

CREATE OR REPLACE might be a better option, not with the "merge into the
existing role" part, but rather as drop+create. If a malicious actor has
already added other roles to the role, or created a table with a malicious
trigger definition, the drop part will fail, which is good from a security
viewpoint. Of course, the drop portion will also fail under other
conditions which don't entail any security concerns, but maybe they could
be addressed in a series of follow-on patches?

I understand this idea is not as useful for creating idempotent scripts,
but maybe it gets you part of the way there?

Well, the CREATE OR REPLACE via just setting the role's attributes
explicitly based on what you passed it could work (not strictly DROP +
CREATE, in that you're keeping existing ownerships, etc, and can avoid
cross-db permissions/ownership checks). Seems like some sort of merge
logic could be in order, as you wouldn't really want to lose existing
permissions granted to a role, but you want to ensure that /at least/ the
permissions granted exist for this role.

David

#16

sfrost@snowman.net

about 4 years ago

In reply to: David Christensen (#15)

Re: CREATE ROLE IF NOT EXISTS

Greetings,

* David Christensen (david.christensen@crunchydata.com) wrote:

On Tue, Nov 9, 2021 at 9:55 AM Mark Dilger <mark.dilger@enterprisedb.com> wrote:

On Nov 9, 2021, at 7:36 AM, David Christensen <

david.christensen@crunchydata.com> wrote:

If CINE semantics are at issue, what about the CREATE OR REPLACE

semantics with some sort of merge into the existing role? I don't care
strongly about which approach is taken, just think the overall "make this
role exist in this form" without an error is useful in my own work, and
CINE was easier to implement as a first pass.

CREATE OR REPLACE might be a better option, not with the "merge into the
existing role" part, but rather as drop+create. If a malicious actor has
already added other roles to the role, or created a table with a malicious
trigger definition, the drop part will fail, which is good from a security
viewpoint. Of course, the drop portion will also fail under other
conditions which don't entail any security concerns, but maybe they could
be addressed in a series of follow-on patches?

I understand this idea is not as useful for creating idempotent scripts,
but maybe it gets you part of the way there?

Well, the CREATE OR REPLACE via just setting the role's attributes
explicitly based on what you passed it could work (not strictly DROP +
CREATE, in that you're keeping existing ownerships, etc, and can avoid
cross-db permissions/ownership checks). Seems like some sort of merge
logic could be in order, as you wouldn't really want to lose existing
permissions granted to a role, but you want to ensure that /at least/ the
permissions granted exist for this role.

What happens with role attributes that aren't explicitly mentioned
though? Do those get reset to 'default' or are they left as-is?

I suspect that most implementations will end up just explicitly setting
all of the role attributes, of course, because they want the role to
look like how it is defined to in whatever manifest is declaring the
role, but we should still think about how we want this to work if we're
going in this direction.

In terms of least-surprise, I do tend to think that the answer is "only
care about what is explicitly put into the command"- that is, if it
isn't in the CREATE ROLE statement then it gets left as-is. Not sure
how others feel about that though.

Thanks,

Stephen

#17

david.christensen@crunchydata.com

about 4 years ago

In reply to: Stephen Frost (#16)

Re: CREATE ROLE IF NOT EXISTS

On Tue, Nov 9, 2021 at 10:22 AM Stephen Frost <sfrost@snowman.net> wrote:

Greetings,

* David Christensen (david.christensen@crunchydata.com) wrote:

Well, the CREATE OR REPLACE via just setting the role's attributes
explicitly based on what you passed it could work (not strictly DROP +
CREATE, in that you're keeping existing ownerships, etc, and can avoid
cross-db permissions/ownership checks). Seems like some sort of merge
logic could be in order, as you wouldn't really want to lose existing
permissions granted to a role, but you want to ensure that /at least/ the
permissions granted exist for this role.

What happens with role attributes that aren't explicitly mentioned
though? Do those get reset to 'default' or are they left as-is?

Since we have the ability to specify explicit negative options
(NOCREATEDB vs CREATEDB, etc), I'd say leave as-is if not specified,
otherwise ensure it matches what you included in the command. Would also
ensure forward compatibility if new permissions/attributes were introduced,
as we don't want to explicitly require that all permissions be itemized to
utilize.

I suspect that most implementations will end up just explicitly setting
all of the role attributes, of course, because they want the role to
look like how it is defined to in whatever manifest is declaring the
role, but we should still think about how we want this to work if we're
going in this direction.

Agreed.

In terms of least-surprise, I do tend to think that the answer is "only
care about what is explicitly put into the command"- that is, if it
isn't in the CREATE ROLE statement then it gets left as-is. Not sure
how others feel about that though.

This is also what would make the most sense to me.

David

#18

mark.dilger@enterprisedb.com

about 4 years ago

In reply to: Stephen Frost (#16)

Re: CREATE ROLE IF NOT EXISTS

On Nov 9, 2021, at 8:22 AM, Stephen Frost <sfrost@snowman.net> wrote:

In terms of least-surprise, I do tend to think that the answer is "only
care about what is explicitly put into the command"- that is, if it
isn't in the CREATE ROLE statement then it gets left as-is. Not sure
how others feel about that though.

bob: CREATE ROLE charlie;
bob: GRANT charlie TO david;

super_alice: CREATE OR REPLACE ROLE charlie SUPERUSER;

I think this is the sort of thing Tom and I are worried about. "david" is now a member of a superuser role, and it is far from clear that "super_alice" intended that. Even if "bob" is not malicious, having this happen by accident is pretty bad.

If we fix the existing bug that the pg_auth_members.grantor field can end up as a dangling reference, instead making sure that it is always accurate, then perhaps this would be ok if all roles granted into "charlie" had grantor="super_alice". I'm not sure that is really good enough, but it is a lot closer to making this safe than allowing the command to succeed when role "charlie" has been granted away by someone else.

—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

#19

sfrost@snowman.net

about 4 years ago

In reply to: Mark Dilger (#18)

Re: CREATE ROLE IF NOT EXISTS

Greetings,

* Mark Dilger (mark.dilger@enterprisedb.com) wrote:

On Nov 9, 2021, at 8:22 AM, Stephen Frost <sfrost@snowman.net> wrote:
In terms of least-surprise, I do tend to think that the answer is "only
care about what is explicitly put into the command"- that is, if it
isn't in the CREATE ROLE statement then it gets left as-is. Not sure
how others feel about that though.

bob: CREATE ROLE charlie;
bob: GRANT charlie TO david;

super_alice: CREATE OR REPLACE ROLE charlie SUPERUSER;

I think this is the sort of thing Tom and I are worried about. "david" is now a member of a superuser role, and it is far from clear that "super_alice" intended that. Even if "bob" is not malicious, having this happen by accident is pretty bad.

I understand the concern that you and Tom have raised, I just don't see
it as such an issue that we can't give users this option. They're
already doing it via DO blocks and that's surely not any better.
Documenting that you should care about who is able to create roles in
your system when thinking about this is certainly reasonable, but just
saying we won't add it because someone might somewhere mis-use it isn't.

If we fix the existing bug that the pg_auth_members.grantor field can end up as a dangling reference, instead making sure that it is always accurate, then perhaps this would be ok if all roles granted into "charlie" had grantor="super_alice". I'm not sure that is really good enough, but it is a lot closer to making this safe than allowing the command to succeed when role "charlie" has been granted away by someone else.

I agree we should fix the issue of the grantor field being a dangling
reference, that's clearly not a good thing.

I'm not sure what is meant by making sure they're always 'accurate' or
why 'accurate' in this case means that the grantor is always
'super_alice'..? Are you suggesting that the CREATE OR REPLACE ROLE run
by super_alice would remove the GRANT that bob made of granting charlie
to david? I would argue that it's entirely possible that super_alice
knows exactly what is going on and intends for charlie to have superuser
access and understands that any role which charlie has been GRANT'd to
would therefore be able to become charlie, that's not a surprise.

Now, bringing this around to the more general discussion about making it
possible for folks who aren't superuser to be able to create roles, I
think there's another way to address this that might satisfy everyone,
particularly with the CREATE OR REPLACE approach- to wit: if the role
create isn't one that you've got appropriate rights on, then you
shouldn't be able to CREATE OR REPLACE it. This, perhaps, gets to a
distinction between having ADMIN rights on a role vs. the ability to
redefine the role (perhaps by virtue of being the 'owner' of that role)
that's useful.

In other words, in the case outlined above:

bob: CREATE ROLE charlie;
-- charlie is now a role 'owned' by bob and that isn't able to be
-- changed by bob to be some other owner, unless bob can become the
-- role to which they want to change ownership to
bob: GRANT charlie TO david;

alice: CREATE OR REPLACE ROLE charlie;
-- This now fails because while alice is able to create roles, alice
-- can only 'replace' roles which alice owns.

I appreciate that the case of 'super_alice' doing things where they're
an actual superuser might still be an issue, but running around doing
things with superuser is already risky business and the point here is to
get away from doing that by splitting superuser up, ideally in a way
that privileges can be given out to non-superusers in a manner that's
safer than doing things as a superuser and where independent
non-superusers aren't able to do bad things to each other.

Thanks,

Stephen

#20

mark.dilger@enterprisedb.com

about 4 years ago

In reply to: Stephen Frost (#19)

Re: CREATE ROLE IF NOT EXISTS

On Nov 9, 2021, at 8:50 AM, Stephen Frost <sfrost@snowman.net> wrote:

If we fix the existing bug that the pg_auth_members.grantor field can end up as a dangling reference, instead making sure that it is always accurate, then perhaps this would be ok if all roles granted into "charlie" had grantor="super_alice". I'm not sure that is really good enough, but it is a lot closer to making this safe than allowing the command to succeed when role "charlie" has been granted away by someone else.

I agree we should fix the issue of the grantor field being a dangling
reference, that's clearly not a good thing.

Just FYI, I have a patch underway to fix it. I'm not super close to posting it, though.

I'm not sure what is meant by making sure they're always 'accurate' or
why 'accurate' in this case means that the grantor is always
'super_alice'..?

I mean that the dangling reference could point at a role that no longer exists, but if the oid gets recycled, it could point at the *wrong* role rather than merely at no role. So we'd need that fixed before we could rely on the "grantor" field for anything. I think Robert mentioned this issue already, on another thread.

Are you suggesting that the CREATE OR REPLACE ROLE run
by super_alice would remove the GRANT that bob made of granting charlie
to david?

Suppose user "stephen.frost" owns a database and runs a script which creates roles, schemas, etc:

CREATE OR REPLACE ROLE super_alice SUPERUSER;
SET SESSION AUTHORIZATION super_alice;
CREATE OR REPLACE ROLE charlie;
CREATE OR REPLACE ROLE david IN ROLE charlie;

User "stephen.frost" runs that script again. The system cannot tell, as things currently are implemented, that "stephen.frost" was the original creator of role "super_alice", nor that "super_alice" was the original creator of "charlie" and "david".

The "grantor" field for "david"'s membership in "charlie" points at "super_alice", so we know enough to allow the "IN ROLE charlie" part, at least if we fix the dangling reference bug.

If we add an "owner" (or perhaps a "creator") field to pg_authid, the first time the script runs, it could be set to "stephen.frost" for "super_alice" and to "super_alice" for "charlie" and "david". When the script gets re-run, the CREATE OR REPLACE commands can succeed because that field matches.

I would argue that it's entirely possible that super_alice
knows exactly what is going on and intends for charlie to have superuser
access and understands that any role which charlie has been GRANT'd to
would therefore be able to become charlie, that's not a surprise.

I agree that super_alice might know that, but perhaps we can make this feature less of a foot-gun and still achieve the goal of making idempotent role creation scripts work?

Now, bringing this around to the more general discussion about making it
possible for folks who aren't superuser to be able to create roles, I
think there's another way to address this that might satisfy everyone,
particularly with the CREATE OR REPLACE approach- to wit: if the role
create isn't one that you've got appropriate rights on, then you
shouldn't be able to CREATE OR REPLACE it.

Agreed. You shouldn't be able to CREATE OR REPLACE a role that you couldn't CREATE in the first case.

This, perhaps, gets to a
distinction between having ADMIN rights on a role vs. the ability to
redefine the role (perhaps by virtue of being the 'owner' of that role)
that's useful.

In other words, in the case outlined above:

bob: CREATE ROLE charlie;
-- charlie is now a role 'owned' by bob and that isn't able to be
-- changed by bob to be some other owner, unless bob can become the
-- role to which they want to change ownership to
bob: GRANT charlie TO david;

alice: CREATE OR REPLACE ROLE charlie;
-- This now fails because while alice is able to create roles, alice
-- can only 'replace' roles which alice owns.

This sounds reasonable. It means, of course, implementing a role ownership system. I thought you had other concerns about doing so.

I appreciate that the case of 'super_alice' doing things where they're
an actual superuser might still be an issue, but running around doing
things with superuser is already risky business and the point here is to
get away from doing that by splitting superuser up, ideally in a way
that privileges can be given out to non-superusers in a manner that's
safer than doing things as a superuser and where independent
non-superusers aren't able to do bad things to each other.

I'm not sure what to do about this.

—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

#21

david.christensen@crunchydata.com

about 4 years ago

In reply to: Mark Dilger (#20)

1 attachment(s)

Re: CREATE ROLE IF NOT EXISTS

Modulo other issues/discussions, here is a version of this patch that
implements CREATE OR REPLACE ROLE just by handing off to AlterRole if it's
determined that the role already exists; presumably any/all additional
considerations would need to be added in both places were there a separate
code path for this.

It might be worth refactoring the AlterRole into a helper if there are any
deviations in messages, etc, but could be a decent approach to handling the
problem (which arguably would have similar restrictions/requirements in
ALTER ROLE itself) in a single location.

Best,

David

Attachments:

CREATE-OR-REPLACE-ROLE.patchapplication/octet-stream; name=CREATE-OR-REPLACE-ROLE.patchDownload

From 04c747f8ceb3417b400ea66cce46e7fa4c294b45 Mon Sep 17 00:00:00 2001
From: David Christensen <david.christensen@crunchydata.com>
Date: Wed, 10 Nov 2021 09:30:51 -0600
Subject: [PATCH] Support CREATE OR REPLACE for roles, users, and groups

---
 doc/src/sgml/ref/create_group.sgml           |  2 +-
 doc/src/sgml/ref/create_role.sgml            | 14 +++++-
 doc/src/sgml/ref/create_user.sgml            |  2 +-
 src/backend/commands/user.c                  | 26 +++++++++--
 src/backend/nodes/copyfuncs.c                |  1 +
 src/backend/nodes/equalfuncs.c               |  1 +
 src/backend/parser/gram.y                    | 30 ++++++++++++
 src/include/nodes/parsenodes.h               |  1 +
 src/test/regress/expected/roleattributes.out | 48 ++++++++++++++++++++
 src/test/regress/sql/roleattributes.sql      | 28 ++++++++++++
 10 files changed, 145 insertions(+), 8 deletions(-)

diff --git a/doc/src/sgml/ref/create_group.sgml b/doc/src/sgml/ref/create_group.sgml
index d124c98eb5..da4061093f 100644
--- a/doc/src/sgml/ref/create_group.sgml
+++ b/doc/src/sgml/ref/create_group.sgml
@@ -21,7 +21,7 @@ PostgreSQL documentation
 
  <refsynopsisdiv>
 <synopsis>
-CREATE GROUP <replaceable class="parameter">name</replaceable> [ [ WITH ] <replaceable class="parameter">option</replaceable> [ ... ] ]
+CREATE [OR REPLACE] GROUP <replaceable class="parameter">name</replaceable> [ [ WITH ] <replaceable class="parameter">option</replaceable> [ ... ] ]
 
 <phrase>where <replaceable class="parameter">option</replaceable> can be:</phrase>
 
diff --git a/doc/src/sgml/ref/create_role.sgml b/doc/src/sgml/ref/create_role.sgml
index b6a4ea1f72..d9d5443ac8 100644
--- a/doc/src/sgml/ref/create_role.sgml
+++ b/doc/src/sgml/ref/create_role.sgml
@@ -21,7 +21,7 @@ PostgreSQL documentation
 
  <refsynopsisdiv>
 <synopsis>
-CREATE ROLE <replaceable class="parameter">name</replaceable> [ [ WITH ] <replaceable class="parameter">option</replaceable> [ ... ] ]
+CREATE [ OR REPLACE ] ROLE <replaceable class="parameter">name</replaceable> [ [ WITH ] <replaceable class="parameter">option</replaceable> [ ... ] ]
 
 <phrase>where <replaceable class="parameter">option</replaceable> can be:</phrase>
 
@@ -74,6 +74,18 @@ in sync when changing the above synopsis!
   <title>Parameters</title>
 
     <variablelist>
+     <varlistentry>
+      <term><literal>OR REPLACE</literal></term>
+      <listitem>
+       <para>
+        Ensure that the given role exists with the parameters provided on in
+        the command.  If the role already exists, this will force any
+        parameters on this role into the existing role.  This may end up with
+        some surprising behavior.
+       </para>
+      </listitem>
+     </varlistentry>
+
      <varlistentry>
       <term><replaceable class="parameter">name</replaceable></term>
       <listitem>
diff --git a/doc/src/sgml/ref/create_user.sgml b/doc/src/sgml/ref/create_user.sgml
index 48d2089238..e130a40cb9 100644
--- a/doc/src/sgml/ref/create_user.sgml
+++ b/doc/src/sgml/ref/create_user.sgml
@@ -21,7 +21,7 @@ PostgreSQL documentation
 
  <refsynopsisdiv>
 <synopsis>
-CREATE USER <replaceable class="parameter">name</replaceable> [ [ WITH ] <replaceable class="parameter">option</replaceable> [ ... ] ]
+CREATE [ OR REPLACE ] USER <replaceable class="parameter">name</replaceable> [ [ WITH ] <replaceable class="parameter">option</replaceable> [ ... ] ]
 
 <phrase>where <replaceable class="parameter">option</replaceable> can be:</phrase>
 
diff --git a/src/backend/commands/user.c b/src/backend/commands/user.c
index aa69821be4..6fad8575bc 100644
--- a/src/backend/commands/user.c
+++ b/src/backend/commands/user.c
@@ -304,11 +304,27 @@ CreateRole(ParseState *pstate, CreateRoleStmt *stmt)
 	pg_authid_rel = table_open(AuthIdRelationId, RowExclusiveLock);
 	pg_authid_dsc = RelationGetDescr(pg_authid_rel);
 
-	if (OidIsValid(get_role_oid(stmt->role, true)))
-		ereport(ERROR,
-				(errcode(ERRCODE_DUPLICATE_OBJECT),
-				 errmsg("role \"%s\" already exists",
-						stmt->role)));
+	if (OidIsValid(get_role_oid(stmt->role, true))) {
+		if (stmt->replace) {
+			AlterRoleStmt *alter = makeNode(AlterRoleStmt);
+
+			alter->role = makeNode(RoleSpec);
+			alter->role->roletype = ROLESPEC_CSTRING;
+			alter->role->rolename = stmt->role;
+			alter->role->location = -1;
+			alter->options = stmt->options;
+			alter->action = 1;
+
+			table_close(pg_authid_rel, NoLock);
+
+			return AlterRole(pstate, alter);
+		}
+		else
+			ereport(ERROR,
+					(errcode(ERRCODE_DUPLICATE_OBJECT),
+					 errmsg("role \"%s\" already exists",
+							stmt->role)));
+	}
 
 	/* Convert validuntil to internal form */
 	if (validUntil)
diff --git a/src/backend/nodes/copyfuncs.c b/src/backend/nodes/copyfuncs.c
index ad1ea2ff2f..e1c4f90276 100644
--- a/src/backend/nodes/copyfuncs.c
+++ b/src/backend/nodes/copyfuncs.c
@@ -4516,6 +4516,7 @@ _copyCreateRoleStmt(const CreateRoleStmt *from)
 	COPY_SCALAR_FIELD(stmt_type);
 	COPY_STRING_FIELD(role);
 	COPY_NODE_FIELD(options);
+	COPY_SCALAR_FIELD(replace);
 
 	return newnode;
 }
diff --git a/src/backend/nodes/equalfuncs.c b/src/backend/nodes/equalfuncs.c
index f537d3eb96..e6224156af 100644
--- a/src/backend/nodes/equalfuncs.c
+++ b/src/backend/nodes/equalfuncs.c
@@ -2129,6 +2129,7 @@ _equalCreateRoleStmt(const CreateRoleStmt *a, const CreateRoleStmt *b)
 	COMPARE_SCALAR_FIELD(stmt_type);
 	COMPARE_STRING_FIELD(role);
 	COMPARE_NODE_FIELD(options);
+	COMPARE_SCALAR_FIELD(replace);
 
 	return true;
 }
diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y
index a6d0cefa6b..42bceaf6f6 100644
--- a/src/backend/parser/gram.y
+++ b/src/backend/parser/gram.y
@@ -1061,6 +1061,16 @@ CreateRoleStmt:
 					n->stmt_type = ROLESTMT_ROLE;
 					n->role = $3;
 					n->options = $5;
+					n->if_not_exists = false;
+					$$ = (Node *)n;
+				}
+			| CREATE OR REPLACE ROLE RoleId opt_with OptRoleList
+				{
+					CreateRoleStmt *n = makeNode(CreateRoleStmt);
+					n->stmt_type = ROLESTMT_ROLE;
+					n->role = $5;
+					n->options = $7;
+					n->if_not_exists = true;
 					$$ = (Node *)n;
 				}
 		;
@@ -1217,6 +1227,16 @@ CreateUserStmt:
 					n->stmt_type = ROLESTMT_USER;
 					n->role = $3;
 					n->options = $5;
+					n->if_not_exists = false;
+					$$ = (Node *)n;
+				}
+			| CREATE OR REPLACE USER RoleId opt_with OptRoleList
+				{
+					CreateRoleStmt *n = makeNode(CreateRoleStmt);
+					n->stmt_type = ROLESTMT_USER;
+					n->role = $5;
+					n->options = $7;
+					n->if_not_exists = true;
 					$$ = (Node *)n;
 				}
 		;
@@ -1356,6 +1376,16 @@ CreateGroupStmt:
 					n->stmt_type = ROLESTMT_GROUP;
 					n->role = $3;
 					n->options = $5;
+					n->if_not_exists = false;
+					$$ = (Node *)n;
+				}
+			| CREATE OR REPLACE GROUP_P RoleId opt_with OptRoleList
+				{
+					CreateRoleStmt *n = makeNode(CreateRoleStmt);
+					n->stmt_type = ROLESTMT_GROUP;
+					n->role = $5;
+					n->options = $7;
+					n->if_not_exists = true;
 					$$ = (Node *)n;
 				}
 		;
diff --git a/src/include/nodes/parsenodes.h b/src/include/nodes/parsenodes.h
index 067138e6b5..b953d092ae 100644
--- a/src/include/nodes/parsenodes.h
+++ b/src/include/nodes/parsenodes.h
@@ -2622,6 +2622,7 @@ typedef struct CreateRoleStmt
 	RoleStmtType stmt_type;		/* ROLE/USER/GROUP */
 	char	   *role;			/* role name */
 	List	   *options;		/* List of DefElem nodes */
+	bool       replace;			/* Merge new definition into existing role */
 } CreateRoleStmt;
 
 typedef struct AlterRoleStmt
diff --git a/src/test/regress/expected/roleattributes.out b/src/test/regress/expected/roleattributes.out
index 5e6969b173..a81c999062 100644
--- a/src/test/regress/expected/roleattributes.out
+++ b/src/test/regress/expected/roleattributes.out
@@ -247,3 +247,51 @@ DROP ROLE regress_test_def_replication;
 DROP ROLE regress_test_replication;
 DROP ROLE regress_test_def_bypassrls;
 DROP ROLE regress_test_bypassrls;
+-- CREATE OR REPLACE ROLE
+CREATE ROLE regress_test_exists_role;
+CREATE ROLE regress_test_exists_role;
+ERROR:  role "regress_test_exists_role" already exists
+CREATE OR REPLACE ROLE regress_test_exists_role;
+DROP ROLE regress_test_exists_role;
+CREATE ROLE regress_test_exists_role WITH NOINHERIT;
+CREATE OR REPLACE ROLE regress_test_exists_role WITH INHERIT;
+SELECT rolname, rolinherit FROM pg_authid WHERE rolname = 'regress_test_exists_role';
+         rolname          | rolinherit 
+--------------------------+------------
+ regress_test_exists_role | t
+(1 row)
+
+ALTER ROLE regress_test_exists_role WITH INHERIT;
+SELECT rolname, rolinherit FROM pg_authid WHERE rolname = 'regress_test_exists_role';
+         rolname          | rolinherit 
+--------------------------+------------
+ regress_test_exists_role | t
+(1 row)
+
+DROP ROLE regress_test_exists_role;
+CREATE USER regress_test_exists_user;
+CREATE USER regress_test_exists_user;
+ERROR:  role "regress_test_exists_user" already exists
+CREATE OR REPLACE USER regress_test_exists_user;
+DROP USER regress_test_exists_user;
+CREATE USER regress_test_exists_user WITH NOINHERIT;
+CREATE OR REPLACE USER regress_test_exists_user WITH INHERIT;
+SELECT rolname, rolinherit FROM pg_authid WHERE rolname = 'regress_test_exists_user';
+         rolname          | rolinherit 
+--------------------------+------------
+ regress_test_exists_user | t
+(1 row)
+
+ALTER USER regress_test_exists_user WITH INHERIT;
+SELECT rolname, rolinherit FROM pg_authid WHERE rolname = 'regress_test_exists_user';
+         rolname          | rolinherit 
+--------------------------+------------
+ regress_test_exists_user | t
+(1 row)
+
+DROP USER regress_test_exists_user;
+CREATE GROUP regress_test_exists_group;
+CREATE GROUP regress_test_exists_group;
+ERROR:  role "regress_test_exists_group" already exists
+CREATE OR REPLACE GROUP regress_test_exists_group;
+DROP GROUP regress_test_exists_group;
diff --git a/src/test/regress/sql/roleattributes.sql b/src/test/regress/sql/roleattributes.sql
index c961b2d730..185931f413 100644
--- a/src/test/regress/sql/roleattributes.sql
+++ b/src/test/regress/sql/roleattributes.sql
@@ -96,3 +96,31 @@ DROP ROLE regress_test_def_replication;
 DROP ROLE regress_test_replication;
 DROP ROLE regress_test_def_bypassrls;
 DROP ROLE regress_test_bypassrls;
+
+-- CREATE OR REPLACE ROLE
+CREATE ROLE regress_test_exists_role;
+CREATE ROLE regress_test_exists_role;
+CREATE OR REPLACE ROLE regress_test_exists_role;
+DROP ROLE regress_test_exists_role;
+CREATE ROLE regress_test_exists_role WITH NOINHERIT;
+CREATE OR REPLACE ROLE regress_test_exists_role WITH INHERIT;
+SELECT rolname, rolinherit FROM pg_authid WHERE rolname = 'regress_test_exists_role';
+ALTER ROLE regress_test_exists_role WITH INHERIT;
+SELECT rolname, rolinherit FROM pg_authid WHERE rolname = 'regress_test_exists_role';
+DROP ROLE regress_test_exists_role;
+
+CREATE USER regress_test_exists_user;
+CREATE USER regress_test_exists_user;
+CREATE OR REPLACE USER regress_test_exists_user;
+DROP USER regress_test_exists_user;
+CREATE USER regress_test_exists_user WITH NOINHERIT;
+CREATE OR REPLACE USER regress_test_exists_user WITH INHERIT;
+SELECT rolname, rolinherit FROM pg_authid WHERE rolname = 'regress_test_exists_user';
+ALTER USER regress_test_exists_user WITH INHERIT;
+SELECT rolname, rolinherit FROM pg_authid WHERE rolname = 'regress_test_exists_user';
+DROP USER regress_test_exists_user;
+
+CREATE GROUP regress_test_exists_group;
+CREATE GROUP regress_test_exists_group;
+CREATE OR REPLACE GROUP regress_test_exists_group;
+DROP GROUP regress_test_exists_group;
-- 
2.30.1 (Apple Git-130)

#22

Daniel Gustafsson

daniel@yesql.se

about 4 years ago

In reply to: David Christensen (#21)

Re: CREATE ROLE IF NOT EXISTS

On 10 Nov 2021, at 18:14, David Christensen <david.christensen@crunchydata.com> wrote:

Modulo other issues/discussions, here is a version of this patch..

This patch fails to compile since you renamed the if_not_exists member in
CreateRoleStmt but still set it in the parser.

--
Daniel Gustafsson https://vmware.com/

#23