First attempt at Stored Procedure - can anyone offer advice

[aaj]

SQL SERVER 2000

Hi all

This is my first attempt at writing a stored procedure. I have managed to
get it working but its unlikely to be the best way of handling the problem.
While writing it I found some things that I don't understand so if any one
could shed any light it would be much appreciated. I have posted these at
the end.

Sorry about the length but I thought it might be worthwhile posting the code

The purpose of the procedures is as follows : we have a view of lots of bits
of information that need automatically mailing to different people. each
element of information has a name allocated against it. If we had 100 pieces
of data, 50 could go to manager 1 25 could go to manager 2 and 25 to manager
3 etc...

Both SP's look at the same view

The first SP generates a distinct list of managers and for each manager
calls the second SP

The second SP filters the view for the data belonging to the selected
manager, and builds an HTML mail. It then sends all the bits of information
belonging to that manager off in an EMAIL to him/her. ( It uses a brilliant
bit of code from sqldev.net to handle the mail)

the first mail then repeats for all the managers in the list

CODE ---- SP 1
ALTER PROCEDURE dbo.PR_ADMIN_CLIENT_WEEKLY_NOTIFICATION_2
AS
begin
SET NOCOUNT ON
declare @no_of_managers as int
declare @current_record as int
declare @manager_name as varchar(100)

-- count how many distinct managers we need to send the mail to
select @no_of_managers = COUNT(DISTINCT manager_name) FROM
dbo.vw_client_notification_email_1

-- open a cursor to the same distinct list
declare email_list cursor for select distinct manager_name from
dbo.vw_client_notification_email_1 dsc
open email_list

-- for each distinct manager get the managers name and pass it to the stored
procedure that generates the mail.
set @current_record = 0
while (@current_record) < @no_of_managers
begin
fetch next from email_list into @manager_name
EXECUTE dbo.pr_admin_client_weekly_notification @manager_name
set @current_record = @current_record+1
end
-- close the cursor
close email_list
deallocate email_list
end

CODE ---- SP2
ALTER PROCEDURE dbo.PR_ADMIN_CLIENT_WEEKLY_NOTIFICATION
(@current_manager_name as varchar(100))
-- a unique managers name is passed from the calling procedure
as begin
SET NOCOUNT ON
-- declarations for use in the stored procedure
DECLARE @to as varchar(100)
DECLARE @entry varchar(500)
DECLARE @region as varchar(100)
DECLARE @type as varchar(100)
DECLARE @site_ref as varchar(100)
DECLARE @aborted as varchar(100)
DECLARE @weblink as varchar(1000)
DECLARE @manager_name as varchar(100)
DECLARE @manager_email as varchar(100)
DECLARE @body VARCHAR(8000)
DECLARE @link varchar(150)
DECLARE @web_base VARCHAR(150)

-- set up a connection to the view that contains the details for the mail

DECLARE email_contents cursor for select region,type,
site_ref,aborted_visit,link,manager_name,manager_e mail from
vw_client_notification_email_1 where manager_name = @current_manager_name
open email_contents
--some initial text
set @body = '<font color="#FF8040"><b>Reports W/E ' +convert(char(50),
getdate()) + '</b></font><br><br> <a href = http://xxxx > Click here to log
on to xxxxx </a><br><br> '
--fetch the first matching record from the table and build the body of the
message
fetch next from email_contents into
@region,@type,@site_ref,@aborted,@link,@manager_na me,@manager_email
set @web_base = 'http://'
set @weblink = @web_base + @link
if @aborted = 0 set @aborted = '' else set @aborted = 'ABORTED'
set @body = @body + '<font size="2"><b> Region </b>' + @region
+ ' <b>Type</b> ' + @type
+ ' <b>Site Reference </b> <a href = "' + @weblink + '">' + @site_ref+
'</a>'
+ ' <b>Unique Report Reference </b>' + @link + '<br>'

-- continue reading the records for this particular message and adding on to
the body of the text
while(@@fetch_status = 0)
begin
fetch next from email_contents into
@region,@type,@site_ref,@aborted,@link,@manager_na me,@manager_email
if @aborted = 0 set @aborted = '' else set @aborted = 'ABORTED'
if (@@fetch_status = 0) set @body = @body + '<b> Region </b>' + @region
+ ' <b>Type</b> ' + @type
+ ' <b>Site Reference </b> <a href = "' + @weblink + '">' + @site_ref+
'</a>'
+ '<b>Unique Report Reference </b>' + @link + '<br>'
end

-- close the cursor
set @body = @body + '</font>'
close email_contents
deallocate email_contents
-- generate the mail
DECLARE @rc int EXEC @rc = master.dbo.xp_smtp_sendmail
@FROM = N'FROM ME',
@TO = @manager_email,
@server = N'server',
@subject = N'Weekly Import',
@message = @body,
@type = N'text/html'
end


Questions

is the way I've done it OK. I thought I would be able to do it in a single
SP but I really struggled nesting the cursor things.

@@fetchstatus seems to be global, so if your using nested cursors, how do
you know which one you are refering to. If you have multiple calls to the
same SP how does it know which instance of the SP it refers to.

When I first wrote it, I used a cursor in SP1 to call SP2, but I couldn't
get the while loop working - I have a feeling it was down to the @@
fetchstatus in the 'calling' procedure being overwritten by the
@@fetchstatus in the 'called' procedure.

The whole @@fetchatus thing seems a bit odd. In the second procedure, I have
to fetch, then check, manipulate then fetch again, meaning that the same
manipulation code is written twice. thats why in the first procedure I used
the select distint count to know how long the record set is so I only have
to run the manipulation code once. Is what I have done wrong?

its possible that the body of the mail could be > 8K, is there another
datatype I can use to hold more than 8K
many thanks for any help or advice

Andy

[Simon Hayes]

"aaj" <a.*@c.com> wrote in message
news:40***********************@news.easynet.co.uk. ..

SQL SERVER 2000

Hi all

This is my first attempt at writing a stored procedure. I have managed to
get it working but its unlikely to be the best way of handling the problem. While writing it I found some things that I don't understand so if any one
could shed any light it would be much appreciated. I have posted these at
the end.

Sorry about the length but I thought it might be worthwhile posting the code
<snip>
Questions

is the way I've done it OK. I thought I would be able to do it in a single
SP but I really struggled nesting the cursor things.

@@fetchstatus seems to be global, so if your using nested cursors, how do
you know which one you are refering to. If you have multiple calls to the
same SP how does it know which instance of the SP it refers to.

When I first wrote it, I used a cursor in SP1 to call SP2, but I couldn't
get the while loop working - I have a feeling it was down to the @@
fetchstatus in the 'calling' procedure being overwritten by the
@@fetchstatus in the 'called' procedure.

The whole @@fetchatus thing seems a bit odd. In the second procedure, I have to fetch, then check, manipulate then fetch again, meaning that the same
manipulation code is written twice. thats why in the first procedure I used the select distint count to know how long the record set is so I only have
to run the manipulation code once. Is what I have done wrong?

its possible that the body of the mail could be > 8K, is there another
datatype I can use to hold more than 8K
many thanks for any help or advice

Andy

I must admit I didn't read your code in detail, but I'm not sure why you
need two procedures. The inner one appears to go through every row in
dbo.vw_client_notification_email_1, so I don't see the benefit of the outer
one (unless perhaps you removed some code to simplify it). Also, the outer
procedure seems to use a counter to find the end of the cursor, but
@@FETCH_STATUS will tell you when you have reached the end of the cursor
anyway.

As for the data type, there are larger data types available (text and
ntext), but xp_smtp_sendmail doesn't support them. If you need to send large
emails, I would consider using an external script instead of pure SQL code -
it's much easier to create attachment files, do text/HTML formatting and
validation etc.If you want to keep control within a procedure, then you
could use xp_cmdshell to call your script, or perhaps just schedule it as a
SQL Agent job.

Simon

[Erland Sommarskog]

aaj (a.*@c.com) writes:

is the way I've done it OK. I thought I would be able to do it in a single
SP but I really struggled nesting the cursor things.
It could be done in a single SP, but splitting it in two has the
advantage that you don't risk that the value of some variable spill
over from the previous manager.

You could also do it with one cursor only. In this case you would
have something like:

IF @old_manager IS NOT NULL AND @manager <> @old_manager
BEGIN
EXEC master.dbo.xp_sendmail ....
-- Reset all variables.
SELECT @old_manager = @manager
END

You could of course have to order the cursor by manager.
@@fetchstatus seems to be global, so if your using nested cursors, how do
you know which one you are refering to. If you have multiple calls to the
same SP how does it know which instance of the SP it refers to.

@@fetch_status refers to the cursor you last operated on. And if you
check @@fetch_status directly after FETCH you are safe. Here is the
idiom for writing a cursor loop:

DECLARE some_cur INSENSITIVE CURSOR FOR
SELECT yadayada

OPEN some_cur

WHILE 1 = 1
BEGIN
FETCH some_cur INTO ...
IF @@fetch_status <> 0
BREAK
...
END

DEALLOCATE some_cur

The INSENSITIVE is there, because the default keyset-driven cursors can
sometimes come with nasty suprises.

By only using one FETCH statement your code is easier to maintain; if you
need another column in the cursor, you only have to change in two places.
--
Erland Sommarskog, SQL Server MVP, so****@algonet.se

Books Online for SQL Server SP3 at
http://www.microsoft.com/sql/techinf...2000/books.asp

[aaj]

Thanks chaps

I'll have a read in detail and see if I can pick up some tips
Andy

"aaj" <a.*@c.com> wrote in message
news:40***********************@news.easynet.co.uk. ..

SQL SERVER 2000

Hi all

This is my first attempt at writing a stored procedure. I have managed to
get it working but its unlikely to be the best way of handling the problem. While writing it I found some things that I don't understand so if any one
could shed any light it would be much appreciated. I have posted these at
the end.

Sorry about the length but I thought it might be worthwhile posting the code
The purpose of the procedures is as follows : we have a view of lots of bits of information that need automatically mailing to different people. each
element of information has a name allocated against it. If we had 100 pieces of data, 50 could go to manager 1 25 could go to manager 2 and 25 to manager 3 etc...

Both SP's look at the same view

The first SP generates a distinct list of managers and for each manager
calls the second SP

The second SP filters the view for the data belonging to the selected
manager, and builds an HTML mail. It then sends all the bits of information belonging to that manager off in an EMAIL to him/her. ( It uses a brilliant bit of code from sqldev.net to handle the mail)

the first mail then repeats for all the managers in the list

CODE ---- SP 1
ALTER PROCEDURE dbo.PR_ADMIN_CLIENT_WEEKLY_NOTIFICATION_2
AS
begin
SET NOCOUNT ON
declare @no_of_managers as int
declare @current_record as int
declare @manager_name as varchar(100)

-- count how many distinct managers we need to send the mail to
select @no_of_managers = COUNT(DISTINCT manager_name) FROM
dbo.vw_client_notification_email_1

-- open a cursor to the same distinct list
declare email_list cursor for select distinct manager_name from
dbo.vw_client_notification_email_1 dsc
open email_list

-- for each distinct manager get the managers name and pass it to the stored procedure that generates the mail.
set @current_record = 0
while (@current_record) < @no_of_managers
begin
fetch next from email_list into @manager_name
EXECUTE dbo.pr_admin_client_weekly_notification @manager_name
set @current_record = @current_record+1
end
-- close the cursor
close email_list
deallocate email_list
end

CODE ---- SP2
ALTER PROCEDURE dbo.PR_ADMIN_CLIENT_WEEKLY_NOTIFICATION
(@current_manager_name as varchar(100))
-- a unique managers name is passed from the calling procedure
as begin
SET NOCOUNT ON
-- declarations for use in the stored procedure
DECLARE @to as varchar(100)
DECLARE @entry varchar(500)
DECLARE @region as varchar(100)
DECLARE @type as varchar(100)
DECLARE @site_ref as varchar(100)
DECLARE @aborted as varchar(100)
DECLARE @weblink as varchar(1000)
DECLARE @manager_name as varchar(100)
DECLARE @manager_email as varchar(100)
DECLARE @body VARCHAR(8000)
DECLARE @link varchar(150)
DECLARE @web_base VARCHAR(150)

-- set up a connection to the view that contains the details for the mail

DECLARE email_contents cursor for select region,type,
site_ref,aborted_visit,link,manager_name,manager_e mail from
vw_client_notification_email_1 where manager_name = @current_manager_name
open email_contents
--some initial text
set @body = '<font color="#FF8040"><b>Reports W/E ' +convert(char(50),
getdate()) + '</b></font><br><br> <a href = http://xxxx > Click here to log on to xxxxx </a><br><br> '
--fetch the first matching record from the table and build the body of the
message
fetch next from email_contents into
@region,@type,@site_ref,@aborted,@link,@manager_na me,@manager_email
set @web_base = 'http://'
set @weblink = @web_base + @link
if @aborted = 0 set @aborted = '' else set @aborted = 'ABORTED'
set @body = @body + '<font size="2"><b> Region </b>' + @region
+ ' <b>Type</b> ' + @type
+ ' <b>Site Reference </b> <a href = "' + @weblink + '">' + @site_ref+
'</a>'
+ ' <b>Unique Report Reference </b>' + @link + '<br>'

-- continue reading the records for this particular message and adding on to the body of the text
while(@@fetch_status = 0)
begin
fetch next from email_contents into
@region,@type,@site_ref,@aborted,@link,@manager_na me,@manager_email
if @aborted = 0 set @aborted = '' else set @aborted = 'ABORTED'
if (@@fetch_status = 0) set @body = @body + '<b> Region </b>' + @region
+ ' <b>Type</b> ' + @type
+ ' <b>Site Reference </b> <a href = "' + @weblink + '">' + @site_ref+
'</a>'
+ '<b>Unique Report Reference </b>' + @link + '<br>'
end

-- close the cursor
set @body = @body + '</font>'
close email_contents
deallocate email_contents
-- generate the mail
DECLARE @rc int EXEC @rc = master.dbo.xp_smtp_sendmail
@FROM = N'FROM ME',
@TO = @manager_email,
@server = N'server',
@subject = N'Weekly Import',
@message = @body,
@type = N'text/html'
end


Questions

is the way I've done it OK. I thought I would be able to do it in a single
SP but I really struggled nesting the cursor things.

@@fetchstatus seems to be global, so if your using nested cursors, how do
you know which one you are refering to. If you have multiple calls to the
same SP how does it know which instance of the SP it refers to.

When I first wrote it, I used a cursor in SP1 to call SP2, but I couldn't
get the while loop working - I have a feeling it was down to the @@
fetchstatus in the 'calling' procedure being overwritten by the
@@fetchstatus in the 'called' procedure.

The whole @@fetchatus thing seems a bit odd. In the second procedure, I have to fetch, then check, manipulate then fetch again, meaning that the same
manipulation code is written twice. thats why in the first procedure I used the select distint count to know how long the record set is so I only have
to run the manipulation code once. Is what I have done wrong?

its possible that the body of the mail could be > 8K, is there another
datatype I can use to hold more than 8K
many thanks for any help or advice

Andy