sql: avoid role existence check in DROP ROLE when it's not necessary

[rafiss]

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

This check was recently added in 696869a.

informs: #134538
Release note: None

[blathers-crl]

It looks like your PR touches production code but doesn't add or edit any test code. Did you consider adding tests to your PR?

_{🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf.}

[cockroach-teamcity]

This change is 

[rafiss]

I cherry-picked this commit into these backports: #134967, #134968, #134969, #134970

[spilchen]

Good catch. I missed this in my review.

Reviewed 1 of 1 files at r1, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @Dedej-Bergin)

[Dedej-Bergin]

pkg/sql/drop_role.go line 113 at r1 (raw file):

				// 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)

Maybe we can combine these here?

			if roleExists, err := RoleExists(params.ctx, params.p.InternalSQLTxn(), name); err != nil {
				return err
			} else if !roleExists {
				// If the role does not exist, we can skip the check for targetIsAdmin.
				continue
			}

[Dedej-Bergin]

Reviewable status: complete! 2 of 0 LGTMs obtained (waiting on @rafiss)

[rafiss]

Reviewable status: complete! 2 of 0 LGTMs obtained (waiting on @Dedej-Bergin)

---

pkg/sql/drop_role.go line 113 at r1 (raw file):

Previously, Dedej-Bergin (Bergin Dedej) wrote…

Maybe we can combine these here?

			if roleExists, err := RoleExists(params.ctx, params.p.InternalSQLTxn(), name); err != nil {
				return err
			} else if !roleExists {
				// If the role does not exist, we can skip the check for targetIsAdmin.
				continue
			}

that is possible, but it does make the if statement a bit longer and hard to read. it also only saves 2 lines of code, so i don't feel that it's needed

[Dedej-Bergin]

pkg/sql/drop_role.go line 113 at r1 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

that is possible, but it does make the if statement a bit longer and hard to read. it also only saves 2 lines of code, so i don't feel that it's needed

What about this to reduce nesting?

		if hasAdmin {
			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
                        }
		}

		// Non-admin users cannot drop admins.
		targetIsAdmin, err := params.p.UserHasAdminRole(params.ctx, name)
		if err != nil {
			return err
		}
		if targetIsAdmin {
			return pgerror.New(pgcode.InsufficientPrivilege, "must be superuser to drop superusers")
		}

[rafiss]

tftr

bors r+

[craig]

Build succeeded:

• lint
• linux_amd64_fips_build
• macos_amd64_build
• check_generated_code
• linux_arm64_build
• linux_amd64_build
• windows_build
• docker_image_amd64
• macos_arm64_build
• acceptance
• examples_orms
• local_roachtest
• local_roachtest_fips
• unit_tests