mjhjmtu

I have common clause in most of the procedures like

Select * from TABLE A + Joins where <Conditions>

And 
(
 -- All Broker
 ('True' = (Select AllBrokers from SiteUser where ID = @SiteUserID)) 
 OR 
 ( 
 A.BrokerID in 
 (
 Select BrokerID from SiteUserBroker where SiteUserID 
 = @SiteUserID)
 )
)

Basically, if the user has access to all brokers the whole filter should not be applied else it should get the list of brokers.

I am a bit worried about the performance as this is used in lot of procedures. Data has started reaching over 100,000 records and will grow soon, so can this be better written?

Any ideas are appreciated.

You are correct that it is not a good practice to bundle the permission check in the relational query. The extra predicates can make the optimizer's choice more complicated, and the code is not very readable. It also may cause the optimizer to process all the data from the actual SELECT you are looking for, even if the permission check ends up failing, and returning an empty set.

What if instead, you split it into 2 phases - first check for the permissions, and then execute your query only if permissions check succeeds.
For example:

IF (
 -- All Broker
 ( 'True' = (
 SELECT AllBrokers 
 FROM SiteUser 
 WHERE ID = @SiteUserID
 )
 ) 
 OR 
 ( A.BrokerID IN (
 SELECT BrokerID 
 FROM SiteUserBroker 
 WHERE SiteUserID = @SiteUserID
 )
 )
 )
SELECT * 
FROM TABLE A <+ Joins> 
WHERE <Conditions>
ELSE
SELECT NULL AS ColumnA, NULL AS ColumnB... -- all columns as the * above
WHERE 1=0 -- Forces an empty set

This will keep the same output columns of the set either if permission fail, or succeed, but will skip the actual SELECT processing if it fails. It is also much more readable IMHO.

An even better approach would be to raise an explicit access denied message, for example:

ELSE
THROW <Access Denied Error>

However, this approach may require application code changes to handle the message correctly.

BTW - never use SELECT * in production code, see this thread...

HTH

First I'd suggest you check you do (or will) have an actual problem. Mock up a lot of data representative of some future state, perhaps by duplicating current data ten or twenty times. Then check performance. If it's acceptable (how do you define that?) all's well and you can leave your query alone.

That said, there are other designs that produce your desired outcome. One would be to stop using SiteUser.AllBrokers as a proxy and use the actual data. In other words, create a row in SiteUserBroker for every broker for those users which have access to every broker. The permission check then reduces to a simple inner join.

Select * from TABLE A + Joins 
inner join SiteUserBroker as sub
 on sub.BrokerId = A.BrokerId
 and sub.SiteUserID = @SiteUserID
where <Conditions>

With an index on SiteUserBroker (SiteUserID, BrokerID) access will be fast.

The actual performance compared to your current code will depend on the data distribution. If there are very many users and very many brokers, and most of the users have access to all brokers, this proposal will produce a lot of rows compared to your current solution. In this case inefficient access to a small number of rows (current) may be faster than efficient access to very many rows (proposed). Alternatively, if most users have most, but not all, brokers this proposal will increase the row count in SiteUserBroker only modestly. Access times will be no different to they are currently and the code will likely be faster. Performance will be affected by working set size, IO performance and other things, too. As is so often the case, you must test and see how it looks on your hardware, with your data.

Changes to the Broker list have to be accommodated. If you add a new broker, to which users should it be allocated also so they maintain their "all brokers" status? To this end I'd suggest you retain SiteUser.AllBrokers. Removing a broker, of course, removes it from all users regardless.