views:

102

answers:

3

I am currently developing on an advertising system, which have been running just fine for a while now, apart from recently when our views per day have shot up from about 7k to 328k. Our server cannot take the pressure on this anymore - and knowing that I am not the best SQL guy around (hey, I can make it work, but not always in the best way) I am asking here for some optimization guidelines. I hope that some of you will be able to give rough ideas on how to improve this - I don't specifically need code, just to see the light :).

As it is at the moment, when an advert is supposed to be shown a PHP script is called, which in return calls a stored procedure. This stored procedure does several checks, it tests up against our customer database to see if the person showing the advert (given by a primary key id) is an actual customer under the given locale (our system is running on several languages which are all run as separate sites). Next up is all the advert details fetched out (image location as an url, height and width of the advert) - and lest step calls a separate stored procedure to test if the advert is allowed to be shown (is the campaign expired by either date or number of adverts allowed to show?) and if the customer has access to it (we got 2 access systems running, a blacklist and a whitelist one) and lastly what type of campaign we're running, is the view unique and so forth.

The code consists of a couple of stored procedures that I will post in here.

--- procedure called from PHP

CREATE PROCEDURE [dbo].[ExecView]

    (
    @publisherId bigint,
    @advertId bigint,
    @localeId int,
    @ip varchar(15),
    @ipIsUnique bit,
    @success bit OUTPUT,
    @campaignId bigint OUTPUT,
    @advert varchar(500) OUTPUT,
    @advertWidth int OUTPUT,
    @advertHeight int OUTPUT
    )

AS
BEGIN
    SET NOCOUNT ON;
    DECLARE @unique bit
    DECLARE @approved bit
    DECLARE @publisherEarning money
    DECLARE @advertiserCost money
    DECLARE @originalStatus smallint
    DECLARE @advertUrl varchar(500)
    DECLARE @return int

    SELECT @success = 1, @advert = NULL, @advertHeight = NULL, @advertWidth = NULL


    --- Must be valid publisher, ie exist and actually be a publisher
    IF dbo.IsValidPublisher(@publisherId, @localeId) = 0
     BEGIN
      SELECT @success = 0
      RETURN 0
     END

    --- Must be a valid advert
    EXEC @return = FetchAdvertDetails @advertId, @localeId, @advert OUTPUT, @advertUrl OUTPUT, @advertWidth OUTPUT, @advertHeight OUTPUT

    IF @return = 0
     BEGIN
      SELECT @success = 0
      RETURN 0
     END 

    EXEC CanAddStatToAdvert 2, @advertId, @publisherId, @ip, @ipIsUnique, @success OUTPUT, @unique OUTPUT, @approved OUTPUT, @publisherEarning OUTPUT, @advertiserCost OUTPUT, @originalStatus OUTPUT, @campaignId OUTPUT

    IF @success = 1
     BEGIN 
      INSERT INTO dbo.Stat (AdvertId, [Date], Ip, [Type], PublisherEarning, AdvertiserCost, [Unique], Approved, PublisherCustomerId, OriginalStatus)
      VALUES (@advertId, GETDATE(), @ip, 2, @publisherEarning, @advertiserCost, @unique, @approved, @publisherId, @originalStatus)
     END

END

--- IsValidPublisher

CREATE FUNCTION [dbo].[IsValidPublisher] 
(
    @publisherId bigint,
    @localeId int
)
RETURNS bit
AS
BEGIN
    DECLARE @customerType smallint
    DECLARE @result bit

    SET @customerType = (SELECT [Type] FROM dbo.Customer
    WHERE CustomerId = @publisherId AND Deleted = 0 AND IsApproved = 1 AND IsBlocked = 0 AND LocaleId = @localeId)

    IF @customerType = 2
     SET @result = 1
    ELSE
     SET @result = 0

    RETURN @result 
END

-- Fetch advert details

CREATE PROCEDURE [dbo].[FetchAdvertDetails] 
    (
     @advertId bigint,
     @localeId int,
     @advert varchar(500) OUTPUT,
     @advertUrl varchar(500) OUTPUT,
     @advertWidth int OUTPUT,
     @advertHeight int OUTPUT
    )
AS
BEGIN
    -- SET NOCOUNT ON added to prevent extra result sets from
    -- interfering with SELECT statements.
    SET NOCOUNT ON;

    SELECT @advert = T1.Advert, @advertUrl = T1.TargetUrl, @advertWidth = T1.Width, @advertHeight = T1.Height FROM Advert as T1
    INNER JOIN Campaign AS T2 ON T1.CampaignId = T2.Id
    WHERE T1.Id = @advertId AND T2.LocaleId = @localeId AND T2.Deleted = 0 AND T2.[Status] <> 1

    IF @advert IS NULL
     RETURN 0
    ELSE
     RETURN 1
END

--- CanAddStatToAdvert

CREATE PROCEDURE [dbo].[CanAddStatToAdvert] 
    @type smallint, --- Type of stat to add
    @advertId bigint,
    @publisherId bigint,
    @ip varchar(15),
    @ipIsUnique bit,
    @success bit OUTPUT,
    @unique bit OUTPUT,
    @approved bit OUTPUT,
    @publisherEarning money OUTPUT,
    @advertiserCost money OUTPUT,
    @originalStatus smallint OUTPUT,
    @campaignId bigint OUTPUT
AS
BEGIN
    SET NOCOUNT ON;
    DECLARE @campaignLimit int
    DECLARE @campaignStatus smallint
    DECLARE @advertsLeft int
    DECLARE @campaignType smallint
    DECLARE @campaignModeration smallint
    DECLARE @count int

    SELECT @originalStatus = 0
    SELECT @success = 1
    SELECT @approved = 1
    SELECT @unique = 1

    SELECT @campaignId = CampaignId FROM dbo.Advert
    WHERE Id = @advertId

    IF @campaignId IS NULL
     BEGIN
      SELECT @success = 0
      RETURN
     END

    SELECT @campaignLimit = Limit, @campaignStatus = [Status], @campaignType = [Type], @publisherEarning = PublisherEarning, @advertiserCost = AdvertiserCost, @campaignModeration = ModerationType FROM dbo.Campaign
    WHERE Id = @campaignId


    IF (@type <> 0 AND @type <> 2 AND @type <> @campaignType) OR ((@campaignType = 0 OR @campaignType = 2) AND (@type = 1)) -- if not a click or view type, then type must match the campaign (ie, only able to do leads on lead campaigns, no isales or etc), click and view campaigns however can do leads too
     BEGIN
      SELECT @success = 0
      RETURN
     END

    -- Take advantage of the fact that the variable only gets touched if there is a record,
    -- which is supposed to override the existing one, if there is one
    SELECT @publisherEarning = Earning FROM dbo.MapCampaignPublisherEarning
    WHERE CanpaignId = @campaignId AND PublisherId = @publisherId

    IF @campaignStatus = 1
     BEGIN
      SELECT @success = 0
      RETURN
     END

    IF NOT @campaignLimit IS NULL
     BEGIN
      SELECT @advertsLeft = AdvertsLeft FROM dbo.Campaign WHERE Id = @campaignId
      IF @advertsLeft < 1
       BEGIN
        SELECT @success = 0
        RETURN
       END
     END

    IF @campaignModeration = 0 -- blacklist
     BEGIN
      SELECT @count = COUNT([Status]) FROM dbo.MapCampaignModeration WHERE CampaignId = @campaignId AND PublisherId = @publisherId AND [Status] = 3
      IF @count > 0
       BEGIN
        SELECT @success = 0
        RETURN
       END
     END
    ELSE -- whitelist
     BEGIN
      SELECT @count = COUNT([Status]) FROM dbo.MapCampaignModeration WHERE CampaignId = @campaignId AND PublisherId = @publisherId AND [Status] = 2
      IF @count < 1
       BEGIN
        SELECT @success = 0
        RETURN
       END
     END

    IF @ipIsUnique = 1
     BEGIN
      SELECT @unique = 1
     END
    ELSE
     BEGIN
      IF (SELECT COUNT(T1.Id) FROM dbo.Stat AS T1
          INNER JOIN dbo.IQ_Advert AS T2
          ON T1.AdvertId = T2.Id
          WHERE T2.CampaignId = @campaignId
          AND T1.[Type] = @type
          AND T1.[Unique] = 1
          AND T1.PublisherCustomerId = @publisherId
          AND T1.Ip = @ip
          AND DATEADD(SECOND, 86400, T1.[Date]) > GETDATE()
        ) = 0 
       SELECT @unique = 1
       ELSE
       BEGIN
       SELECT @unique = 0, @originalStatus = 1 -- not unique, and set status to be ip conflict
       END

     END

    IF @unique = 0 AND @type <> 0 AND @type <> 2
    BEGIN
     SELECT @unique = 1, @approved = 0
    END

    IF @originalStatus = 0
     SELECT @originalStatus = 5

    IF @approved = 0 OR @type <> @campaignType
    BEGIN
     SELECT @publisherEarning = 0, @advertiserCost = 0
    END

END

I am thinking this needs more than just a couple of indexes thrown in to help it, but rather a total rethinking of how to handle it. I have been heard that running this as a batch would help, but I am not sure how to get this implemented, and really not sure if i can implement it in a such way where I keep all these nice checks before the actual insert or if I have to give up on some of this.

Anyhow, all help would be appreciated, if you need any of the table layouts, let me know :).

Thanks for taking the time to look at it :)

+1  A: 

Make sure to reference tables with the ownership prefix. So instead of:

INNER JOIN Campaign AS T2 ON T1.CampaignId = T2.Id

Use

INNER JOIN dbo.Campaign AS T2 ON T1.CampaignId = T2.Id

That will allow the database to cache the execution plan.

Another possibility is to disable database locking, which has data integrity risks, but can significantly increase performance:

INNER JOIN dbo.Campaign AS T2 (nolock) ON T1.CampaignId = T2.Id

Run a sample query in SQL Analyzer with "Show Execution Plan" turned on. This might give you a hint as to the slowest part of the query.

Andomar
Thank you, that's a great start, what about possibly running stuff in batch, got any ideas on that?
kastermester
Also what sort of data integrity risks are we talking about? Just that data could be manipulated while reading it or are we talking about anything more serious?
kastermester
Yeah you might read the wrong data, but nothing that will corrupt the physical database ofc. Dunno what you mean by batch. One thing that might help is cache recent query results, for example store the last 1000 ExecView results in a caching table, and use them if an identical next query arrives?
Andomar
I meant to try and do all the inserts into the stat table (which is really what it is all about, apart from ofc. showing the advert) in 1 big query, instead of doing many small.Your idea sounds good too though, and I think we will do something like this more or less as well, of course we have issues with things that keeps changing, such as IP addresses and the likes. Thank you anyways :)
kastermester
A: 

it seems like FetchAdvertDetails hit the same tables as the start of CanAddStatToAdvert (Advert and Campaign). If possible, I'd try to eliminate FetchAdvertDetails and roll its logic into CanAddStatToAdvert, so you don't have the hit Advert and Campaign the extra times.

KM
A: 

Get rid of most of the SQL.

This stored procedure does several checks, it tests up against our customer database to see if the person showing the advert (given by a primary key id) is an actual customer under the given locale (our system is running on several languages which are all run as separate sites). Next up is all the advert details fetched out (image location as an url, height and width of the advert) - and lest step calls a separate stored procedure to test if the advert is allowed to be shown (is the campaign expired by either date or number of adverts allowed to show?) and if the customer has access to it (we got 2 access systems running, a blacklist and a whitelist one) and lastly what type of campaign we're running, is the view unique and so forth.

Most of this should not be done in the database for every request. In particular:

  • Customer and local can be stored in memory. Expire them after 5 minutes or so, but do not ask for this info on every repetitive request.
  • Advert details can also be stored. Every advert will have a "key" to identify it (Number)?. Ust a dictionary / hashtable in memory.

Eliminate as many SQL Parts as you can. Dumping repetitive work on the SQL Database is a typical mistake.

TomTom