First attempt at Stored Procedure - can anyone offer advice

Collapse
This topic is closed.
X
X
 
  • Time
  • Show
Clear All
new posts
  • aaj

    First attempt at Stored Procedure - can anyone offer advice

    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_CL IENT_WEEKLY_NOT IFICATION_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_n otification_ema il_1

    -- open a cursor to the same distinct list
    declare email_list cursor for select distinct manager_name from
    dbo.vw_client_n otification_ema il_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_recor d) < @no_of_managers
    begin
    fetch next from email_list into @manager_name
    EXECUTE dbo.pr_admin_cl ient_weekly_not ification @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_CL IENT_WEEKLY_NOT IFICATION
    (@current_manag er_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,aborte d_visit,link,ma nager_name,mana ger_email from
    vw_client_notif ication_email_1 where manager_name = @current_manage r_name
    open email_contents
    --some initial text
    set @body = '<font color="#FF8040" ><b>Reports W/E ' +convert(char(5 0),
    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,@abort ed,@link,@manag er_name,@manage r_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_s tatus = 0)
    begin
    fetch next from email_contents into
    @region,@type,@ site_ref,@abort ed,@link,@manag er_name,@manage r_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_s mtp_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

    #2
    Re: First attempt at Stored Procedure - can anyone offer advice


    "aaj" <a.b@c.com> wrote in message
    news:40236978$0 $15408$afc38c87 @news.easynet.c o.uk...[color=blue]
    > 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[/color]
    problem.[color=blue]
    > 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[/color]
    code[color=blue]
    >[/color]

    <snip>
    [color=blue]
    > 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[/color]
    have[color=blue]
    > to fetch, then check, manipulate then fetch again, meaning that the same
    > manipulation code is written twice. thats why in the first procedure I[/color]
    used[color=blue]
    > 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
    >[/color]

    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_n otification_ema il_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_sendmai l 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


    Comment

    • Erland Sommarskog

      #3
      Re: First attempt at Stored Procedure - can anyone offer advice

      aaj (a.b@c.com) writes:[color=blue]
      > 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.[/color]

      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_s endmail ....
      -- Reset all variables.
      SELECT @old_manager = @manager
      END

      You could of course have to order the cursor by manager.
      [color=blue]
      > @@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.[/color]

      @@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, sommar@algonet. se

      Books Online for SQL Server SP3 at
      Get the flexibility you need to use integrated solutions, apps, and innovations in technology with your data, wherever it lives—in the cloud, on-premises, or at the edge.

      Comment

      • aaj

        #4
        Re: First attempt at Stored Procedure - can anyone offer advice

        Thanks chaps

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


        Andy

        "aaj" <a.b@c.com> wrote in message
        news:40236978$0 $15408$afc38c87 @news.easynet.c o.uk...[color=blue]
        > 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[/color]
        problem.[color=blue]
        > 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[/color]
        code[color=blue]
        >
        > The purpose of the procedures is as follows : we have a view of lots of[/color]
        bits[color=blue]
        > of information that need automatically mailing to different people. each
        > element of information has a name allocated against it. If we had 100[/color]
        pieces[color=blue]
        > of data, 50 could go to manager 1 25 could go to manager 2 and 25 to[/color]
        manager[color=blue]
        > 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[/color]
        information[color=blue]
        > belonging to that manager off in an EMAIL to him/her. ( It uses a[/color]
        brilliant[color=blue]
        > 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_CL IENT_WEEKLY_NOT IFICATION_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_n otification_ema il_1
        >
        > -- open a cursor to the same distinct list
        > declare email_list cursor for select distinct manager_name from
        > dbo.vw_client_n otification_ema il_1 dsc
        > open email_list
        >
        > -- for each distinct manager get the managers name and pass it to the[/color]
        stored[color=blue]
        > procedure that generates the mail.
        > set @current_record = 0
        > while (@current_recor d) < @no_of_managers
        > begin
        > fetch next from email_list into @manager_name
        > EXECUTE dbo.pr_admin_cl ient_weekly_not ification @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_CL IENT_WEEKLY_NOT IFICATION
        > (@current_manag er_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,aborte d_visit,link,ma nager_name,mana ger_email from
        > vw_client_notif ication_email_1 where manager_name = @current_manage r_name
        > open email_contents
        > --some initial text
        > set @body = '<font color="#FF8040" ><b>Reports W/E ' +convert(char(5 0),
        > getdate()) + '</b></font><br><br> <a href = http://xxxx > Click here to[/color]
        log[color=blue]
        > 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,@abort ed,@link,@manag er_name,@manage r_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[/color]
        to[color=blue]
        > the body of the text
        > while(@@fetch_s tatus = 0)
        > begin
        > fetch next from email_contents into
        > @region,@type,@ site_ref,@abort ed,@link,@manag er_name,@manage r_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_s mtp_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[/color]
        have[color=blue]
        > to fetch, then check, manipulate then fetch again, meaning that the same
        > manipulation code is written twice. thats why in the first procedure I[/color]
        used[color=blue]
        > 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
        >
        >
        >[/color]


        Comment

        Working...