-
Notifications
You must be signed in to change notification settings - Fork 1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
#293 Support for @OutputServerName in sp_BlitzCache #419
Conversation
Catching up with DEV
Started working on this, brought out output logic into it's own block like I did with sp_Blitz. Not functional yet.
This variable now works on sp_BlitzCache! Same limitations as sp_Blitz when trying to populate an XML field cross server, so query plans return as NVARCHAR(MAX).
Tested this on SQL Server 2016 (Got my instance connected to my domain) :) |
BEGIN | ||
IF EXISTS (SELECT server_id FROM sys.servers WHERE QUOTENAME([name]) = @OutputServerName) | ||
BEGIN | ||
SET @LinkedServerDBCheck = 'SELECT 1 WHERE EXISTS (SELECT * FROM '+@OutputServerName+'.master.sys.databases WHERE QUOTENAME([name]) = '''+@OutputDatabaseName+''')' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this work if someone only has permissions to a single database, but not to master.sys.databases? I think generally in linked server situations, the cross-server account users don't have a lot of permissions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As long as they are in the Public role it should be fine. If the DBA/Vendor has removed all of the precreated roles for their own, the linked server account needs to have VIEW ANY DATABASE permission.
From the docs on sys.databases:
By default, the public role has the VIEW ANY DATABASE permission, allowing all logins to see database information. To block a login from the ability to detect a database, REVOKE the VIEW ANY DATABASE permission from public, or DENY the VIEW ANY DATABASE permission for individual logins.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great! Just a few small changes.
QueryText NVARCHAR(MAX), | ||
QueryPlan XML, | ||
NumberOfPlans INT, | ||
NumberOfDistinctPlans INT, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Uh oh - looks like maybe this was started from an older version, and it didn't have all of the current fields. Can you add the ones from lines 2182-2187 in the last version, and test that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure thing!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, just update the issue when that code has been added. (Just went in and checked and it still wasn't there, so noting in here in case you were missing that.)
BEGIN | ||
SET @insert_sql = REPLACE(@insert_sql,''''+@OutputSchemaName+'''',''''''+@OutputSchemaName+'''''') | ||
SET @insert_sql = REPLACE(@insert_sql,''''+@OutputTableName+'''',''''''+@OutputTableName+'''''') | ||
SET @insert_sql = REPLACE(@insert_sql,'''DBCC FREEPROCCACHE ('' + CONVERT(VARCHAR(128), [PlanHandle], 1) + '');''','''''DBCC FREEPROCCACHE ('''' + QUOTENAME(CONVERT(VARCHAR(128), [PlanHandle], 1), CHAR(39)) + '''');''''') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Dumb question - what are we search/replacing this part for? (I'm guessing you found something neat during testing, but if there's an easier way to fix it than doing search/replace, I'm totally down with that - replacing in long strings can be pretty slow.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since I have to use sp_executesql on the other box:
EXEC('EXEC('''+@insert_sql+''') AT ' + @OutputServerName);
...I end up needing extra single-quotes in the dynamic SQL string to make it function.
For the Plan Handles and the Query Hashes, it's the same problem. Without the correct number of single-quotes I end up inserting Plan Handle/Query Hash with a pair of single quotes around it into the output table rather than the code itself.
EDIT: Was using brain upside down when I wrote this the first time...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I should also note that I did it this way for code reuse; and that performance was never an issue here since the insert_sql string is not insanely long or anything. If we were updating a ton of strings it would probably be an issue though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nah, you don't want to do that - let's not try to execute SQL on other servers. (I can see that being a security nightmare.)
END + N' DESC ' | ||
|
||
SET @insert_sql += N' OPTION (RECOMPILE) ; ' | ||
SELECT @insert_sql += N' ORDER BY ' + CASE @SortOrder WHEN 'cpu' THEN ' TotalCPU ' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we still be doing an order by in the insert, come to think of it?
This still hasn't been fixed after a few months, so I'm going to go ahead and close it. Thanks though. |
Fixes #293 for sp_BlitzCache.
Changes proposed in this pull request:
How to test this code:
Use the output from the @expertmode = 1 run to compare the output on both the local and remote tables...they should be the same.
Has been tested on: