mak
mak

Reputation: 359

MS SQL Server: Trigger fails because stored procedure fails

I have an app which sends up to 20 device barcodes data into SQL-Server table "device usage".

In an AFTER INSERT trigger of this table I have a stored procedure which transfers the barcodes from columns BC1, BC2, BC3, etc. into another table BC usage.

Table BC usage is set to IGNORE_DUP_KEY = ON as it may happen that BCs are scanned twice.

If a duplicate BC is scanned then also the insert in the first table "device usage" fails!

WHY?

I do not understand why the initial insert into table "device usage" fails although the IGNORE_DUP_KEY = ON in the table BC usage which is only used in the stored procedure.

ALTER TRIGGER [dbo].[UpdatePersonanenDaten] 
ON [dbo].[ScanIT_tblGeraeteerfassung] 
AFTER INSERT
AS 
BEGIN
    DECLARE @Personalnummer varchar(10)
    DECLARE @Mitarbeiter varchar(100)
    DECLARE @InOut char(1)=''
    DECLARE @ID int
    DECLARE @StartID int

    SELECT 
        @Personalnummer = a.[Personalnummer], 
        @Mitarbeiter = a.[DeviceUser] 
    FROM 
        [dbo].[ScanIT_tblDevices] a 
    INNER JOIN 
        inserted i ON a.[DeviceID] = i.[DeviceID]

    SELECT @ID = (SELECT ID FROM inserted)

    BEGIN 
        UPDATE [dbo].[ScanIT_tblGeraeteerfassung] 
        SET [Mitarbeiter] = @Mitarbeiter,
            [Personalnummer] = @Personalnummer,
            [InOut] = CASE 
                         WHEN t.[AufAbbau] = 'Aufbau (Start)' 
                            THEN 'S' 
                            ELSE 'E' 
                      END
        FROM ScanIT_tblGeraeteerfassung t
        INNER JOIN inserted i ON t.ID = i.ID
    END

    -- IN THIS SP THE INSERT INTO tbl "BC USAGE"  HAPPENS  and the Dup Key error let the trigger fail!!

    EXEC [dbo].[ScanIT_spGeraeteEinsatz1] @ID

Thanks for any help

Michael

Edit:

this is the stored procedure ScanIT_spGeraeteEinsatz1 I tried to simplyfy the descsription of the whole job before, but in real the BC sent to the server table is followed by the counter reading of the device. So it needs to lookup the last counter reading of the record for this BC before. S stands for Start, E stands for End of the time the device is used. It also may happed, that this device was not in use before at customers side, then we take the counter from the stocktaking to set the initial counter at Start.

And now, if someone scans this device BC twice it has to be blocked, because the FIRST occurrence of this BC is mandatory the Start and the next occurrence has to be the End. Even if someone adds the BC in a Start record by mistake although the device has already been scanned before in another record.

I hope I could explain the job that you understand the whole process.

ALTER PROCEDURE [dbo].[ScanIT_spGeraeteEinsatz1]
    @GeraeteerfassungID int

AS
BEGIN

BEGIN TRANSACTION;   --neu
SAVE TRANSACTION MySavePoint;

    -- SET NOCOUNT ON added to prevent extra result sets from
    -- interfering with SELECT statements.
    SET NOCOUNT ON;

    declare @tablename as varchar(255)   -- tbl, aus der ausglesen wird: 'ScanIT_tblGeraeteerfassung'
    declare @temptable table (BarcodeName varchar(100))    -- im @temptable werden die Feldnamen Geraetebarcode1, Zaehlerstand1, ... eingelesen
    declare @sqlDynamicString nvarchar(400)
    declare @sqlDynamicString2 nvarchar(400)
    declare @NumberBC int, @Counter int
    declare @temptable2 table (Barcode varchar(10), Zaehlerstand varchar(20))
    declare @strCounter varchar(2)
    declare @Geraetebarcode int

    declare @ID int
    declare @L_ID int

    declare @L_InOut char(1)


    declare @StartID int
    declare @GeraeteerfassungIDOUT int
    --declare @Einsatzvon datetime

    Declare @StartZaehlerstand varchar(25)
    Declare @StartDatum datetime

    declare @BC_AnzahlVerwendungen int=0

    DECLARE @LastDS TABLE
            ([ID] [int],
            [GeraeteerfassungIDOUT] [int] ,
            [GeraeteerfassungIDIN] [int] ,
            [Projektnummer] [varchar](20) ,
            [Geraetebarcode] [varchar](100) ,
            [ZaehlerstandOUT] [varchar](50) ,
            [ZaehlerstandIN] [varchar](50) ,
            [Einsatzvon] [datetime] ,
            [Einsatzbis] [datetime] ,
            [InOut] [char](1) ,
            [PaarID] [int] )


    set @tablename = 'ScanIT_tblGeraeteerfassung'
    set @Counter=1

    -- Einsatzvon-Datum/Zeit un d Projektnummer bleiben für alle Datensätze gleich
    --set @Projektnummer=(Select Arbeitsauftrag from [dbo].[ScanIT_tblGeraeteerfassung] where [ID]=@GeraeteerfassungsID);
    --set @Einsatzvon=(Select Sendtime from [dbo].[ScanIT_tblGeraeteerfassung] where [ID]=@GeraeteerfassungsID);
    --set @L_InOut=(select [InOut] from [dbo].[ScanIT_tblGeraeteerfassung] where [ID]=@GeraeteerfassungsID))   

    --select @Projektnummer=Arbeitsauftrag, @Einsatzvon=Sendtime, @L_InOut=InOut from [dbo].[ScanIT_tblGeraeteerfassung] where [ID]=@GeraeteerfassungsID


    --Insert into @temptable (BarcodeName)
    --wie viele Gerätefelder gibt es in dem SCAN-IT Formular bzw. in der Einlesetabelle?
    set @NumberBC=(SELECT count(c.name) FROM sys.columns c WHERE c.object_id = OBJECT_ID(@tablename) and  c.name like 'Geraetebarcode%' ) ;

    -- hier beginnt der Loop NEU    
        while(@Counter <= @NumberBC)
        begin
                    set @strCounter=convert(varchar(2),@Counter)

                    set @sqlDynamicString2 = 'SET @GeraeteBC=(SELECT Geraetebarcode' + @strCounter + ' FROM [dbo].[ScanIT_tblGeraeteerfassung] WHERE ID=' + convert(nvarchar(8),@GeraeteerfassungsID) + ' AND Len(Geraetebarcode' + @strCounter + ')>0)' 

                    EXECUTE sp_executesql @sqlDynamicString2, N'@GeraeteBC nvarchar(8) OUTPUT',@GeraeteBC=@Geraetebarcode OUTPUT

                    --zuerst muß einmal geschaut werden, ob dieses Gerät überhaupt schon mal im Einsatz war, daher wird die [dbo].[ScanIT_tblGeraeteeinsatz] ausgezählt
                    --bei ERSTMALIGER Verwendung ist die Anzahl 0
                    Set @BC_AnzahlVerwendungen=(Select Count(*) from [dbo].[ScanIT_tblGeraeteeinsatz] where [Geraetebarcode]=@Geraetebarcode)

                    print @Geraetebarcode
                    print @BC_AnzahlVerwendungen

                    -- es wird vorerst der Zählerstand, die Sendtime und die ID für in/out eingegeben, weil erst danach bewertet wird, ob das ein IN oder OUT Datensatz ist
                    set @sqlDynamicString=concat('SELECT  ID, ID ,', @Projektnummer, ', Geraetebarcode' + @strCounter, ', Zaehlerstand' + @strCounter, ', Zaehlerstand' + @strCounter, ', Sendtime',', Sendtime',', InOut',
                                                 ' FROM [dbo].[ScanIT_tblGeraeteerfassung] WHERE ID= ', @GeraeteerfassungsID, ' AND Len(Geraetebarcode' + @strCounter, ')>0' )

                    print @sqlDynamicString

                    BEGIN TRY    --neu
                    Insert into [dbo].[ScanIT_tblGeraeteeinsatz] ([GeraeteerfassungIDOUT],
                                                                  [GeraeteerfassungIDIN],
                                                                  [Projektnummer],
                                                                  [Geraetebarcode],
                                                                  [ZaehlerstandOUT],
                                                                  [ZaehlerstandIN],  
                                                                  [Einsatzvon],
                                                                  [Einsatzbis],
                                                                  [InOut])
                    exec (@sqlDynamicString)

                    COMMIT TRANSACTION 
                    END TRY
                        BEGIN CATCH
                            IF @@TRANCOUNT > 0
                            BEGIN
                                ROLLBACK TRANSACTION MySavePoint; -- rollback to MySavePoint
                            END
                        END CATCH


                    --die ID des eben eingegebenen Datensatzes
                    set @ID=SCOPE_IDENTITY() 


                    -- nun wird geschaut, ob der Barcode schon einmal vokam
                    -- Gerät war noch NIE im Einsatz, d.h. den Startzählerwert aus der [dbo].[Lager_tblInventarArtikel] suchen
                        if @BC_AnzahlVerwendungen = 0   

                            begin           

                                Update a  set   --a.InOut= 'S', 
                                                a.[ZaehlerstandOUT]=coalesce(a.[ZaehlerstandOUT],b.[Anfangsstand]),  --zuerste wird geschaut, ob ein Zählerstand angliefert wird, wenn nicht wird im Lagerstand geschaut
                                                a.[ZaehlerstandIN]='', 
                                                a.[Einsatzbis]=NULL, 
                                                a.[GeraeteerfassungIDIN]= NULL,
                                                a.[PaarID]=a.ID
                                    from [dbo].[ScanIT_tblGeraeteeinsatz] a 
                                    left join [dbo].[Lager_tblInventarArtikel] b on a.[Geraetebarcode]=b.[Barcode] 
                                    where a.ID=@ID

                            end         

                    --Gerät war schon im Einsatz, daher suchen aus [dbo].[ScanIT_tblGeraeteeinsatz]

                    if @BC_AnzahlVerwendungen > 0

                    begin           
                        --print @inout
                        --ID des Startdatensatzes suchen

                        print @Projektnummer
                        print @Einsatzvon 
                        print @L_InOut
                        --set @L_InOut=(select [InOut] from @LastDS)


                        --print @L_InOut


                        if @L_InOut='S'
                        --zuerst den zuvor gelieferten Datensatz suchen, um den Anfangstand zu setzen
                        begin
                                --Befüllen der Table Variablen mit dem vorletzten Datensatz 
                                INSERT INTO @LastDS 
                                            ([ID] ,
                                            [GeraeteerfassungIDOUT]  ,
                                            [GeraeteerfassungIDIN]  ,
                                            [Projektnummer]  ,
                                            [Geraetebarcode]  ,
                                            [ZaehlerstandOUT]  ,
                                            [ZaehlerstandIN]  ,
                                            [Einsatzvon]  ,
                                            [Einsatzbis]  ,
                                            [InOut]  ,
                                            [PaarID] ) 


                                SELECT Top 1 [ID] ,
                                            [GeraeteerfassungIDOUT]  ,
                                            [GeraeteerfassungIDIN]  ,
                                            [Projektnummer]  ,
                                            [Geraetebarcode]  ,
                                            [ZaehlerstandOUT]  ,
                                            [ZaehlerstandIN]  ,
                                            [Einsatzvon]  ,
                                            [Einsatzbis]  ,
                                            [InOut]  ,
                                            [PaarID]
                                from [dbo].[ScanIT_tblGeraeteeinsatz] 
                                where [Geraetebarcode]=@Geraetebarcode and ID < @ID order by ID desc 
                        end         

                        begin
                            Update a set  
                                     a.[GeraeteerfassungIDIN]=NULL  --weil GeraeteerfassungIDOUT existiert, braucht man sie nicht setzen
                                     ,a.[ZaehlerstandOut]= coalesce(a.[ZaehlerstandOut],x.[ZaehlerstandOUT])  --wenn ZaehlerstandOut abgelesen wurde, braucht man ihn nicht setzen, wenn nicht, dann aus letztem DS verwenden
                                     ,a.[ZaehlerstandIN]=''
                                    -- ,a.[Einsatzvon]=x.[Einsatzvon]               --weil Einsatzvon existiert, braucht man sie nicht setzen
                                     ,a.[Einsatzbis]= null
                                     ,a.[PaarID]=a.ID  
                            from [dbo].[ScanIT_tblGeraeteeinsatz] a 
                              left join  @LastDS x on a.[Geraetebarcode]=x.[Geraetebarcode]
                              where a.[ID]=@ID 
                        end

                        if @L_InOut='E'
                        --hier muß man nach dem letzten DS suchen, weil es einen S geben muß!!
                        --da diese Nummer schon verwendet wurde, kann man nach dem S Datensatz zu diesem Gerät suchen
                        begin
                                --Befüllen der Table Variablen mit dem vorletzten Datensatz 
                                INSERT INTO @LastDS 
                                            ([ID] ,
                                            [GeraeteerfassungIDOUT]  ,
                                            [GeraeteerfassungIDIN]  ,
                                            [Projektnummer]  ,
                                            [Geraetebarcode]  ,
                                            [ZaehlerstandOUT]  ,
                                            [ZaehlerstandIN]  ,
                                            [Einsatzvon]  ,
                                            [Einsatzbis]  ,
                                            [InOut]  ,
                                            [PaarID] ) 


                                SELECT Top 1 [ID] ,
                                            [GeraeteerfassungIDOUT]  ,
                                            [GeraeteerfassungIDIN]  ,
                                            [Projektnummer]  ,
                                            [Geraetebarcode]  ,
                                            [ZaehlerstandOUT]  ,
                                            [ZaehlerstandIN]  ,
                                            [Einsatzvon]  ,
                                            [Einsatzbis]  ,
                                            [InOut]  ,
                                            [PaarID]
                                from [dbo].[ScanIT_tblGeraeteeinsatz] 
                                where [Geraetebarcode]=@Geraetebarcode and ID < @ID order by ID desc 
                        end         


                        select * from @LastDS

                        begin
                        Update a set 
                                     a.[GeraeteerfassungIDIN]=x.[GeraeteerfassungIDOUT] 
                                     ,a.[ZaehlerstandOut]= x.[ZaehlerstandOut] 
                                     --,a.[ZaehlerstandIN]= NULL   --steht schon drinnen
                                     ,a.[Einsatzvon]=x.[Einsatzvon] 
                                     --,a.[Einsatzbis]= NULL        --steht schon drinnen
                                     ,a.[PaarID]=x.ID  
                                     ,a.[Verbrauch]= coalesce(cast(a.[ZaehlerstandIN] as float),0)-coalesce(cast(x.[ZaehlerstandOUT] as float),0) 
                                     ,a.[Standzeit]= datediff(d, coalesce(x.[Einsatzvon],0), @Einsatzvon)+1 

                              from [dbo].[ScanIT_tblGeraeteeinsatz] a 
                              left join  @LastDS x on a.[Geraetebarcode]=x.[Geraetebarcode]
                              where a.[ID]=@ID 
                        end

                    end

                    -- zuletzt @Counter hochzählen, dann loop
                    set @Geraetebarcode=null
                    set @BC_AnzahlVerwendungen=null 

                    delete from @LastDS --tablevariable leeren

                    set @Counter=@Counter+1

            end     

    --  Select @StartZaehlerstand, @StartDatum, @StartID,@InOut, @ID, @Counter

END

Upvotes: 0

Views: 418

Answers (1)

Kate
Kate

Reputation: 1836

TL;DR: the obvious question is: why are you even trying to insert duplicate key values ? You should address the root of the problem instead of looking for workarounds that do not solve anything and create more problems.

If is not hard to check whether a record already exists in a table and skip insertion. To put it differently, you should strive for quality of the data at the source, not clean up and sort duplicates after.

Short answer: there is no easy fix for your problem, because you have a broad range of issues and we dot not have a copy of your data and environment to provide in-depth advice. Long answer: see below for some suggestions.


Without having a full insight to your application I nonetheless have the impression that the approach is bloated and unnecessary. Transferring records to another table is duplication, waste of data storage and unreliable if your trigger is not well-coded.

Why not build a view (or a stored procedure) that points to the original table ? I don't even think you should try to fix this code, simplify your code instead. Reconsider your data model and the flow of operations.

Your procedures are already complicated enough, you even have dynamic SQL that as far as I can tell is not justified. Very seldom is it justified, because it adds a layer of complication and can be a security risk.

When you make things more complicated than they should be, the code becomes hard to read, less reliable and difficult to debug. There are so many hooplas to perform basic operations that should be relatively straightforward.

We don't have a copy of your data so it is difficult to reproduce your issue. If you posted your table structure with indexes and constraints, and some sample data maybe it would possible to go further.

Another problem is that you do not have comprehensive error handling in your code. That's something you should start doing from now on to make your code more reliable and more robust. Then, when an error occurs, you could catch the contents of certain variables and gain more information on the offending data. Have a look here for example: How to implement error handling in SQL Server. Just 10 lines of code would make a difference.

You have some transactions but this is not sufficient. They are not used properly and do not cover the whole procedures.

Using a trigger is often a bad solution, especially when you already have a stored procedure to insert/update the data. You should keep the logic concentrated in one place if possible. Too often, triggers are misunderstood and miscoded under wrong assumptions.

If you are still reading this, now here is some potentially important information that may affect you. I think this is what @smor alluded to but I will reinforce the point.

One little-know aspect of insert triggers (at least in MSSQL) is that when you insert multiple rows in one batch, the trigger is invoked once for the whole operation, and not once per row (emphasis is mine):

because an INSERT trigger can be fired by an INSERT INTO (table_name) SELECT statement, the insertion of many rows may cause a single trigger invocation.

Source: Create DML Triggers to Handle Multiple Rows of Data

So that could be one of your problems too (this kind of problem often goes unnoticed for a very long time...).


Unfortunately your code is hard to read but there are things you can do that would cost little and bring some benefit:

  • use meaningful names for your variable and object names to avoid confusion and improve readability.
  • get rid of dynamic SQL - this alone will make the code a bit easier to follow and understand
  • improve the shape of your code - tabulation is important
  • add error handling ASAP - there could be many errors happening, that you don't notice and compromise the integrity of your data
  • remove unused stuff from your code

For instance in stored procedure ScanIT_spGeraeteEinsatz1 you define a in-memory table:

declare @temptable table (BarcodeName varchar(100))

but apparently you are not actually using it. @temptable2 is not used either or not any more. That only brings confusion.

In a nutshell, now is the time to scale back your code. Think hard about your needs and simplify. Don't try to solve problems when you can avoid them. Improve your code and document yourself, even if it takes time. Bad code is expensive in the long run, because it is hard to maintain and creates more work when you have to address the bugs and deficiencies.

Upvotes: 1

Related Questions