Generating email with BCP

Question

This code iterates through the table @Table1 to get the Purchaser, PurchaserID, SaleID store the values in a variable, then queries a secondary table to get data for the specific purchaser. 2 different data sets (one for daily, one for weekly) checks if a folder for the purchaser already exists. If not, it creates then saves the .csv and emails it to the email address listed for the purchaser in @Table1.

The code works, but how would you improve it?

CREATE TABLE [dbo].[AI](
[ID] [int] IDENTITY(1,1) NOT NULL,
[saledate] [datetime] NULL,
[trn] [varchar](max) NULL,
[purchaser] [varchar](max) NULL,
[primaryaddress] [varchar](max) NULL,
[secondaryaddress] [varchar](max) NULL,
[city] [varchar](200) NULL,
[state] [varchar](50) NULL,
[zip] [varchar](10) NULL,
[purchaserid] [varchar](50) NULL,
[saleID] [varchar](50) NULL)
CREATE TABLE [dbo].[LI](
[ID] [int] IDENTITY(1,1) NOT NULL,
[saledate] [datetime] NULL,
[trn] [varchar](max) NULL,
[purchaser] [varchar](max) NULL,
[primaryaddress] [varchar](max) NULL,
[secondaryaddress] [varchar](max) NULL,
[city] [varchar](200) NULL,
[state] [varchar](50) NULL,
[zip] [varchar](10) NULL,
[itempurchased] [varchar](max) NULL,
[amtpurchased] [int] NULL,
[amtshipped] [int] NULL,
[purchaserid] [varchar](50) NULL,
[saleID] [varchar](50) NULL)

DECLARE @SendEmailTo VARCHAR(MAX), @Purchaser NVARCHAR(500), @AI nvarchar(4000), @body nvarchar(max);
DECLARE @emailbody varchar(MAX),@filedate varchar(500), @sql varchar(8000), @DateToAppend nvarchar(100);
DECLARE @LI nvarchar(4000), @PurchaserID varchar(50), @SaleID varchar(50);
DECLARE @chkdirectory as nvarchar(4000), @folder_exists as int, @EmailAttachment nvarchar(MAX);
Declare @Data1 Table (purchaser varchar(500), purchaserid varchar(50), saleid varchar(50), email varchar(100), active varchar(5))
INSERT INTO @Data1 (purchaser, purchaserid, saleid, email, active) VALUES
('Green', 'G12', '22', '12', '[email protected]', '1'), ('Red', 'R11', '10', '14', '[email protected]', '1')
Select distinct([purchaser]) purchaser,[purchaserid] purchaserid
,[saleID] saleID Into #holdingtable FROM @Data1 
WHERE [Active] = '1'
Order By [purchaser] ASC
DECLARE cursor1 CURSOR FOR
SELECT purchaser
,purchaserid
,saleid
FROM #holdingtable 
ORDER BY purchaser ASC
OPEN cursor1
FETCH NEXT FROM cursor1 INTO @Purchaser, @PurchaserID, @SaleID
WHILE @@FETCH_STATUS = 0
BEGIN
SET @SendEmailTo = '';
Select @SendEmailTo = COALESCE(@SendEmailTo + ';', '') + [EmailAddress]
from @Data1
WHERE [purchaserid] = @PurchaserID
AND ISNULL([saleid], '') = ISNULL(@SaleID, '')
SET @SendEmailTo = case
 when @SendEmailTo LIKE ';%' then RIGHT(@SendEmailTo, LEN(@SendEmailTo)-1)
 ELSE @SendEmailTo
 END
SET @DateToAppend = REPLACE(CONVERT (CHAR(10), getdate(), 101),'/','');
if @SaleID >= 1
BEGIN
 Set @AI = 'C:\'+@Purchaser+'\AI_'+@DateToAppend+'.csv'
 SET @LI = 'C:\'+@Purchaser+'\LI_'+@DateToAppend+'.csv'
END
ELSE
BEGIN
 Set @AI = 'C:\'+@Purchaser+'\AI_'+@DateToAppend+'.csv'
 SET @LI = 'C:\'+@Purchaser+'\LI_'+@DateToAppend+'.csv'
END
if @SaleID >=1 
BEGIN
 SET @SQL = 'INSERT INTO [LI]([saledate],[trn],[purchaser],[primaryaddress],[secondaryaddress],[city],[state],[zip],[itempurchased],[amtpurchased],[amtshipped],[purchaserid],[saleID]) '
END
ELSE
BEGIN
 SET @SQL = 'INSERT INTO [LI]([saledate],[trn],[purchaser],[primaryaddress],[secondaryaddress],[city],[state],[zip],[itempurchased],[amtpurchased],[amtshipped],[purchaserid]) '
END
SET @SQL = @SQL + 'Select *, '+@PurchaserID+' '
if @SaleID >= 1
BEGIN
 SET @SQL = @SQL + ','+@SaleID+' '
END
SET @sql = @SQL + 'FROM seccompliance '
 + 'AND CAST(Crl.[pijad] AS NVARCHAR(100)) = '+@PurchaserID+' '
if @SaleID >= 1
BEGIN
 SET @sql = @sql + 'AND (CAST(ISNULL([cag].[Playja], '''') AS NVARCHAR(100)) = ISNULL('+@SaleID+', '''')) '
END
EXECUTE(@SQL)
if @SaleID >=1 
BEGIN
 SET @SQL = 'INSERT INTO [AI]([trn],[saledate],[purchaser],[primaryaddress],[secondaryaddress],[city],[state],[zip],[purchaserid],[saleID]) '
END
ELSE
BEGIN
 SET @SQL = 'INSERT INTO [AI]([trn],[saledate],[purchaser],[primaryaddress],[secondaryaddress],[city],[state],[zip],[purchaserid]) '
END
SET @SQL = @SQL + 'Select *, '+@PurchaserID+' '
if @SaleID >= 1
BEGIN
 SET @SQL = @SQL + ','+@SaleID+' '
END
 SET @sql = @SQL + 'FROM seccompliance '
 + 'AND CAST(Crl.[pijad] AS NVARCHAR(100)) = '+@PurchaserID+' '
if @SaleID >= 1
BEGIN
 SET @sql = @sql + 'AND (CAST(ISNULL([cag].[Playja], '''') AS NVARCHAR(100)) = ISNULL('+@SaleID+', '''')) '
END
EXECUTE(@SQL)
if @@ROWCOUNT >= 1
BEGIN
set @chkdirectory = 'C:\SaveThis\'+@Purchaser+'\';
declare @file_results table(file_exists int,file_is_a_directory int,parent_directory_exists int)
insert into @file_results(file_exists, file_is_a_directory, parent_directory_exists)
exec master.dbo.xp_fileexist @chkdirectory
select @folder_exists = file_is_a_directory
from @file_results
if @folder_exists = 0
 begin
 EXECUTE master.dbo.xp_create_subdir @chkdirectory
 end 
if @SaleID >= 1
BEGIN
 Select @sql = 'bcp "SELECT ''saledate'',''trn'',''purchaser'',''primaryaddress'',''secondaryaddress'',''city'',''state'',''zip'' UNION ALL SELECT CHAR(34) + CONVERT(varchar(10),[saledate],101) + CHAR(34),CHAR(34) + CAST([trn] As VARCHAR(MAX))+CHAR(34),CHAR(34) + CAST([purchaser] As VARCHAR(MAX))+CHAR(34),CHAR(34) + CAST([primaryaddress] As VARCHAR(MAX))+CHAR(34),CHAR(34) + CAST([secondaryaddress] As VARCHAR(MAX))+CHAR(34),CHAR(34) + CAST([city] As VARCHAR(MAX))+CHAR(34),CHAR(34) + CAST([state] As VARCHAR(MAX))+CHAR(34),CHAR(34) + CAST([zip] As VARCHAR(MAX))+CHAR(34) FROM [AI] WHERE purchaserID = ''' + @PurchaserID + ''' AND saleID = '''+@SaleID+'''" queryout "'
END
ELSE
BEGIN
 Select @sql = 'bcp "SELECT ''saledate'',''trn'',''purchaser'',''primaryaddress'',''secondaryaddress'',''city'',''state'',''zip'' UNION ALL SELECT CHAR(34) + CONVERT(varchar(10),[saledate],101) + CHAR(34),CHAR(34) + CAST([trn] As VARCHAR(MAX))+CHAR(34),CHAR(34) + CAST([purchaser] As VARCHAR(MAX))+CHAR(34),CHAR(34) + CAST([primaryaddress] As VARCHAR(MAX))+CHAR(34),CHAR(34) + CAST([secondaryaddress] As VARCHAR(MAX))+CHAR(34),CHAR(34) + CAST([city] As VARCHAR(MAX))+CHAR(34),CHAR(34) + CAST([state] As VARCHAR(MAX))+CHAR(34),CHAR(34) + CAST([zip] As VARCHAR(MAX))+CHAR(34) FROM [AI] WHERE purchaserID = ''' + @PurchaserID + '''" queryout "'
END 
SET @sql = @sql + @AI + '" -c -t, -T -S '+@@SERVERNAME
exec master..xp_cmdshell @sql
if @SaleID >= 1
BEGIN
 Select @sql = 'bcp "SELECT ''saledate'',''trn'',''purchaser'',''primaryaddress'',''secondaryaddress'',''city'',''state'',''zip'',''itempurchased'',''amtpurchased'',''amtshipped'' UNION ALL SELECT CHAR(34)+CONVERT(varchar(10),[saledate],101)+CHAR(34),CHAR(34)+CAST([trn] As VARCHAR(MAX))+CHAR(34),CHAR(34)+CAST([purchaser] As VARCHAR(MAX))+CHAR(34),CHAR(34)+CAST([primaryaddress] As VARCHAR(MAX))+CHAR(34),CHAR(34)+CAST([secondaryaddress] As VARCHAR(MAX))+CHAR(34),CHAR(34)+CAST([city] As VARCHAR(MAX))+CHAR(34),CHAR(34)+CAST([state] As VARCHAR(MAX))+CHAR(34),CHAR(34)+CAST([zip] As VARCHAR(MAX))+CHAR(34),CHAR(34)+CAST([itempurchased] As VARCHAR(MAX))+CHAR(34),CHAR(34)+CAST([amtpurchased] As VARCHAR(MAX))+CHAR(34),CHAR(34)+CAST([amtshipped] As VARCHAR(MAX)) FROM [LI] WHERE purchaserID = ''' + @PurchaserID + ''' AND saleID = '''+@SaleID+'''" queryout "'
END
ELSE
BEGIN
 Select @sql = 'bcp "SELECT ''saledate'',''trn'',''purchaser'',''primaryaddress'',''secondaryaddress'',''city'',''state'',''zip'',''itempurchased'',''amtpurchased'',''amtshipped'' UNION ALL SELECT CHAR(34)+CONVERT(varchar(10),[saledate],101)+CHAR(34),CHAR(34)+CAST([trn] As VARCHAR(MAX))+CHAR(34),CHAR(34)+CAST([purchaser] As VARCHAR(MAX))+CHAR(34),CHAR(34)+CAST([primaryaddress] As VARCHAR(MAX))+CHAR(34),CHAR(34)+CAST([secondaryaddress] As VARCHAR(MAX))+CHAR(34),CHAR(34)+CAST([city] As VARCHAR(MAX))+CHAR(34),CHAR(34)+CAST([state] As VARCHAR(MAX))+CHAR(34),CHAR(34)+CAST([zip] As VARCHAR(MAX))+CHAR(34),CHAR(34)+CAST([itempurchased] As VARCHAR(MAX))+CHAR(34),CHAR(34)+CAST([amtpurchased] As VARCHAR(MAX))+CHAR(34),CHAR(34)+CAST([amtshipped] As VARCHAR(MAX)) FROM [LI] WHERE purchaserID = ''' + @PurchaserID + '''" queryout "'
END
SET @sql = @sql + @LI + '" -c -t, -T -S '+@@SERVERNAME
exec master..xp_cmdshell @sql
 SET @EmailAttachment = @AI+';'+@LI
SET @body = '<html>
 <body style="background: #e3e3e3;">
 Test Email Notification
 </body>
 </html>';
 EXEC msdb.dbo.sp_send_dbmail
 @profile_name = 'Test',
 @from_address = '[email protected]',
 @recipients= @SendEmailTo,
 @subject= 'System Generated Email', 
 @body_format = 'HTML',
 @body= @body,
 @file_attachments=@EmailAttachment 
END
FETCH NEXT FROM cursor1 INTO @Purchaser, @PurchaserID, @SaleID
END
CLOSE cursor1
DEALLOCATE cursor1
DROP TABLE #holdingtable

Answer 1

Instead of creating a table via SELECT INTO, I always prefer to create them explicitly.

DROP TABLE IF EXISTS #HoldingTable;
CREATE TABLE #HoldingTable
(
 purchaser varchar(500) COLLATE DATABASE_DEFAULT NOT NULL,
 purchaserid varchar(50) COLLATE DATABASE_DEFAULT NOT NULL,
 saleid varchar(50) COLLATE DATABASE_DEFAULT NOT NULL
);
INSERT INTO #HoldingTable
( purchaser, purchaserid, saleid )
 SELECT DISTINCT
 D.purchaser,
 D.purchaserid,
 D.saleid
 FROM @Data1 D
 WHERE D.active = 1;

Unless you have a reason to actually order your input before inserting (e.g. it picks a better plan), just remove the ORDER BY.

You have unsupported string aggregation syntax here:

SELECT @SendEmailTo = COALESCE( @SendEmailTo + ';', '' ) + [EmailAddress]
 FROM @Data1
 WHERE [purchaserid] = @PurchaserID
 AND ISNULL( [saleid], '' ) = ISNULL( @SaleID, '' );

Instead, you should use STUFF:

SET @SendEmailTo = STUFF(( SELECT N';' + EmailAddress
 FROM @Data1
 WHERE [purchaserid] = @PurchaserID
 AND ( saleid = @SaleID
 OR ( saleid IS NULL
 AND @SaleID IS NULL ))
 FOR XML PATH( '' )),
 1,
 1,
 N'' );

This also removes the need to initialize it, or to remove trailing semi-colons. I also got rid of the gross ISNULL() = ISNULL().

Next, some of your other variables I turned into computed columns in your holding table, because it is a bit easier

DROP TABLE IF EXISTS #HoldingTable;
CREATE TABLE #HoldingTable
(
 purchaser varchar(500) COLLATE DATABASE_DEFAULT NOT NULL,
 purchaserid varchar(50) COLLATE DATABASE_DEFAULT NOT NULL,
 saleid varchar(50) COLLATE DATABASE_DEFAULT NULL,
 DateToAppend AS REPLACE( CONVERT( char(10), GETDATE(), 101 ), '/', '' ),
 AI AS CONCAT( N'C:\', purchaser, N'\AI_', REPLACE( CONVERT( char(10), GETDATE(), 101 ), '/', '' ), N'.csv' ),
 LI AS CONCAT( N'C:\', purchaser, N'\LI_', REPLACE( CONVERT( char(10), GETDATE(), 101 ), '/', '' ), N'.csv' ),
 ChkDirectory AS CONCAT( N'C:\SaveThis\', purchaser, N'\' )
);

From there, I looked at your actual insert statement. From here, I discovered a bug - you have a query of the form

INSERT INTO <<TableName>> (<<InsertList>>)
SELECT <<SelectList>>
 FROM <<TableName>> AND

That AND isn't valid; I assume you're trying to join something here, but the code isn't there.

You also have security issues here; you should never blindly concatenate a value with dynamic SQL, as you risk SQL injection. This is always the case, even if you think you can trust the input. Instead, you should parameterize your query and use sys.sp_executesql.

You actually don't need this to be dynamic at all, however.

INSERT INTO [LI]
( [saledate], [trn], [purchaser], [primaryaddress], [secondaryaddress], [city], [state], [zip], [itempurchased], [amtpurchased], [amtshipped], [purchaserid], [saleID] )
 SELECT [saledate],
 [trn],
 [purchaser],
 [primaryaddress],
 [secondaryaddress],
 [city],
 [state],
 [zip],
 [itempurchased],
 [amtpurchased],
 [amtshipped],
 @PurchaserID,
 CASE WHEN @SaleID >= 1 THEN @SaleID
 ELSE NULL END
 FROM seccompliance
 WHERE crl.pijad = @PurchaserID
 AND ( @SaleID >= 1
 OR cag.playja = @SaleID )
 OPTION( RECOMPILE );

Again, I don't know exactly what should be happening in there, but this should be close. A few other notes about this query:

1. Don't use a bare SELECT *, use an explicit select list
2. Instead of dyanamically including the @SaleId stuff, you can just put the logic in there
3. When you have a predicate like CAST( ColumnName AS datatype ) = @ParameterValue, you will get much better performance if you do ColumnName = CAST( @ParameterValue AS datatype ) or ColumnName = @ParameterValue where @ParameterValue has already been cast to the appropriate type.
4. The use of optional parameters (which is what @SaleID effectively is) can have a significant negative impact on plan choice and performance. You'll either need to recompile the query (what I opted to do) or continue using dynamic SQL to avoid that issue. For your use-case, RECOMPILE should be fine - you're already forcing recompiles of the query every iteration of the loop because it isn't parameterized.

There is nothing novel to say aobut the AI vs LI queries, so I'll move along.

When you create your bcp command, it would be much nicer to just give it a stored procedure, so you can avoid more dynamic SQL.

Lastly, I think a lot of the work you do in the CURSOR could be done in a set-based approach, then just use the CURSOR to send the mail. This will perform much better than your current procedure.

DECLARE @PurchaserList table
(
 PurchaserId varchar(50) NOT NULL,
 SaleId varchar(50) NULL
);
INSERT INTO [LI]
( [saledate], [trn], [purchaser], [primaryaddress], [secondaryaddress], [city], [state], [zip], [itempurchased], [amtpurchased], [amtshipped], [purchaserid], [saleID] )
OUTPUT Inserted.purchaserid, Inserted.saleid INTO @PurchaserList( PurchaserId, SaleId )
 SELECT [saledate],
 [trn],
 [purchaser],
 [primaryaddress],
 [secondaryaddress],
 [city],
 [state],
 [zip],
 [itempurchased],
 [amtpurchased],
 [amtshipped],
 @PurchaserID [purchaserid],
 CASE WHEN @SaleID >= 1 THEN @SaleID
 ELSE NULL END [saleid]
 FROM seccompliance
 WHERE crl.pijad = @PurchaserID
 AND ( @SaleID >= 1
 OR cag.playja = @SaleID );

From there, you are going to CURSOR over distinct purchaser and sale combinations (also, you should include the scope in your cursor definition):

DECLARE cursor1 CURSOR LOCAL READ_ONLY FAST_FORWARD FOR
 SELECT DISTINCT
 HoldingTable.purchaserid,
 HoldingTable.purchaser,
 HoldingTable.saleid,
 HoldingTable.AI,
 HoldingTable.LI,
 HoldingTable.ChkDirectory
 FROM #HoldingTable HoldingTable
 INNER JOIN @PurchaserList PurchaserList
 ON HoldingTable.purchaserid = PurchaserList.PurchaserId
 AND ( HoldingTable.saleid = PurchaserList.SaleId
 OR ( HoldingTable.saleid IS NULL
 AND PurchaserList.SaleId IS NULL ))
 ORDER BY HoldingTable.purchaser ASC;

And then the body of your WHILE loop is largely the same, but without the INSERTs into the AI and LI tables