From ec632a74c328cd2707b17c9bc79c6261e26410ff Mon Sep 17 00:00:00 2001
From: Bergin Dedej <bergin.dedej@cockroachlabs.com>
Date: Mon, 11 Nov 2024 18:12:08 +0000
Subject: [PATCH 1/2] sql: Drop Role does not properly check if target user
 does not exist

Previously when we are a non-admin user and we run drop role if exists
on a user that does not exist we would get an error that the user does
not exist.  This is incosistent with other if exists commands and it
does not make sense to get an error since by typing if exists we expect
that the user may not exist.  These code changes take care of that.

Fixes: #134538

Release note (bug fix): When you are a non-admin user and you run drop
role if exists on a user that does not exist you no longer get an error
message.
---
 pkg/sql/drop_role.go                          | 11 +++++++++
 .../drop_role_with_default_privileges         | 24 +++++++++++++++++++
 2 files changed, 35 insertions(+)

diff --git a/pkg/sql/drop_role.go b/pkg/sql/drop_role.go
index 2cedbda46537..2b446664c7ba 100644
--- a/pkg/sql/drop_role.go
+++ b/pkg/sql/drop_role.go
@@ -104,8 +104,18 @@ func (n *DropRoleNode) startExec(params runParams) error {
 		if name.IsReserved() {
 			return pgerror.Newf(pgcode.ReservedName, "role name %q is reserved", name.Normalized())
 		}
+
 		// Non-admin users cannot drop admins.
 		if !hasAdmin {
+			roleExists, err := RoleExists(params.ctx, params.p.InternalSQLTxn(), name)
+			if err != nil {
+				return err
+			}
+			if !roleExists {
+				// If the role does not exist, we can skip the check for targetIsAdmin.
+				continue
+			}
+
 			targetIsAdmin, err := params.p.UserHasAdminRole(params.ctx, name)
 			if err != nil {
 				return err
@@ -114,6 +124,7 @@ func (n *DropRoleNode) startExec(params runParams) error {
 				return pgerror.New(pgcode.InsufficientPrivilege, "must be superuser to drop superusers")
 			}
 		}
+
 	}
 
 	privilegeObjectFormatter := tree.NewFmtCtx(tree.FmtSimple)
diff --git a/pkg/sql/logictest/testdata/logic_test/drop_role_with_default_privileges b/pkg/sql/logictest/testdata/logic_test/drop_role_with_default_privileges
index d6f1d959e918..e884c5b9f1dc 100644
--- a/pkg/sql/logictest/testdata/logic_test/drop_role_with_default_privileges
+++ b/pkg/sql/logictest/testdata/logic_test/drop_role_with_default_privileges
@@ -159,3 +159,27 @@ ALTER DEFAULT PRIVILEGES FOR ALL ROLES IN SCHEMA public REVOKE ALL ON SEQUENCES
 
 statement ok
 DROP ROLE testuser4;
+
+subtest fix_for_regression_bug_134538
+
+statement ok
+CREATE USER not_admin WITH PASSWORD '123';
+GRANT SYSTEM CREATEROLE TO not_admin;
+SET ROLE not_admin;
+
+statement error pq: role/user "a_user_that_does_not_exist" does not exist
+DROP USER a_user_that_does_not_exist;
+
+statement ok
+DROP USER IF EXISTS a_user_that_does_not_exist;
+
+statement ok
+SET ROLE admin;
+
+statement error pq: role/user "a_user_that_does_not_exist" does not exist
+DROP USER a_user_that_does_not_exist;
+
+statement ok
+DROP USER IF EXISTS a_user_that_does_not_exist;
+
+subtest end

From 5133c9d0e9ac988c5984cfd84526df03fa51b943 Mon Sep 17 00:00:00 2001
From: Rafi Shamim <rafi@cockroachlabs.com>
Date: Tue, 12 Nov 2024 11:43:02 -0500
Subject: [PATCH 2/2] sql: avoid role existence check in DROP ROLE when it's
 not necessary

We only need to make the RoleExists call when the `IF EXISTS` clause is
used.

This check was recently added in 696869afeb369f3999384e2637610f5fee147362.

Release note: None
---
 pkg/sql/drop_role.go | 18 +++++++++++-------
 1 file changed, 11 insertions(+), 7 deletions(-)

diff --git a/pkg/sql/drop_role.go b/pkg/sql/drop_role.go
index 2b446664c7ba..b550dbbbb3a3 100644
--- a/pkg/sql/drop_role.go
+++ b/pkg/sql/drop_role.go
@@ -107,13 +107,17 @@ func (n *DropRoleNode) startExec(params runParams) error {
 
 		// Non-admin users cannot drop admins.
 		if !hasAdmin {
-			roleExists, err := RoleExists(params.ctx, params.p.InternalSQLTxn(), name)
-			if err != nil {
-				return err
-			}
-			if !roleExists {
-				// If the role does not exist, we can skip the check for targetIsAdmin.
-				continue
+			if n.ifExists {
+				// If `IF EXISTS` was specified, then a non-existing role should be
+				// skipped without causing any error.
+				roleExists, err := RoleExists(params.ctx, params.p.InternalSQLTxn(), name)
+				if err != nil {
+					return err
+				}
+				if !roleExists {
+					// If the role does not exist, we can skip the check for targetIsAdmin.
+					continue
+				}
 			}
 
 			targetIsAdmin, err := params.p.UserHasAdminRole(params.ctx, name)