Possible change to remove sysadmin requirement

[KennethScott]

I've been playing around with a small change to remove the sysadmin requirement for the database user and wanted to ask for thoughts/feedback.

*This is in reference to issue #23 and the DBCC LOGINFO call that requires sysadmin.

I'm essentially just proposing an IF statement to check if the current user is sysadmin before attempting to populate the vlf temp table. The way the code is already structured, if no records are inserted into the temp table, the end result is 0's for the VLFCount for each database.

I realize it might not be immediately obvious that there's an issue, but it does seem better than the big ugly error that's thrown otherwise that probably turns some people off from using the application.

The change is the addition of the IF IS_SRVROLEMEMBER ('sysadmin') = 1 in the code below:

Create Table #VLFCounts (DatabaseId int, DatabaseName sysname, VLFCount int);
Create Table #vlfTemp (
    RecoveryUnitId int,
    FileId int, 
    FileSize nvarchar(255), 
    StartOffset nvarchar(255), 
    FSeqNo nvarchar(255), 
    Status int, 
    Parity int, 
    CreateLSN nvarchar(255)
);
Declare @dbId int, @dbName sysname;
Declare dbs Cursor Local Fast_Forward For (
    Select db.database_id, db.name From sys.databases db
    Left Join sys.database_mirroring m ON db.database_id = m.database_id
    Where db.state <> 6
        and ( db.state <> 1 
            or ( m.mirroring_role = 2 and m.mirroring_state = 4 )
            )
    );
Open dbs;
Fetch Next From dbs Into @dbId, @dbName;
While @@FETCH_STATUS = 0
BEGIN   
    IF IS_SRVROLEMEMBER ('sysadmin') = 1 
       INSERT Into #vlfTemp
       EXEC('DBCC LOGINFO(''' + @dbName + ''') WITH NO_INFOMSGS');      
    Insert Into #VLFCounts (DatabaseId, DatabaseName, VLFCount)
    Values (@dbId, @dbName, @@ROWCOUNT); 
    Truncate Table #vlfTemp;
    Fetch Next From dbs Into @dbId, @dbName;
End
Close dbs;
Deallocate dbs;
Select * From #VLFCounts;
Drop Table #VLFCounts;
Drop Table #vlfTemp;

So if they want VLF stats, the user has to be in sysadmin. Otherwise the app will work just fine without it.

Thoughts?

[smurariu]

I could live with this ;)

[SQLSourcerer]

What I'd really like to do is create a script that deploys stored procedures into an instance to be monitored, and sign the stored procedures with certificates associated with a sysadmin login. Then, Opserver can be granted permission to just the procedures, and it would call them instead of the queries it uses now. It's a little bit tricky to get this sort of script working (along with nice characteristics like idempotency), but once in place, it grants exactly the permission needed; no more, no less. I believe this works all the way back to SQL Server 2005. It would also obviate issue #23 completely.

[NickCraver]

@KennethScott absolutely would love this change!

@SQLSourcerer While this may sound appealing to some, it's not a route I'd ever suggest. Installing stored procs across many servers is both a burden and a high bar to getting started. Given it's also not necessary, it's just a code path I'd never want to maintain. Version skew is also just very, very painful. Instead of pulling latest code on Opserver it's pull latest and re-deploy these procs across your infrastructure. I don't agree it removes the call for #23 either, as "what does this thing read from and/or do to my SQL server?" is still a valid question to document.

[NickCraver]

Fixed in #306, thank you!