The most proper use of a sql cursor ever



  • TRUNCATE TABLE tblBrand
    
    DECLARE CUR_BRAND CURSOR FOR
    SELECT DISTINCT BRAND FROM V_OVER
    OPEN CUR_BRAND;
    FETCH NEXT FROM CUR_BRAND INTO @BRAND
    WHILE @@FETCH_STATUS = 0
    	BEGIN
    		INSERT INTO tblBrand values (@LC,@BRAND)
    		SET @LC = @LC + 1
    		FETCH NEXT FROM CUR_BRAND INTO @BRAND
    	END
    CLOSE CUR_BRAND
    DEALLOCATE CUR_BRAND
    


  • That is serious. I noticed the extra-ugly syntax, like the designer almost completely but not entirely couldn't remember what PL/SQL looks like. Perhaps this db doesn't allow rownum to be used in inserts (just like Oracle doesn't allow you to use sequences)?



  • Very strange - this is what happens when you allow procedural programmers to touch the database. They write procedures.

    I don't see where @LC is initialized, and there is no ORDER BY in the cursor declaration, so the ID numbers you're going to get are defined to be random. In practice they will tend to be ordered in the same way each time, but there is no requirement for that, and SQL Server (or whatever) is allowed to spit those rows back at you in ANY order it wants. So, hopefully they aren't using those numbers as keys of any kind - because one of these days, SQL Server will re-partition that table, and your select will come out in a completely different order.

    Doing a bulk insert into a table with an identity column is the way to go here...



  • Assuming this is SQL Server, @LC has to be initialized to a valid number beforehand if this is to work; I think the OP left out some stuff for brevity.
    If is has a value of NULL, then SET @LC = @LC + 1 will set @LC to NULL again.



  • Just to clarify,

    This is MS SQL Server 2005

    @LC is Initialized and given an initial value of 1

    this should have been a identity column with a seed of 1 and increment of 1 and one single insert command.

    It is just one of the atrocities that I come across at this clients code base.



  • The only excuse for this would be if some idiot wrote a trigger on tblBrand that doesn't properly handle multi-row inserts. I'm willing to bet that's not the case here, though, and it still wouldn't account for the broken reimplementation of an identity column.



  • I have also seen this style of update used when you have a table that needs high-availability. If you do this right, it updates one row at a time and never locks the table for very long, so if there's a lot of query activity against the table all the time, then updating one row at a time hurts performance for the update, but gives you a moderate increase, occasionally, on the client queries. Since you probably only do updates once in a while, it's kinda of a stupid trade-off. However, if this procedure was running continuously or quite often, it might make sense. At any rate, you wouldn't do it with a cursor, as it wouldn't give you the 'non-locking' behavior anyway. You're not in Utah, are you?



  • Not in Utah, just down here in the Netherlands.



    There are a couple of hundred users using this database. This particular cursor is at the end of a 500+ line sp that does one update after another without a transaction which often leads to corrupted data.



    The proces for every table is:

    1. truncate the table
    2. insert some product ids
    3. run a seperate update statement for every single column in the table (most of the tables are 10 columns or so)



      The import proces itself is started by pressing a button in an msaccess frontend and takes an hour or so importing about 12 different files. after that 2 sp's like the one described in here are used to fill some staging tables and one more to fill the eventual tables that hold the data used for the reports they need.



      THe worst thing is that this is one of 3 dozen applications we need to maintain for this customer and this little bit is kind of exemplary for the whole lot of them :-(




  • OMG. Good luck with that :)



  • It could be worse.

    I could be the a**hole that wrote this and be hated by all ;-) Since I didn't write it I am just hated by some



  • @jasmine2501 said:

    I have also seen this style of update used when you have a table that needs high-availability. If you do this right, it updates one row at a time and never locks the table for very long

    If you need high availability, truncating the table is probably not a good first step.



  • @Scarlet Manuka said:

    If you need high availability, truncating the table is probably not a good first step.
     

    Oh, shit.


Log in to reply