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