0

I have recently noticed performance issues for the main application I am developing right now (it is a legacy one). A job triggers a stored procedure that computes the security for all the documents and all the application users in the database. The table looks like the following:

Id INT IDENTITY(1, 1) CONSTRAINT PK_DocumentSecurity PRIMARY KEY CLUSTERED,
DocumentId INT CONSTRAINT FK_DocumentSecurity FOREIGN KEY REFERENCES Document,
UserId INT CONSTRAINT FK_DocumentSecurity FOREIGN KEY REFERENCES ApplicationUser,
CanWrite BIT NOT NULL,
+ an index for DocumentId and UserId

This table has about 3.6M records in production, but this keeps growing due to the increasing number of documents and users.

This happens very often (once a minute) and I have noticed using sp_who2 that it sometimes blocks other SPIDs. The profiler also indicates a very large number of reads and CPU, so clearly, this process is very heavy.

Sometimes, on the test environments, the computation seems to take forever, probably due to a very inefficient query plan that happens from time to time (test servers are significantly lower on resources than the production one).

I have checked the code and the current algorithm looks like the following:

  • compute a hash for all the records in the tables involved in the computation
  • if the hash is the same as before, do nothing
  • insert the result of a big and complicated SELECT statement (I will provide details below) to insert into a persistent table having the exact schema as above
  • when done the view used for joining to apply the security is altered to use the brand new computed table

The actual computation looks like the following (actual details are removed due to brevity):

SELECT DISTINCT ISNULL(ds.UserId, 0), ISNULL(ds.DocumentId, 0)
FROM (
    SELECT ... FROM Document ... CROSS JOIN ApplicationUser ...
    UNION
    SELECT ... FROM Document ... CROSS JOIN ApplicationUserRole ...
    UNION
    SELECT ... FROM Document ...
    UNION 
    SELECT DISTINCT ... FROM DocumentDetail ...
    UNION 
) ds
JOIN (
    SELECT ... FROM Document ...
    WHERE (... OR ... OR ... ) AND ...
    UNION
    SELECT ...
) dsp ON dsp.DocumentId = ds.DocumentId AND dsp.UserId = ds.UserId

From my perspective, there are two big problems with this way of computation:

  • maintenance: it is becoming harder and harder to introduce new security rules
  • performance: ORs and DISTINCTs are killing the performance

I have already created a story to refactor this code, but I would like to validate that my reasoning is correct (I have an OOP bias, as I am mainly a .NET developer) before starting working on it:

  • refactor the security rules (to be validated by the business, since they certainly look different than what is already there which also might not be 100% correct for all cases) that they all like the following (precedence matter here):

if role is admin, allow access for all documents if role is sales and
user has read role for document, insert with CanWrite = 0 if role is
sales and user has a write role for document, insert with CanWrite = 1

  • create a temporary table with the target schema

  • create an index for DocumentId and UserId

  • apply each security rule:

    INSERT INTO #buffer (DocumentId, UserId, CanWrite)
    SELECT ...
    FROM Document D ...
      JOIN ApplicationUser U ...
    WHERE NOT EXISTS (SELECT 1 FROM #buffer B WHERE B.DocumentId = D.DocumentId AND B.UserId = U.UserId)
    
  • truncate the target table

  • INSERT INTO DocumentSecurity SELECT * FROM #buffer

The last two steps might need to be optimized if they take too long (maybe replace with the swap the table under the view mechanism already in place).

This should make the code more maintainable and I assume the performance will be better if I remove all the ORs and the DISTINCTs, but I am interested in a DBA's perspective for such an approach. Is it suboptimal?

Note: I cannot have duplicates in the security table because there are currently many places (Entity Framework used in application layer + stored procedures) that simply JOIN with the security view (as opposed to check for existence).

2
  • Could you please upload execution plan on brentozar.com/pastetheplan – Learning_DBAdmin Sep 27 '20 at 11:01
  • @Learning_DBAdmin here you are (some entities are named differently because this computation is done for multiple entities). I am aware that is can be optimized, but I am also very interested in a clear and maintainable code, that's why I asked about rewriting the whole thing. – Alexei Sep 27 '20 at 12:26
2

The amount of data we are talking about here is not nearly enough to be this big of a problem. Odds are, a bit of optimization will do you quite nicely.

So, opening your query plan, as I go along:

  • Fix your WHERE clauses. Your current query forces table scans just about everywhere because you use functions. IsNull(COALESCE(linkedlot.IsPrivate, c.IsPrivate), 0) = 0 should be ((ll.IsPrivate = 0) Or (ll.IsPrivate Is Null And c.IsPrivate = 0) Or (ll.IsPrivate Is Null And c.IsPrivate Is Null)). Side note: are these fields actually nullable? Why? These queries would be MUCH simpler if they were not.

  • Change UNION to UNION ALL. UNION does an implicit DISTINCT, but you're doing one in the main query already. Depending on your data, this may help a lot or hurt a lot.

  • Make sure your temporary tables have indexes/primary keys.

  • If the query actually takes a while, load it into a copy of the table, then MERGE it into the live copy afterwards.

Wait, never mind... very simply, your CROSS JOINs are killing you.

Look through your query plan (or rather, save it and open it up in SQL Server Management Studio), and hover over some of those thick arrows. You're exploding Contract times ApplicationUserRole five times (8 million rows every time), Contract times ApplicationUser twice (6 million rows every time), then slapping all those together (on several levels) to get distinct values (sorting 8-16 million rows every time).

But "explode all possible combinations for every possible way rights can be affected, whether they exist or not, then combine, then sort, then sort again" is not what you want. You want users versus documents, and look up their rights.

So let's do that.

Your query should look like this:

Select
  UserId = U.UserId,
  ContractId = C.ContractId,
  -- If there are explicit denials, MAX should be MIN
  CanRead = Max(Case When <<highest priority condition>>
                     When <<down the list...>>
                     Else 0 End),
  -- If there are explicit denials, MAX should be MIN
  CanWrite = Max(Case When <<highest priority condition>>
                      When <<down the list...>>
                      Else 0 End)
From
  dbo.User As U
  Cross Join dbo.Contract As C
  Left Outer Join (<<Look up first thing...>>)
  Left Outer Join (<<down the list...>>)
Group By
  U.UserId, C.ContractId

So, grabbing a few from your plan:

Select
  UserId = U.UserId,
  ContractId = C.ContractId,
  CanRead = Max(Case When ContractOwner.UserId Is Not Null Then 1
                     When <<down the list...>>
                     Else 0 End),
  CanWrite = Max(Case When ContractOwner.UserId Is Not Null Then 1
                      When <<down the list...>>
                      Else 0 End)
From
  dbo.User As U
  Cross Join dbo.Contract As C
  Left Outer Join dbo.LotSupplier As LotSupplier
    On LotSupplier.ContractId = C.Id
  Left Outer Join dbo.ContractBuyer As ContractBuyer 
    On ContractBuyer.ContractId = C.Id
  Left Outer Join dbo.Lot As LinkedLot 
    On LotSupplier.LotId = LinkedLot.Id
  Left Outer Join dbo.ApplicationUserRole As ContractOwner
    On ContractOwner.UserId = U.UserId
    And ContractOwner.BUid = C.SignatoryBUId
    And ((C.IsPrivate Is Null) Or (C.IsPrivate = Cast(0 As Bit)))  -- NO ISNULL!!
    And ContractOwner.RoleId In (16, 19, 20)
  Left Outer Join dbo.ContractBuyer As ContractBuyer
    On ContractBuyer.ContractId = C.Id
  Left Outer Join dbo.ApplicationUserRole As Buyer
    On ((C.IsPrivate = Cast(1 As Bit)
        Or (LinkedLot.IsPrivate = Cast(1 As Bit))
    And ContractBuyer.BuyerId = Buyer.UserId
    And ...on and on...
  Left Outer Join (<<down the list...>>)
Group By
  U.UserId, C.ContractId

Unless you're running this on a brutally underspecced server, the entire query can be made to run in a few seconds, trust me.

Since you're also obviously working in what I call a many-hats model (a person can be a user, buyer, owner, et cetera, I highly recommend you consider normalizing out a Person (or Party, or whatever tickles your fancy) table.

  • Information would have to be updated in one place only
  • You would not have to do that awful join on e-mail
  • That join would not fail on updating info in only one place :-)
  • You would be much better set up for sub-contracting, multi-owner contracting, contract transfers and all that fun

It looks like you're kind of using ApplicationUser for this purpose, but I think your question alone shows one of the downsides to that.

2

What you are ReComputing ? and how you are Recomputing is not so clear ?

Advice to Avoid DISTINCT,UNION is already given.

I don't know what is the purpose of CROSS JOIN in your query and whether it can be avoided or not.

If CROSS JOIN is producing not a row more not a row less then it will not hurt performance.

Instead of querying FROM Document again and again put the Result in #Temp table.

Nor sure if in your case it is worth putting in #Temp table.

This happens very often (once a minute) and I have noticed using sp_who2 that it sometimes blocks other SPIDs. The profiler also indicates a very large number of reads and CPU, so clearly, this process is very heavy.

One of the reason is Foriegn Key Constraint.This is one of the disadvantage of FK constraint.

When you are already validating DocumentId,Userid from their respective table then why put FK Constraint.

Once you can try disabling FK Constraint.

Or create Trusted FK constraint

1
+100

The only way to efficiently maintain a large cache of relationships like this is to not recompute the whole lot. Unfortunately partial updates, and reliably knowing what to update, can be complex depending on the requirements dictated by your business rules.

compute a hash for all the records in the tables involved

This is your first (and possibly biggest) expense. You are going to scan large chunks of data every time you even check to see if updates are needed. As a first step, instead mark the data as dirty when something relevant is updated. If all updates to those records are mediated through stored procedures you can do this there, if not you may need to use triggers. I would recommend keeping it within the DB (procs/triggers) unless all current and future applications that modify the data use one access library (otherwise you are adding logic that has to be replicated in every routine that updates relevant data and this is a source of bugs and related maintenance & future development headaches).

In the first instance the "dirty" flag could just be a single value that indicates everything needs recomputing. It will at least save you hashing all the data every minute even if you still recreate the whole cross-reference cache every time an update might be needed.

You may already have an audit trail of changes that means you effectively have this ready to use: check the audit trail to see if relevant records have been updated since the cross-reference table was last refreshed, and when updating record what time you started the update procedure for the next check to refer to.

noticed using sp_who2 that it sometimes blocks other SPIDs.

This will be it holding read locks while computing the hash, blocking other processes from writing until it is finished. Or worse it may have escalated those locks to full locks holding back even other processes that are just reading the data. If you record a "data is dirty" marker when things are updated all it will need to do in cases where an update isn't needed is read that flag and decide to do nothing - no big scans and no other locks.

The profiler also indicates a very large number of reads and CPU

That will be the scanning data (reads) and computing the hash (CPU).

the result of a big and complicated SELECT statement ... into a persistent table having the exact schema as above ... the view used ... is altered to use the brand new computed table

Rebuilding a cache table like this in a general access application is not recommended. As a weekly/daily process in a reporting database that isn't constantly updated maybe, but not here. While thinking about it per-entry it often looks more efficient to recreate the whole lot ("The while think takes 20 seconds, if I do it per entity it takes 2000 seconds") if you instead only update the entries for entities that have seen updates you might only need to reassess 0.01% of the entries each time instead of all of them so 0.2s (warning: figures plucked from the ether in the absence of details about your specific cardinalities and timings).

If you update the relevant rows in the cross-reference cache each tie a relevant entity is updated in a relevant way then you should only have a small number of row changes each time not one mass change. This does increase the complexity of any updates but saves the massive "check and update everything" and means that your cross-refence cache is always bang up-to-date instead of being minutes out of date. So if a new document arrives, check to see who should have access and record that. A person changes from a basic role to being an admin, reassess what documents that person can do things too. As with the dirty flag this could be done in one of three places depending on your application(s): triggers, access layer procs, or application code. Of course the important thing here would be to make sure that your checks/updates are as efficient as possible - you don't want to scan everything everytime a person or document is touched.

One important consideration here is how often is an update actually needed? If something relevant changes only a couple of times per day then keeping your current "rebuild everything" method might be far preferable to the extra complexity of partial updates, with the dirty data indicator removing the need for an expensive check every minute, especially if you can be sure that the dirty flag is not set too many times as false positives.

The actual computation

I'll skip that huge bucket of possible details about the business rules being applied here for now, as the above might make it irrelevant. Maybe with dirty data marking removing the scans to check will remove enough activity to make all the difference because the big computation hardly ever need to be run.

You should probably add to your question some details about this:

  • How much IO is currently needed for the scans for the hash check
  • How often an update is actually needed because data has changed
  • How much time/IO/other resource is used by the update when one happens

computer in temporary table, truncate the target table, INSERT INTO DocumentSecurity SELECT * FROM #buffer

Using a temporary table might not be any quicker than a real table: it is still a table, and unless it is small it might still end up on disk (though in tempdb rather than the actual DB, which might be faster depending on your tempdb setup but remember that tempdb is a shared resource if you have other DBs on the same instance).

Also you are still rebuilding the whole target table each time, and the delay might be worse than your current "build alongside, then flip use with a schema update". While building alongside in a new table you compete with other processes for IO but hopefully won't block them entirely with locks, then there is a short time when everything accessing the structure is paused for the schema (view) change. With your suggestion here everything accessing the cross reference table is going to be blocked and queued waiting for it to be rebuild after truncation, i.e. for the duration of all those writes.

If you do try this approach I would instead try to only add/remove/update rows in the target instead of writing every single one every single time. Also if you are using a recovery mode other than basic, log-shipping, etc, you need to consider that all the excess updates will go through the logs and affect your infrastructure that way (actually this could be an argument in favour of temporary tables if you try this method: you keep the initial build out of your main DB's transaction log). Though again, if an update only rarely needs to happen, you might want to keep the inefficiency as a trade-off against complexity.

tl;dr:

  1. Don't scan everything for changes every time. Update the cross-reference as needed, or use dirty data markers to avoid needing the scan and hash everything. If it doesn't end up needing to alter the cross-reference table often then this might be all you need to do to fix the problem (for now).
  2. Don't smash and recreate the whole cross-reference table every time an update is needed. Unless the update actually needs to happen so rarely that you can live with this inefficiency (for the sake of simplicity) once the regular scans to check for needed updates is removed.
  3. Extra point not already mentioned: you may want to keep the old "check all, update all" procedure around. You are then maintaining two bits of code for the same job, but you can periodically, as part of overnight/weekly/other maintenance tasks, rebuild into another table and verify that the current status is correct. You then have a sanity check that will highlight bugs in the main check/update code, and a ready-made rebuild method if something does go wrong or for when the business rules change so everything needs recomputing in one go to apply the new rules to all entities.
2
  • Thank you for the exhaustive answer. Point 1 is certainly possible and we will go this way as all triggers seem to be from a single application layer (using Entity Framework). It is just that we need time to analyze and be very careful with the change since there are no integrative tests. Until then we want to change one step at a time. First optimize the computation and make it more maintainable as security changes epics are already defined. I have also thought of leaving the whole computation during the night (no hash checks) as a sanity check and it is nice to have this confirmation. – Alexei Sep 30 '20 at 10:54
  • Not sure I saw it mentioned in the answer. Is my approach effective for the short-time solution (split in multiple INSERT INTO ... WHERE NOT EXISTS ...)? Before rewriting the whole security, it would be rather easy to use the same procedures, but provide user or document as a parameter to apply the rules for that user / document only, thus being able to change the security computation towards a real-time computation. – Alexei Sep 30 '20 at 10:57

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.