+ Start a Discussion
ministe2003ministe2003 

Bulk Trigger and too many soql queries

Hi all

I was having issues with too many SOQL queries in a trigger.  I'm trying to mass update opportunities which I think the limit of is 19, so I changed the code to work with a Batchable class.  Then I got too many concurrent batches errors.  So in my trigger, I query the AsyncApexJob object and only set batches to run when the queue is clear.

 

Now I'm back to having too many SOQL queries because I'm querying AsyncApexJob too often!  AAGH!!

 

Is there a way around this?

 

Regards

 

Steven

Best Answer chosen by Admin (Salesforce Developers) 
ministe2003ministe2003

Managed to sort it, hurrah!! Is there a more fulfilling feeling than sorting something that's taken foreveeeeer??

 

Here's my finished trigger, thanks for all your help.  I'll mark this as the solution but really CaptainObvious' first post on Page 1 got me going, I wouldnt have got this without that post!

 

The final issue I had to resolve was the fact I wasnt updating the live opportunity but rather the one in a map, so I scrapped the map and made a list of opps in the Trigger loop as I was going along, then after gathering partner/account data, looped through the opps and made the match.  One less SOQL query and one less embedded loop.

 

 

trigger SetUKReseller on Opportunity (before update) {
// Need to pull off the primary partners on UK Opps for a report so this field will populate
// Reseller__c with the primary partner if there is one, or if not populate it with the account name.
// This field is inivisible to the UK and these values arent stored in the picklist.

    //Create a set of Opportunity and Account Ids
    //These will only be populated with 'UK Opps'
    Set<Id> OppIDs = new Set<Id>();
    Set<Id> AccIDs = new Set<Id>();
    List<Opportunity> lOpps = new List<Opportunity>();
   
    for (Opportunity Opp :Trigger.New) {
        //only take place if type is UK Opportunity
        if (Opp.RecordTypeID == '012300000004uS3AAI') {
            //Add this opportunity to the set
            if (!OppIDs.contains(Opp.Id)) {
                OppIDs.add(Opp.Id);
            }
            //Add the Account ID to the set
            if (!AccIDs.contains(Opp.AccountId)) {
                AccIDs.add(Opp.AccountId);
            }
            lOpps.add(Opp);
        }
    }

    //Continue only if there are opps in the set
    if(OppIDs.size() > 0) {
        //put information into maps
        //Retrieve 'Account' fields:
        Map<ID,Account> AFields = new Map<ID,Account>([SELECT Id, Name FROM Account WHERE Id in:AccIDs]);
       
        //Retrieve 'Partner' fields:
        Map<ID,Partner> PFields = new Map<ID,Partner>([SELECT Id, AccountToId, AccountTo.Name, OpportunityID
                                                        FROM Partner
                                                        WHERE IsPrimary = TRUE AND OpportunityID in:OppIDs]);

        //Now that we have all information we'll need,
        //we can finally loop through each opportunity
        for(integer i = 0; i < lOpps.size();i++){
            Boolean foundPartner = false;
            //First see if there's a Partner
            for(Partner p : PFields.Values()) {
                if (p.OpportunityID==lOpps[i].id) {
                    lOpps[i].Reseller__c = PFields.get(p.Id).AccountTo.Name;
                    foundPartner = true;
                    break; //exit from the loop if a partner was found
                }
            }
            //If a partner was not found, use the Account Name instead
            if (foundPartner == false){
                lOpps[i].Reseller__c = AFields.get(lOpps[i].AccountId).Name;
            }
        }
    }
}

Message Edited by ministe2003 on 03-31-2010 03:34 PM
Message Edited by ministe2003 on 03-31-2010 03:37 PM

All Answers

Shailesh DeshpandeShailesh Deshpande
can you post your code?
ministe2003ministe2003

Sure can, was going to but didnt know if you were allowed to.  Assumed you could but I know some boards which are funny about it:

 

Trigger:

 

trigger SetUKReseller on Opportunity (before update) {
// Need to pull off the primary partners on UK Opps for a report so this field will populate
// Reseller__c with the primary partner if there is one, or if not populate it with the account name.
// This field is inivisible to the UK and these values arent stored in the picklist.

    private Partner ptnr;

    for (Opportunity Opp :Trigger.New) {
        // first check the record type, Only apply to UK Opp
        if (Opp.RecordTypeID == '012300000004uS3')
        {
            SetUKReseller_Batch batch = new SetUKReseller_Batch(Opp.ID);
            //if job not running, queue a new job     
            Integer runningJobs = [SELECT count() FROM AsyncApexJob WHERE (status = 'Queued' OR status = 'Processing') AND ApexClass.Name = 'SetUKReseller_Batch'];
            if(runningJobs < 1) id batchinstanceid = database.executeBatch(batch, 19);
        }
    }
}

 

 

 

Class:

 

 

global class SetUKReseller_Batch implements Database.Batchable<sObject>{
//Allows SetUKReseller to update opportunities without hitting SOQL limit

    private String query;
    private String OppID;
    private Partner ptnr;
   
    global SetUKReseller_Batch(String OppID){
        this.OppID = OppID;
    }

       global Database.QueryLocator start(Database.BatchableContext BC){
           query = 'SELECT Id,Reseller__c FROM Account WHERE ID = :'+OppID+' LIMIT 1';
           return Database.getQueryLocator(query);
       }
      
       global void execute(Database.BatchableContext BC, List<Sobject> scope){
        for(sobject sOpp : scope){
                try{
                     ptnr = [SELECT Id,AccountToID FROM Partner WHERE IsPrimary = TRUE AND OpportunityID = :OppID LIMIT 1];
                }catch(Exception e){
                // no need to do anything, just means no primary partner exists.
                // this is dealt with below.
                }
                if(ptnr != NULL){
                    Account acnt = [SELECT Id, Name FROM Account WHERE Id = :ptnr.AccountToID];
                    sOpp.put('Reseller__c',acnt.Name);
                }else{
                    ID accID = (ID)(sOpp.get('AccountID'));
                    Account acnt = [SELECT Id, Name FROM Account WHERE Id = :accID];
                    sOpp.put('Reseller__c',acnt.Name);
                }
        }
       }

       global void finish(Database.BatchableContext BC){}
}

 

 

Message Edited by ministe2003 on 03-30-2010 01:06 PM
Shailesh DeshpandeShailesh Deshpande

you get the error because you are querying the data inside the for loop....as soon as the counter reaches 20 it throws the error... so avoid the query inside the for loop....

 

ministe2003ministe2003

yes im querying AsyncApexJob inside the trigger loop but surely I have to?  Otherwise how will the count value get updated for my test?

 

Thanks

Shailesh DeshpandeShailesh Deshpande
cant you do it outside the loop ? im sure thats possible... and infact you dont need batch apex as well...
ministe2003ministe2003
What I mean is if the trigger runs en mass, if i dont run the query on AsyncApexJob within the loop then the value wont update with every call, so I wont know how many batches are queued.  Perhaps we're talking slightly at cross purposes, how would you achieve this to avoid this error?
CaptainObviousCaptainObvious

Is this a typo?

query = 'SELECT Id,Reseller__c FROM Account WHERE ID = :'+OppID+' LIMIT 1';

perhaps you meant FROM Opportunity?

 

Anyway, as Shailesh mentioned, you can run your queries outside of loops and you can implement this without batch apex.

The following is not tested (may need some tweaking), but I hope it illustrates the idea:

 

trigger SetUKReseller on Opportunity (before update) { // Need to pull off the primary partners on UK Opps for a report so this field will populate // Reseller__c with the primary partner if there is one, or if not populate it with the account name. // This field is inivisible to the UK and these values arent stored in the picklist. //Create a set of Opportunity and Account Ids //These will only be populated with 'UK Opps' Set<Id> OppIDs = new Set<Id>(); Set<Id> AccIDs = new Set<Id>(); for (Opportunity Opp :Trigger.New) { /* You may want to add a condition where if the reseller has already been set, * dont attempt to re-set it. Otherwise, this trigger will run every time you * edit the opportunity. * Also, you should probably retrieve the recordtypeid dynamically rather than * hardcoding it. */ if (Opp.RecordTypeID == '012300000004uS3') { //Add this opportunity to the set if (!OppIDs.contains(Opp.Id)) { OppIDs.add(Opp.Id); } //Add the Account ID to the set if (!AccIDs.contains(Opp.AccountId)) { AccIDs.add(Opp.AccountId); } } } //Continue only if there are opps in the set if(OppIDs.size() > 0) { /* Retrieve information you may need into maps * You can then iterate through the maps without additional queries * (Notice how this is done outside any loop) */ //Retrieve 'Account' fields: Map<ID,Account> AFields = new Map<ID,Account>([ SELECT Id, Name FROM Account WHERE Id in:AccIDs]); //Retrieve 'Partner' fields: Map<ID,Partner> PFields = new Map<ID,Partner>([ SELECT Id, AccountToId, AccountTo.Name, OpportunityID FROM Partner WHERE IsPrimary = TRUE AND OpportunityID in:OppIDs]); //Retrieve 'Opportunity' fields: Map<ID,Opportunity> OFields = new Map<ID,Opportunity>([ SELECT Id, Reseller__c, AccountId FROM Opportunity WHERE Id in :OppIDs]); //Now that you have all information you'll need, you can finally loop //through each opportunity: for(Opportunity opp: OFields.values()){ Integer foundPartner=0; //First see if there's a Partner for(Partner p : PFields.Values()) { //additional logic may be needed here.... if (p.OpportunityID==opp.id) { opp.reseller__c = PFields.get(p.Id).AccountTo.Name; foundPartner++; break; //exit from the loop if a partner was found } } //If a partner was not found, use the Account Name instead if (foundPartner==0){ opp.reseller__c = AFields.get(opp.AccountId).Name; } } } }

Hope that helps!

ministe2003ministe2003

Thanks for that, looks good!  I've only been using SF since December and only really been developing in it for a couple of months and I've had a bit of difficulty understanding Maps but now you've shown me a practical use for them!

I'll start changing my trigger to look like your example now, thanks!

 

I take it from the example that in a for (Opportunity Opp :Trigger.New) {} loop, it runs every opportunity which is to be updated, all at once before it comes out of the loop?

 

 

Also, how do you put code into those boxes?  I tried [code][/code] as well as <code></code> but neither worked in mine.

 

 

Thanks again!

Message Edited by ministe2003 on 03-31-2010 10:31 AM
ministe2003ministe2003

Hi all, having other issues now.  Because I'm coming out of the loop in order to do the rest of the work, i need to go back into the loop to update the opportunity.  However this means having to save a link between the opportunity and reseller string to use once im back inside the loop.  But now i'm getting "too many script statement" errors when updating a bunch of opps with the dataloader, can anyone see a way of sorting that?

 

trigger SetUKReseller on Opportunity (before update) {
// Need to pull off the primary partners on UK Opps for a report so this field will populate
// Reseller__c with the primary partner if there is one, or if not populate it with the account name.
// This field is inivisible to the UK and these values arent stored in the picklist.

    //Create a set of Opportunity and Account Ids
    //These will only be populated with 'UK Opps'
    Set<Id> OppIDs = new Set<Id>();
    Set<Id> AccIDs = new Set<Id>();
    Map<String,String> OppToReseller = new Map<String,String>();

    for (Opportunity Opp :Trigger.New) {
        //only take place if type is UK Opportunity
        if (Opp.RecordTypeID == '012300000004uS3AAI') {
            //Add this opportunity to the set
            if (!OppIDs.contains(Opp.Id)) {
                system.debug('STE - Opp added to set');
                OppIDs.add(Opp.Id);
            }
            //Add the Account ID to the set
            if (!AccIDs.contains(Opp.AccountId)) {
                system.debug('STE - Acc added to set');
                AccIDs.add(Opp.AccountId);
            }
        }
    }

    //Continue only if there are opps in the set
    if(OppIDs.size() > 0) {
        //put information into maps
        //Retrieve 'Account' fields:
        Map<ID,Account> AFields = new Map<ID,Account>([SELECT Id, Name FROM Account WHERE Id in:AccIDs]);
        system.debug('STE - Acc fields retrieved');
       
        //Retrieve 'Partner' fields:
        Map<ID,Partner> PFields = new Map<ID,Partner>([SELECT Id, AccountToId, AccountTo.Name, OpportunityID
                                                        FROM Partner
                                                        WHERE IsPrimary = TRUE AND OpportunityID in:OppIDs]);
        system.debug('STE - Partner fields retrieved');
       
        //Retrieve 'Opportunity' fields:
        Map<ID,Opportunity> OFields = new Map<ID,Opportunity>([SELECT Id, Reseller__c, AccountId
                                                                FROM Opportunity WHERE Id in :OppIDs]);       
        system.debug('STE - Opp fields retrieved');

        //Now that we have all information we'll need,
        //we can finally loop through each opportunity
       
        for(Opportunity oppToTest: OFields.values()){
            String ResellerField = '';
            Boolean foundPartner = false;
            //First see if there's a Partner
            for(Partner p : PFields.Values()) {
                if (p.OpportunityID==oppToTest.id) {
                    ResellerField = PFields.get(p.Id).AccountTo.Name;
                    foundPartner = true;
                    system.debug('STE - partner found');
                    break; //exit from the loop if a partner was found
                }
            }
            //If a partner was not found, use the Account Name instead
            if (foundPartner == false){
                system.debug('STE - partner not found');
                ResellerField = AFields.get(oppToTest.AccountId).Name;
            }
            OppToReseller.put(oppToTest.Id, ResellerField);
        }
    }
   
    for (Opportunity Opp :Trigger.New) {
        //iterate through Opp Ids to find match
        for(String s : OppToReseller.keySet()){
            //when we find a matching opp, set the reseller from the value
            if(s == Opp.Id){
                Opp.Reseller__c = OppToReseller.get(s);
            }
        }
    }
}

ministe2003ministe2003
any help useful, im at a dead end!
CaptainObviousCaptainObvious

Instead of

OppToReseller.put(oppToTest.Id, ResellerField);

try

 

oppToTest.reseller__c = ResellerField;

Because your trigger is "before update", this should be all you need to set the field.

 

When using this extra loop at the end

 

for (Opportunity Opp :Trigger.New) { ... }

 

you're actually iterating over every single opportunity, not just the 'UK Opps'.

ministe2003ministe2003
Thanks for the reply but as I tried to explain that doesnt actually update the opportunity, presumably because I'm actually applying that value to the Opportunity in the map not the one in the database :(
CaptainObviousCaptainObvious
Are you sure you're updating existing opportunities and not creating (inserting) new ones? New opportunities would not be affected by a before update trigger.
ministe2003ministe2003

No I'm definitely updating.  What I've done now is made a list of Opportunities during the first trigger loop and when I get the reseller values I'm applying the values to those opps rather than those in the Maps, which seems to work.

However once again when doing a bulk update im still getting too many script errors :(  Didnt realise something so simple would be so hard!

 

trigger SetUKReseller on Opportunity (before update) {
// Need to pull off the primary partners on UK Opps for a report so this field will populate
// Reseller__c with the primary partner if there is one, or if not populate it with the account name.
// This field is inivisible to the UK and these values arent stored in the picklist.

    //Create a set of Opportunity and Account Ids
    //These will only be populated with 'UK Opps'
    Set<Id> OppIDs = new Set<Id>();
    Set<Id> AccIDs = new Set<Id>();
    Map<String,String> OppToReseller = new Map<String,String>();
    List<Opportunity> lOpps = new List<Opportunity>();
   
    for (Opportunity Opp :Trigger.New) {
        //only take place if type is UK Opportunity
        if (Opp.RecordTypeID == '012300000004uS3AAI') {
            //Add this opportunity to the set
            if (!OppIDs.contains(Opp.Id)) {
                system.debug('STE - Opp added to set');
                OppIDs.add(Opp.Id);
            }
            //Add the Account ID to the set
            if (!AccIDs.contains(Opp.AccountId)) {
                system.debug('STE - Acc added to set');
                AccIDs.add(Opp.AccountId);
            }
            lOpps.add(Opp);
        }
    }

    //Continue only if there are opps in the set
    if(OppIDs.size() > 0) {
        //put information into maps
        //Retrieve 'Account' fields:
        Map<ID,Account> AFields = new Map<ID,Account>([SELECT Id, Name FROM Account WHERE Id in:AccIDs]);
        system.debug('STE - Acc fields retrieved');
       
        //Retrieve 'Partner' fields:
        Map<ID,Partner> PFields = new Map<ID,Partner>([SELECT Id, AccountToId, AccountTo.Name, OpportunityID
                                                        FROM Partner
                                                        WHERE IsPrimary = TRUE AND OpportunityID in:OppIDs]);
        system.debug('STE - Partner fields retrieved');
       
        //Retrieve 'Opportunity' fields:
        Map<ID,Opportunity> OFields = new Map<ID,Opportunity>([SELECT Id, Reseller__c, AccountId
                                                                FROM Opportunity WHERE Id in :OppIDs]);       
        system.debug('STE - Opp fields retrieved');

        //Now that we have all information we'll need,
        //we can finally loop through each opportunity
       
        for(Opportunity oppToTest: OFields.values()){
            String ResellerField = '';
            Boolean foundPartner = false;
            //First see if there's a Partner
            for(Partner p : PFields.Values()) {
                if (p.OpportunityID==oppToTest.id) {
                    ResellerField = PFields.get(p.Id).AccountTo.Name;
                    foundPartner = true;
                    system.debug('STE - partner found');
                    break; //exit from the loop if a partner was found
                }
            }
            //If a partner was not found, use the Account Name instead
            if (foundPartner == false){
                system.debug('STE - partner not found');
                ResellerField = AFields.get(oppToTest.AccountId).Name;
            }
            for(integer i = 0; i < lOpps.size();i++){
                if(lOpps[i].Id == oppToTest.Id){
                    lOpps[i].Reseller__c = ResellerField;
                }
            }
        }
    }

}

Message Edited by ministe2003 on 03-31-2010 02:58 PM
ministe2003ministe2003

Managed to sort it, hurrah!! Is there a more fulfilling feeling than sorting something that's taken foreveeeeer??

 

Here's my finished trigger, thanks for all your help.  I'll mark this as the solution but really CaptainObvious' first post on Page 1 got me going, I wouldnt have got this without that post!

 

The final issue I had to resolve was the fact I wasnt updating the live opportunity but rather the one in a map, so I scrapped the map and made a list of opps in the Trigger loop as I was going along, then after gathering partner/account data, looped through the opps and made the match.  One less SOQL query and one less embedded loop.

 

 

trigger SetUKReseller on Opportunity (before update) {
// Need to pull off the primary partners on UK Opps for a report so this field will populate
// Reseller__c with the primary partner if there is one, or if not populate it with the account name.
// This field is inivisible to the UK and these values arent stored in the picklist.

    //Create a set of Opportunity and Account Ids
    //These will only be populated with 'UK Opps'
    Set<Id> OppIDs = new Set<Id>();
    Set<Id> AccIDs = new Set<Id>();
    List<Opportunity> lOpps = new List<Opportunity>();
   
    for (Opportunity Opp :Trigger.New) {
        //only take place if type is UK Opportunity
        if (Opp.RecordTypeID == '012300000004uS3AAI') {
            //Add this opportunity to the set
            if (!OppIDs.contains(Opp.Id)) {
                OppIDs.add(Opp.Id);
            }
            //Add the Account ID to the set
            if (!AccIDs.contains(Opp.AccountId)) {
                AccIDs.add(Opp.AccountId);
            }
            lOpps.add(Opp);
        }
    }

    //Continue only if there are opps in the set
    if(OppIDs.size() > 0) {
        //put information into maps
        //Retrieve 'Account' fields:
        Map<ID,Account> AFields = new Map<ID,Account>([SELECT Id, Name FROM Account WHERE Id in:AccIDs]);
       
        //Retrieve 'Partner' fields:
        Map<ID,Partner> PFields = new Map<ID,Partner>([SELECT Id, AccountToId, AccountTo.Name, OpportunityID
                                                        FROM Partner
                                                        WHERE IsPrimary = TRUE AND OpportunityID in:OppIDs]);

        //Now that we have all information we'll need,
        //we can finally loop through each opportunity
        for(integer i = 0; i < lOpps.size();i++){
            Boolean foundPartner = false;
            //First see if there's a Partner
            for(Partner p : PFields.Values()) {
                if (p.OpportunityID==lOpps[i].id) {
                    lOpps[i].Reseller__c = PFields.get(p.Id).AccountTo.Name;
                    foundPartner = true;
                    break; //exit from the loop if a partner was found
                }
            }
            //If a partner was not found, use the Account Name instead
            if (foundPartner == false){
                lOpps[i].Reseller__c = AFields.get(lOpps[i].AccountId).Name;
            }
        }
    }
}

Message Edited by ministe2003 on 03-31-2010 03:34 PM
Message Edited by ministe2003 on 03-31-2010 03:37 PM
This was selected as the best answer
CaptainObviousCaptainObvious
Great job! You're correct- the code i suggested was indeed updating the 'map' value instead of the 'live' opportunity. I usually do my updates in after triggers, so I didnt realize my mistake. :smileyvery-happy: