Generating email with BCP

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', 'xxx@gmail.com', '1'), ('Red', 'R11', '10', '14', 'xxx1@gmail.com', '1')

Se

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 > (>)
SELECT >
  FROM > 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:

• Don't use a bare SELECT *, use an explicit select list

• Instead of dyanamically including the @SaleId stuff, you can just put the logic in there

• 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.

• 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

StackExchange Code Review Q#145957, answer score: 2