Since I started at Stack Overflow, I’ve had the chance to see some of the interesting challenges with running our databases at our scale.
My coworker Taryn Pratt (blog|twitter) recently blogged about fighting with deadlocks related to Stack Overflow for Teams.
Lots of logins
Taryn mentions in her post that each team gets their own login, user, schema, etc. This means we have a lot of logins. Additionally, we use Availability Groups for our High Availability, which means we need to keep those logins in sync across AG Replicas.
We recently ran into some performance problems with our login sync, which is based on
sp_help_revlogin, the documented & recommended approach by Microsoft.
I’ve been installing & using these two procedures since I started working with SQL Server, back at the turn of the century. In the nearly two decades since, I’ve blindly installed & used these procedures, first on SQL Server 2000, and then on every version since… just because that’s the way I’ve always done it. But our recent performance problems made me rethink that, and dive in to take a look at the two procedures to see if I could do better, which made me realize, OHBOY! WE CAN DO BETTER!!
I started by looking at the code for
sp_hexadecimal, which is a pretty short stored procedure. After just a few minutes, I realized that it was simply converting the binary value to a hexadecimal string (which is how it’s normally displayed to us in SSMS anyway). It’s just doing a data type conversion. It’s just doing a
Since SQL Server 2008, SQL Server has allowed the conversion of
varchar() using the
CONVERT() function, and specifying a style–this follows the same pattern we are familiar with using to convert between
sp_hexadecimal converts the binary value to
varchar(514), so let’s try to use
CONVERT() to do the same thing.
The docs explain the differences in style, which tells me I want to use style 1. I confirmed this by running a quick query to check the output matched my understanding of the docs:
SELECT Sid_OrigBinary = sid, Sid_Style0 = CONVERT(varchar(514),sid,0), Sid_Style1 = CONVERT(varchar(514),sid,1), Sid_Style2 = CONVERT(varchar(514),sid,2) FROM sys.server_principals;
And because I don’t trust myself, I then did a head-to-head matchup to compare the results of
sp_hexadecimal, just in case there was a minor detail I was missing. Sure enough, they produce equivalent results.
DECLARE hex_cur CURSOR LOCAL FAST_FORWARD FOR SELECT Sid_OrigBinary = sid, Sid_Style1 = CONVERT(varchar(514),sid,1) FROM sys.server_principals; DECLARE @Sid_OrigBinary varbinary(85); DECLARE @Sid_Convert_Style1 varchar(514); DECLARE @Sid_sp_hexadecimal varchar(514); OPEN hex_cur; FETCH hex_cur INTO @Sid_OrigBinary, @Sid_Convert_Style1; WHILE (@@FETCH_STATUS = 0) BEGIN EXEC sp_hexadecimal @binvalue = @Sid_OrigBinary, @hexvalue = @Sid_sp_hexadecimal OUTPUT; IF (@Sid_Convert_Style1 = @Sid_sp_hexadecimal) BEGIN PRINT CONCAT('MATCHED!!! - ', @Sid_sp_hexadecimal, ' - ', @Sid_Convert_Style1); END ELSE BEGIN PRINT CONCAT('OH NO!!!! NOT MATCHED!!! - ', @Sid_sp_hexadecimal, ' - ', @Sid_Convert_Style1); END FETCH hex_cur INTO @Sid_OrigBinary, @Sid_Convert_Style1; END CLOSE hex_cur; DEALLOCATE hex_cur;
Stop using sp_hexadecimal!
Mind blown. 🤯 It has been 13 years since
sp_hexadecimal became obsolete, and it’s still being used heavily & continues to be recommended in the official Microsoft docs. Why continue to use this quasi-official procedure that you need to manually install when you’ve got a built-in function that does the same work for you?
- That’s the way I’ve always done it
sp_help_revloginuses it, so it’s easier
- It’s built-in to the T-SQL language
- It’s faster
- It can be used in set-based operations without looping
Other than inertia and the dependency from
sp_help_revlogin, there’s no upside to
sp_hexadecimal. Next step is to see if we need
sp_help_revlogin, or if we can get rid of that, too.
Let’s look at the code for
A big chunk of
sp_help_revlogin is actually just a cursor definition and the accompanying looping logic, all to accommodate the fact that
sp_hexadecimal is a procedure. This is the spot where it actually builds the output inisde that cursor, creating a string as
DECLARE @tmpstr varchar (1024) -- SET @tmpstr = 'CREATE LOGIN ' + QUOTENAME( @name ) + ' WITH PASSWORD = ' + @PWD_string + ' HASHED, SID = ' + @SID_string + ', DEFAULT_DATABASE = [' + @defaultdb + ']' IF ( @is_policy_checked IS NOT NULL ) BEGIN SET @tmpstr = @tmpstr + ', CHECK_POLICY = ' + @is_policy_checked END IF ( @is_expiration_checked IS NOT NULL ) BEGIN SET @tmpstr = @tmpstr + ', CHECK_EXPIRATION = ' + @is_expiration_checked END END IF (@denylogin = 1) BEGIN -- login is denied access SET @tmpstr = @tmpstr + '; DENY CONNECT SQL TO ' + QUOTENAME( @name ) END ELSE IF (@hasaccess = 0) BEGIN -- login exists but does not have access SET @tmpstr = @tmpstr + '; REVOKE CONNECT SQL TO ' + QUOTENAME( @name ) END IF (@is_disabled = 1) BEGIN -- login is disabled SET @tmpstr = @tmpstr + '; ALTER LOGIN ' + QUOTENAME( @name ) + ' DISABLE' END PRINT @tmpstr
When I looked at this with a critical eye, I came across a few problems with the code:
DECLARE @tmpstr varchar (1024)– The string is declared as
nvarchar. Login names & database names both allow unicode characters. If you have a login or default database that uses a unicode character, the output from
sp_help_revloginwill be invalid. This is one of those cases where it might not affect you now, but when the scenario does come up, it will break your code, and you’ll have to troubleshoot. Fix it now, and you don’t have to worry later.
DEFAULT_DATABASE = [' + @defaultdb + ']'– Manually quoting object names like this is an opportunity for SQL Injection vulnerabilities, and also for bugs. There’s probably a low likelihood of hitting problems here since it’s been this way for two decades, but possible. A database name with a
]character will break the script output. Someone could even do something malicious, like creating a database named
];GO; SHUTDOWN WITH NOWAIT;. Instead, the code should use
QUOTENAME(). My PR to fix this has been merged, which corrects the “official” version, but you may want to update your installed version, since this will not be pushed out in a CU.
sp_help_revloginincludes permissions – If the login is disabled, or has
CONNECT SQLpermission denied or revoked,
sp_help_revloginwill include that in the same output. I can appreciate from a security perspective that this is a good move to preserve these attributes, but it can get in the way, too. I’ve seen many cases where companies juggle permissions so that a login is disabled on Primary, but enabled on read-only Secondary. Having the
DENY CONNECT SQLin the output makes using sp_help_revlogin to copy logins between servers not possible in this scenario. Additionally, it ignores other server-level permissions like
VIEW SERVER STATE, etc. That means you’ll still need to sync server-level permissions separately.
- The output is just printed as text – This isn’t something new, but it’s certainly something that prevents using
sp_help_revloginin automation. You’d need to modify it to return a data set, which you could then do something with.
There’s so much going on here that by the time I remove the cursor, replace
CONVERT(), and take care of these other things, I might as well just start from scratch. So I did.
Stop using sp_help_revlogin!
Before you tell me to use PowerShell, let me come out and say that I know I can use PowerShell. There are still plenty of folks who prefer to do these things in T-SQL without the extra context switch, and that’s how we’re going to do it today.
I’ve added two new views to my DBA database, available on GitHub.
In dbo.ServerLogins, I recreate the logic of
sp_help_revlogin, and fix my concerns to make it unicode-friendly, set-based, and useful in automation. To get a list of recently-modified logins that can log in (have
CONNECT SQL permission & are enabled), along with the necessary
CREATE LOGIN statement, I’d simply do something like this. Note that the
ENABLE statements are in separate columns so they can be handled separately in automation:
SELECT LoginName, DateModified, CreateSql, EnableSql FROM DBA.dbo.ServerLogins WHERE CanLogIn = 1 AND DateModified >= DATEADD(DAY, -7, GETUTCDATE());
In the second view, dbo.ServerLoginPermissions, I provide all the
DENY statements for permissions. To get the ones appropriate for the
CONNECT SQL permissions you would get from
sp_help_revlogin, you can simply do this:
SELECT LoginName, PermissionType, PermissionSql FROM DBA.dbo.ServerLoginPermissions WHERE PermissionType = 'COSQ';
And obviously, I can combine the two views to get everything
sp_help_revlogin gave us, in a more usable format that supports unicode and solves my other complaints:
SELECT l.LoginName, l.DateModified, l.CreateSql, l.EnableSql, p.PermissionSql FROM DBA.dbo.ServerLogins AS l JOIN DBA.dbo.ServerLoginPermissions AS p ON l.LoginSid = p.LoginSid AND p.PermissionType = 'COSQ' WHERE l.CanLogIn = 1;
And as a bonus, we can get the server-level permissions that
SELECT l.LoginName, l.DateModified, l.CreateSql, l.EnableSql, PermissionsSql = STRING_AGG(p.PermissionSql, CHAR(10)) FROM DBA.dbo.ServerLogins AS l JOIN DBA.dbo.ServerLoginPermissions AS p ON l.LoginSid = p.LoginSid WHERE l.CanLogIn = 1 --AND l.DateModified >= DATEADD(DAY, -7, GETUTCDATE()) GROUP BY l.LoginName, l.DateModified, l.CreateSql, l.EnableSql;
Go grab the code
Head over to my GitHub repo, clone the repo, install my DBA database, and update your automation, scripts, and internal documentation to use this view. Then update your server build scripts/documentation so you can stop using
Hey that’s pretty cool.
It’s very similar to what I posted here but as views instead of procedure: