Unsafe string aggregation in sp_DatabaseRestore (code done, testing needed)

[renegm]

sp_DatabaseRestore
Unsafe string aggregation.

1.Variable accumulation: SELECT @sql = @sql + N'KILL ' + CAST(spid as nvarchar(5)) + N';' + NCHAR(13) etc This works almost everytime except, well, when it doesn't. Result is unpredictable.
(Also kill connections or Set single user its not enough.) Quote
https://docs.microsoft.com/en-us/sql/relational-databases/databases/set-a-database-to-single-user-mode?view=sql-server-2017

Prerequisites
Before you set the database to SINGLE_USER, verify that the AUTO_UPDATE_STATISTICS_ASYNC option is set to OFF.

It is necessary to prevent that auto update steals SINGLE_USER.

2. Using XML
   SELECT CHAR(10) + ',DISK=''' + @BackupPathFull + BackupFile + '''' FROM #SplitBackups ORDER BY BackupFile FOR XML PATH('')),1,2,'')
   This is better. But XML is tricky:
   SELECT STUFF((SELECT ',AT&T' FOR XML PATH ('')),1,1,'')
   result is AT&T
   You need
   SELECT STUFF(CAST((SELECT ',AT&T' FOR XML PATH ('')) AS xml).value('.','nvarchar(max)'),1,1,'')
   If you are absolutely sure that there is no html entity, you can avoid jumping through these hoops. But not in this case where a path is referenced

[BrentOzar]

Ok, cool. Thanks for catching this. Read through the Contribution Guide and then I’ll get your pull request in pretty quickly. Thanks!

…

--- Tiny glass keyboard Typos flowing like rivers As winter snow thaws

On Sep 2, 2019, at 5:49 AM, renegm ***@***.***> wrote:  sp_DatabaseRestore Unsafe string aggregation. 1.Variable accumulation: SELECT @Sql = @Sql + N'KILL ' + CAST(spid as nvarchar(5)) + N';' + NCHAR(13) etc This works almost everytime except, well, when it doesn't. Result is unpredictable. (Also kill connections or Set single user its not enough.) Quote https://docs.microsoft.com/en-us/sql/relational-databases/databases/set-a-database-to-single-user-mode?view=sql-server-2017 Prerequisites Before you set the database to SINGLE_USER, verify that the AUTO_UPDATE_STATISTICS_ASYNC option is set to OFF. It is necessary to prevent that auto update steals SINGLE_USER. Using XML SELECT CHAR(10) + ',DISK=''' + @BackupPathFull + BackupFile + '''' FROM #SplitBackups ORDER BY BackupFile FOR XML PATH('')),1,2,'') This is better. But XML is tricky: SELECT STUFF((SELECT ',AT&T' FOR XML PATH ('')),1,1,'') result is AT&T You need SELECT STUFF(CAST((SELECT ',AT&T' FOR XML PATH ('')) AS xml).value('.','nvarchar(max)'),1,1,'') If you are absolutely sure that there is no html entity, you can avoid jumping through these hoops. But not in this case where a path is referenced — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub, or mute the thread.

[renegm]

That was fast.
I'll do it.

[sm8680]

Pertaining to the Kill statement in this sproc. I wasn't sure why the kill statements are needed in this sproc or I'm just clueless and losing my mind.

In my past for restores. If a database already exists I’ve always found it best to take the database offline and restore over the existing database. I almost never use single user mode or kill spids. For their not fail proof IMO.
ALTER DATABASE [YourDatabaseName] SET OFFLINE WITH ROLLBACK IMMEDIATE

Then restore your database or Drop your database

[renegm]

I think the user must be able to do whatever he want. So activity param must address don’t restore, single user or offline. And other param to restore db settings maybe?

[BrentOzar]

It's been several months, and no one has been interested in testing this, so I'm going to go ahead and close it. If you want to revisit it, can you work with other sp_DatabaseRestore users in Slack to see if they can help you get it tested on a few machines, like repro the problem and show that this fixes it? Thanks!