Backup a database over an SQL connection

Developing some industrial WinForms application for some industrial setting, I wanted to provide users of our software with a convenient way to back up the database the software uses (to send it to developer for investigation of any issues that might arise, or just to keep it at hand in case something goes wrong).

The assumptions (more or less checked to be true in our environment):

• The GUI of software works on some server, accesing the DB over an SQL connection

• The user of the GUI is the person most likely to report the problems; he is an enginier, not an IT guy

• We store no confidential data - anyone with the right to use the database is ok to read it all (not so ok with writing it all over)

• The database server administrator is not readily available (there is no dedicated database administrator; most of the time the thing just works, and when it does not, someone comes to fix the problem - and that "someone", having a rather wide area of responsibility, would like to have less things to care about, not more)

• Some computers this is to work on might be a part of domain, some not; SMB shares might exist, someone might know and/or periodically change the password, network names, all sort of stuff.

The solution:

The solution I came up with is to back up the database onto the same computer the GUI is running on, over the same SQL connection GUI accesses the database through. Restoring is still manual.

The following is the SQL part. So, what do you say about it?

```
CREATE PROCEDURE get_backup
@make bit = 1,
@download bit = 1,
@delete bit = 1
AS
BEGIN
SET NOCOUNT ON;

DECLARE @count int, @part int, @sql nvarchar(max)
DECLARE @files table (n int, pathname nvarchar(2000), path nvarchar(2000), quoted_pathname nvarchar(2000), quoted_name nvarchar(200))

IF @make = 1
BEGIN
DECLARE @size int
SET @size = (SELECT SUM(total_pages)/128.0 AS megabytes FROM sys.allocation_units)
SET @count = (@size + 1000) /

Leaving aside your general design, here are some comments - in no particular order - purely about the code itself:

• There are no comments anywhere. This makes the code very difficult to read. You should have enough comments in the code so that someone can read through them quickly and get an immediate understanding of what the code does. The code itself explains how it does it.

• Your parameter and variable naming is somewhat vague: 'make', 'count', 'size' etc. are all general terms and more precise naming would be clearer and make the code more readable, e.g. @make_backup, @number_of_files and @backup_file_size.

• You're using dynamic SQL heavily but you don't have any easy way to debug it. Dynamic SQL is notoriously error-prone (although often unavoidable), so if you use it then make sure you have an easy way to troubleshoot. I always add a @debug parameter and put lots of if @debug = 0x1 print @sql statements in the code. It can also be useful to add an @execute parameter to control whether or not the dynamic SQL code should be executed or just generated.

• Your code is full of implicit data type conversions: your string variables are nvarchar but your string literals are varchar, and your bit literals are actually integers. nvarchar literals are preceded with N ie. N'literal' and bits are either 0x0 or 0x1. In this specific procedure it will probably make no difference, but when writing queries against large tables conversions can cause noticeable performance problems.

• The condition IF @count IS NULL would be better written as IF @make = 0x0 because the only way @count will be NULL is if the make section wasn't executed. Relying on default behaviour - in this case that an uninitialized variable is NULL - is difficult to read and maintain and breaks easily when you modify what appears to be an unrelated piece of the code.

• xp_delete_file is an undocumented stored procedure. In other words it may change behaviour or vanish completely in the next service pack (even if it's been around for a long time already). Relying on undocumented features always has some risk - even for 'well-known' undocumented features - and I wouldn't ship any code that relies on them.

• You have a 'magic string' in your code: 2080-01-01. There's no comment to explain it, so you're assuming that the next person to maintain this code will understand it immediately. In fact, this often leads to code that everyone is afraid to change. And ironically the maintenance developer struggling to understand it will possibly be you, because you just forgot why you put it there.

• In general, whenever you embed a literal value of any kind in your code (tmp-bak is another example), consider whether or not it should be either declared (and commented!) as a variable as the start of your procedure, or put into a 'configuration' or 'constants' table where it can be more easily maintained and documented. This depends mainly on the scope of the value and how often it's used, of course.

As a general observation, I find this a very strange way of making a backup. Since you're writing a .NET application, I would use SMO to do the whole thing 'natively' in .NET code. TSQL is a very poor language for working with files and anything else outside the database, so you would probably find that SMO is easier. But as always, you know your own situation and requirements best so there may be good reasons for trying to do this all in TSQL.

StackExchange Code Review Q#19598, answer score: 6