SQL: What's wrong with my Stored Procedure?

"darrel" <notreal@nowhere.com> wrote in message
news:eo6fok$8lt$1@forums.macromedia.com...
> As for dropping tables, do you still need to use 'DROP TABLE' at the end
> of the SP or is that redundant as well?

It's not required, but it's considered good practice to DROP TABLE on all
temp tables as soon as you're done with them. There's never any reason to
drop a temp table before you create it. ;)

>> 2. It's not the best idea to use SELECT ... INTO. Depending on how much
>> tempdb activity you have, you might find yourself in locking/blocking
>> wars and have unexplained slowdowns.
>
> Shouldn't this temp table only be in use by this instance of the SP?

That's correct, but to implicitly create the table schema-locks the tempdb;
tempdb is used for all temporary tables on the server. An unrelated temp
table might be blocked while waiting for the query to resolve in an earlier
SELECT ... INTO. Explicity creating the table is safer because nothing
needs to be analyzed before the table is created.

>> 3. SELECT * has no place in production code. Do not use it.
>
> Is that a syntax/style argument or performance argument?

Both. It improves performance sometimes (when you don't need all columns);
it improves safety and reliability sometimes (when more than one table share
a column name); and it definitely improves compatibility with future
changes. If the table definition changes later, your queries still
reference the same columns in the same order - nothing is accidentally
broken by adding columns.

> I'm reading up on dynamic sql now to try and figure out when one would
> actually want to use it...

Basically, you'd only ever use it in place of a list or array (WHERE myCol
IN (@List) doesn't work in a SP; you'd have to have WHERE myCol IN (@Item1,
@Item2, ..., @ItemN). There are certain types of queries where it helps
too, usually in an admin script of some sort that does the same thing to
many different objects.

> Thanks for that. Much simpler, and makes a lot of sense.
>

You're welcome.

>> /* Clean up */
>> If exists (select * from tempdb.dbo.sysobjects where substring(name,1,13)
>> = '#PostingItems')
>> Drop table #PostingItems
>
> There's no point to this. Temporary tables are dropped when they go out
> of scope. Exiting a stored procedure means they go out of scope. (Just
> for clarity's sake - get in the habit of starting every stored procedure
> with BEGIN and ending every stored procedure with END.)

My knowledge of SP has been mainly formed from what we've been using in the
system before the time I got here.

Obviously, that wasn't the best to base things off of. ;o)

As for dropping tables, do you still need to use 'DROP TABLE' at the end of
the SP or is that redundant as well?

>> /* Grab staging server data */
>> Select @SqlRun = 'SELECT * INTO #PostingItems from WePostingContent_NEW
>> WHERE (PostID = ' + str(@postingID) +')'
>> Execute (@SqlRun)
>
> Oooh - mixing a temp table with dynamic SQL. You've created quite a mess
> for yourself here, I'm afraid. Especially since there's absolutely no
> reason to use this STRONGLY discouraged method. List time:
> 1. Avoid dynamic SQL in stored procedures. That means really, really try
> to avoid it.

I've dropped the Dynamic SQL. I'm not even quite sure why it was being used
before.

> 2. It's not the best idea to use SELECT ... INTO. Depending on how much
> tempdb activity you have, you might find yourself in locking/blocking wars
> and have unexplained slowdowns.

Shouldn't this temp table only be in use by this instance of the SP?

> Better to explicity create the table.

Good tip. I'll do that.

> 3. SELECT * has no place in production code. Do not use it.

Is that a syntax/style argument or performance argument?

> Same complaints as above - rewrite both of the above queries as QUERIES,
> and drop the dynamic sql. Why you've even attempted to use it here is
> simply beyond my comprehension.

Mine too. It wasn't an intentional decision as much a it was a lazy decision
(it's what was here before, so I'll just modify it like this...)

I'm reading up on dynamic sql now to try and figure out when one would
actually want to use it...

> Now that I've read the whole thing - why are you even using a temp table?

Umm...uhh...well...err... ;o)

> Here's how I'd write that procedure

Thanks for that. Much simpler, and makes a lot of sense.

-Darrel

The first one didn't work because you were creating the temp table in a SQL
string and executing it, so the temp table would not exist if you tried to
access it out of that scope.

Tom Muck
http://www.tom-muck.com/


"darrel" <notreal@nowhere.com> wrote in message
news:eo666n$r6e$1@forums.macromedia.com...
>> Well, I decided to rewrite it because, in hindsight, I really have no
>> idea why our SP's are written as they are. I've just beeb building them
>> off of the template the last person used. Simplifying it as such seems to
>> work just fine:
>>
>> --------------
>
>
> Oops! Here it is:
>
> CREATE proc dbo.sp_UpdateNewsPosting
> @postingID int
>
> as
>
> /* Vars for Procedure */
> DECLARE
> @SqlRun varchar(1000)
>
> /* Update Posting Date Fields */
> UPDATE WePostingContent_NEW SET DbPostDt_prod = DbPostDt WHERE (PostID =
> @postingID)
>
> /* Grab staging server data */
> SELECT * INTO #PostingItems from WePostingContent_NEW WHERE (PostID =
> @postingID)
>
> /* Delete Production Data */
> DELETE FROM [pubDB].publicSite.WePostingContent_NEW WHERE (PostID =
> @postingID)
>
> /* Move staging data to production */
> INSERT INTO [pubDB].publicSite.WePostingContent_NEW SELECT * FROM
> #PostingItems
>
> /* Clean up */
> Drop table #PostingItems
>
> GO
>
>

"darrel" <notreal@nowhere.com> wrote in message
news:eo5u8q$hsq$1@forums.macromedia.com...
> Can anyone see what's wrong with this? I've been staring at it for an
> hour. I keep getting an error of:

Let's go one-by-one, shall we?

> /* Clean up */
> If exists (select * from tempdb.dbo.sysobjects where substring(name,1,13)
> = '#PostingItems')
> Drop table #PostingItems

There's no point to this. Temporary tables are dropped when they go out of
scope. Exiting a stored procedure means they go out of scope. (Just for
clarity's sake - get in the habit of starting every stored procedure with
BEGIN and ending every stored procedure with END.)

> /* Grab staging server data */
> Select @SqlRun = 'SELECT * INTO #PostingItems from WePostingContent_NEW
> WHERE (PostID = ' + str(@postingID) +')'
> Execute (@SqlRun)

Oooh - mixing a temp table with dynamic SQL. You've created quite a mess
for yourself here, I'm afraid. Especially since there's absolutely no
reason to use this STRONGLY discouraged method. List time:
1. Avoid dynamic SQL in stored procedures. That means really, really try to
avoid it.
2. It's not the best idea to use SELECT ... INTO. Depending on how much
tempdb activity you have, you might find yourself in locking/blocking wars
and have unexplained slowdowns. This is also why Julian thought you hadn't
recreated the table - SELECT ... INTO creates the table after sniffing the
result set to see what the data types and names are. The tempdb is
schema-locked while that happens. Better to explicity create the table.
3. SELECT * has no place in production code. Do not use it.
4. Always include the owner in your table names, ESPECIALLY if the owner is
not dbo.

>
> /* Delete Production Data */
> Select @SqlRun = 'DELETE FROM [pubDB].publicSite.WePostingContent_NEW
> WHERE (PostID = ' + str(@postingID) +')'
> Execute (@SqlRun)
>
>
> /* Move staging data to production */
> INSERT INTO [pubDB].publicSite.WePostingContent_NEW
> SELECT *
> FROM #PostingItems

Same complaints as above - rewrite both of the above queries as QUERIES, and
drop the dynamic sql. Why you've even attempted to use it here is simply
beyond my comprehension.

Now that I've read the whole thing - why are you even using a temp table?
That's a waste of resources. You're not doing any processing on the result
set, but are simply moving it from one place to another. That's what an
INSERT query is for.
Here's how I'd write that procedure (I've used "dbo" as the owner in your
local database and left publicSite as the owner on your pubDB database;
obviously, change dbo to publicSite if that's also the owner on your local
database):

CREATE PROCEDURE [dbo].[UpdateNewsPosting]
@PostingID INT
AS
BEGIN
SET NOCOUNT ON;

/* Update Posting Date Fields */
UPDATE dbo.WePostingContent_NEW
SET DbPostDt_prod = DbPostDt
WHERE PostID = @PostingID

/* Delete Production Data */
DELETE FROM pubDB.publicSite.WePostingContent_NEW
WHERE PostID = @PostingID

/* Move staging data to production */
INSERT INTO pubDB.publicSite.WePostingContent_NEW(Col1, Col2, ..., ColN)
SELECT Col1, Col2, ..., ColN
FROM dbo.WePostingContent_NEW
WHERE PostID = @PostingID
END
GO

> Well, I decided to rewrite it because, in hindsight, I really have no idea
> why our SP's are written as they are. I've just beeb building them off of
> the template the last person used. Simplifying it as such seems to work
> just fine:
>
> --------------


Oops! Here it is:

CREATE proc dbo.sp_UpdateNewsPosting
@postingID int

as

/* Vars for Procedure */
DECLARE
@SqlRun varchar(1000)

/* Update Posting Date Fields */
UPDATE WePostingContent_NEW SET DbPostDt_prod = DbPostDt WHERE (PostID =
@postingID)

/* Grab staging server data */
SELECT * INTO #PostingItems from WePostingContent_NEW WHERE (PostID =
@postingID)

/* Delete Production Data */
DELETE FROM [pubDB].publicSite.WePostingContent_NEW WHERE (PostID =
@postingID)

/* Move staging data to production */
INSERT INTO [pubDB].publicSite.WePostingContent_NEW SELECT * FROM
#PostingItems

/* Clean up */
Drop table #PostingItems

GO

> I was under the impression that just stating the temp table was enough to
> create it. Is that incorrect?

This page seems to confirm that:

http://www.sqlteam.com/item.asp?ItemID=2029

stating that you can just use the SELECT INTO to create the temp table.

So...must be something else...

Well, I decided to rewrite it because, in hindsight, I really have no idea
why our SP's are written as they are. I've just beeb building them off of
the template the last person used. Simplifying it as such seems to work just
fine:

--------------

> Looks like you've dropped the temp table and then not recreated it. I'd
> also suggest cleaning up at the end, not the beginning.

I was under the impression that just stating the temp table was enough to
create it. Is that incorrect?

Of all the stored procedures we use with temp tables, none of the
explicitely CREATE the table. That doesn't mean we're not wrong, of course.
;o)

-Darrel

Looks like you've dropped the temp table and then not recreated it. I'd also
suggest cleaning up at the end, not the beginning.

--
Jules
http://www.charon.co.uk/charoncart
Charon Cart 3
Shopping Cart Extension for Dreamweaver MX/MX 2004