8

I'm caught in a debate at work, and I need some advice on possible Pitfalls I could be overlooking.

Imagine a scenario where a Trigger is used to copy Deleted Records to an Audit Table. The Trigger uses SELECT *. Everyone points and shouts and tells us how bad this is.

However, if a modification is made to the Structure of the Main Table, and the Audit Table is overlooked then the Trigger will generate an error letting people know the Audit table also needs modification.

The error will be caught during testing on our DEV servers. But we need to ensure Production Matches DEV, so we allow SELECT * in Production Systems (Triggers only).

So my Question is: I'm being pushed to remove the SELECT *, but I'm not sure how else to ensure we are automatically capturing Development Errors of this nature, any ideas or is this best practice?

I've put together an example below:

--Create Test Table
CREATE TABLE dbo.Test(ID INT IDENTITY(1,1), Person VARCHAR(255))
--Create Test Audit Table
CREATE TABLE dbo.TestAudit(AuditID INT IDENTITY(1,1),ID INT, Person VARCHAR(255))

--Create Trigger on Test
CREATE TRIGGER [dbo].[trTestDelete] ON [dbo].[Test] AFTER DELETE
NOT FOR REPLICATION
AS
BEGIN
    SET NOCOUNT ON;
    INSERT  dbo.TestAudit([ID], [Person])
    SELECT  *
    FROM    deleted
END

--Insert Test Data into Test
INSERT INTO dbo.Test VALUES
('Scooby')
,('Fred')
,('Shaggy')

--Perform a delete
DELETE dbo.Test WHERE Person = 'Scooby'

UPDATE (rephrase question):

I'm a DBA and need to ensure Developers don't provide poorly thought-out Deployment scripts by contributing to our Best Practice Documentation. SELECT * causes an error in DEV when the Developer overlooks the Audit Table (this is a safety net) so the error is caught early in the development process. But somewhere in the SQL Constitution - 2nd amendment it reads "Thou shalt not use SELECT *" . So now there is a push to get rid of the Safety Net.

How would you replace the Safety Net, or should I consider this to be best practice for Triggers?

UPDATE 2: (solution)

Thankyou for all of your input, I'm not sure if I have a clear answer because this appears to be a very Grey subject. But collectively you've provided discussion points that can help our developers move forward with defining their Best Practice.

Thanks Daevin for your contribution, your answer provides the groundwork for some test mechanisms that our Developers can implement. +1

Thanks CM_Dayton, your suggestions contributing to best practice can be beneficial to anyone who is devloping Audit Triggers. +1

Big Thanks to ypercube, you've raised plenty of thought around the issues concerning tables undergoing different forms of definition changes. +1

In conclusion:

Is Select * ok in a tigger? Yes, it's a Grey area, don't blindly follow the "Select * is Bad" ideology".

Am I asking for Trouble? Yes, we do more than just add new columns to tables.

  • you answer yourself in the question. select * will break if source table is changed. To make sure dev and prod are the same, use some form of source control. – Bob Klimes Feb 1 '17 at 14:56
  • slightly broader question, how frequently do you delete records and how many as a total percentage of the table? an alternative to triggers would be to have a bit flag that marks rows as deleted and a agent job that runs on a schedule to move them to a log table. You could build into the agent job checks to see if the table schema matches and the job will simply fail if there's a problem with that step until it's fixed. – Tanner Feb 1 '17 at 15:01
  • I usually agree with the SELECT * being lazy, but since you have a legitimate reason to use it it's more grey than black-and-white. What you should try doing is something like this, but adjust it to not only have the same column count, but that the column names and data types are the same (since someone could change data types and still cause issues in the db not normally caught with your SELECT * 'safety net'. – Daevin Feb 1 '17 at 19:20
  • 3
    I like the idea of using SELECT * as a safety net but it won't catch all cases. For example, if you drop a column and add it again. This will change the order of the columns and (unless all columns are of the same type) the inserts into the audit table will fail or result in loss of data due to the implicit type conversions. – ypercubeᵀᴹ Feb 1 '17 at 21:03
  • 2
    I also wonder how your audit design will work out when a column is dropped from a table. Do you also drop the column from the audit table (and lose all previous audit data)? – ypercubeᵀᴹ Feb 1 '17 at 21:05
10

Typically, it is considered "lazy" programming.

Given that you're specifically inserting two values into your TestAudit table here, I'd be careful to make sure your select is also getting exactly two values. Because if, for some reason, that Test table has or ever gets a third column, this trigger will fail.

Not directly related to your question but if you're setting up an audit table, I'd also add some additional columns to your TestAudit table to...

  • track the action you are auditing (delete in this case, vs inserts or updates)
  • date/time column to track when the audit event occurred
  • user ID column to track who carried out the action you are auditing.

So that results in a query like:

INSERT dbo.TestAudit([ID], [Person], [AuditAction], [ChangedOn], [ChangedBy])
SELECT [ID], [Person], 
   'Delete', -- or a 'D' or a numeric lookup to an audit actions table...
   GetDate(), -- or SYSDATETIME() for greater precision
   SYSTEM_USER -- or some other value for WHO made the deletion
FROM deleted

That way you're getting the exact columns you need and you're auditing what/when/why/who the audit event is about.

  • "user ID" This one is tricky with auditing. Typically, database accounts don't correspond to actual users. Much more often, they correspond to a single web application or other kind of component, with a single set of credentials used by that component. (And sometimes, the components will also share credentials.) So the database credentials are pretty useless as an identifier of who did what, unless you're just interested in what component did it. But passing down application data that identifies the "who" isn't exactly easy with a trigger function, as far as I know. – jpmc26 Feb 1 '17 at 23:54
  • see update to question. – pacreely Feb 3 '17 at 19:59
  • Another issue that could come up with SELECT * in general (though probably not in your example) is that if the underlying table's columns aren't in the same order as your insert columns the insert will fail. – CaM Feb 3 '17 at 20:05
3

I commented this on your question, but I figured I'd try to actually present a code solution.

I usually agree with SELECT * being lazy, but since you have a legitimate reason to use it it's more grey than black-and-white.

What you should (in my opinion) try doing is something like this, but adjust it to ensure the column names and data types are the same (since someone could change data types and still cause issues in the db not normally caught with your SELECT * 'safety net'.

You could even create a function that will let you quickly check if the Audit version of the table matches the non-audit version:

-- The lengths are, I believe, max values for the corresponding db objects. If I'm wrong, someone please correct me
CREATE FUNCTION TableMappingComparer(
    @TableCatalog VARCHAR(85) = NULL,
    @TableSchema VARCHAR(32) = NULL,
    @TableName VARCHAR(128) = NULL) RETURNS BIT
AS
BEGIN
    DECLARE @ReturnValue BIT = NULL;
    DECLARE @VaryingColumns INT = NULL;

    IF (@TableCatalog IS NOT NULL
            AND @TableSchema IS NOT NULL
            AND @TableName IS NOT NULL)
        SELECT @VaryingColumns = COUNT(COLUMN_NAME)
            FROM (SELECT COLUMN_NAME,
                        DATA_TYPE -- Add more columns that you want to ensure are identical
                    FROM INFORMATION_SCHEMA.COLUMNS
                    WHERE TABLE_CATALOG = @TableCatalog
                        AND TABLE_SCHEMA = @TableSchema
                        AND TABLE_NAME = @TableName
                EXCEPT
                    SELECT COLUMN_NAME,
                            DATA_TYPE -- Add more columns that you want to ensure are identical
                        FROM INFORMATION_SCHEMA.COLUMNS
                        WHERE (TABLE_CATALOG = @TableCatalog
                            AND TABLE_SCHEMA = @TableSchema
                            AND TABLE_NAME = @TableName + 'Audit')
                            AND (COLUMN_NAME != 'exclude your audit table specific columns')) adt;
    IF @VaryingColumns = 0
        SET @ReturnValue = 1
    ELSE IF @VaryingColumns > 0
        SET @ReturnValue = 0

    RETURN @ReturnValue;
END;

The SELECT ... EXCEPT SELECT ...Audit will show you what columns in the table are not in the Audit table. You could even change the function to return the name of columns that are not the same instead of just whether they map or not, or even raise an exception.

You can then run this before moving from DEV to PRODUCTION servers for each table in the db, using a cursor over:

SELECT TABLE_NAME
    FROM INFORMATION_SCHEMA.TABLES
    WHERE NOT (TABLE_NAME LIKE '%Audit')
  • 1
    see update to question – pacreely Feb 3 '17 at 19:59
  • Glad I could help. Credit to you for reading all the answers and bringing them back to your team for suggestions; adaptability and willingness to improve are the ways tech departments keep companies running, and running smoothly! :D – Daevin Feb 3 '17 at 21:11
0

The statement that will cue the trigger will fail and the trigger will fail. It would be better practice to document the trigger and audit trail so you know to modify the query to add the columns instead of specify the *.

At the very least you should modify the trigger so it can fail gracefully while logging errors to a table and perhaps put an alert on the table the trigger is logging the errors to.

This also brings to mind, you can put a trigger or alert when someone alters the table and adds more columns or removes columns, to notify you to append the trigger.

Performance wise I believe * does not change anything, it just increases the chances of failures down the road when things change and also can cause network latency when you pull more information across the network when you need. There is a time and place for *, but I feel as described above you have better solutions and tools to try instead.

0

If your either your original or audit table structures change at all, you're guaranteeing that you'll run into an issue with your select *.

INSERT INTO [AuditTable]
(Col1,Col2,Col3)
SELECT * 
FROM [OrigTable] or [deleted];

If either changes, the trigger will error.

You could do:

INSERT INTO [AuditTable]
SELECT * 
FROM [OrigTable];

But as CM_Dayton says, that's lazy programming and opens the door for other inconsistencies. For this scenario to work, you'd have to make absolutely sure that you're updating the structure of both tables at the same time.

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.