Nop_ProductLoadAllPaged inefficient

Pseudo-bug -- the stored procedure works, but could be more efficient.

The stored procedure Nop_ProductLoadAllPaged uses more columns than needed:

  CREATE TABLE #DisplayOrderTmp
  (
    [ID] int IDENTITY (1, 1) NOT NULL,
    [ProductID] int NOT NULL,
    [Name] nvarchar(400) not null,
    [Price] money not null,
    [DisplayOrder1] int,
    [DisplayOrder2] int,
    [DisplayOrder3] int,
    [CreatedOn] datetime
  )

INSERT INTO #DisplayOrderTmp ([ProductID], [Name], [Price], [DisplayOrder1], [DisplayOrder2], [DisplayOrder3], [CreatedOn])
  SELECT p.ProductID, p.Name, pv.Price, pcm.DisplayOrder, pmm.DisplayOrder, rp.DisplayOrder, p.CreatedOn
  FROM Nop_Product p with (NOLOCK)
  LEFT OUTER JOIN Nop_Product_Category_Mapping pcm with (NOLOCK) ON p.ProductID=pcm.ProductID
  LEFT OUTER JOIN Nop_Product_Manufacturer_Mapping pmm with (NOLOCK) ON p.ProductID=pmm.ProductID
  LEFT OUTER JOIN Nop_ProductTag_Product_Mapping ptpm with (NOLOCK) ON p.ProductID=ptpm.ProductID
  LEFT OUTER JOIN Nop_RelatedProduct rp with (NOLOCK) ON p.ProductID=rp.ProductID2
  LEFT OUTER JOIN Nop_ProductVariant pv with (NOLOCK) ON p.ProductID = pv.ProductID
  LEFT OUTER JOIN Nop_ProductVariantLocalized pvl with (NOLOCK) ON pv.ProductVariantID = pvl.ProductVariantID AND pvl.LanguageID = @LanguageID
  LEFT OUTER JOIN Nop_ProductLocalized pl with (NOLOCK) ON p.ProductID = pl.ProductID AND pl.LanguageID = @LanguageID

but only the ProductID is extracted from the temporary table. So, the above code could be simplified to:

  CREATE TABLE #DisplayOrderTmp
  (
    [ID] int IDENTITY (1, 1) NOT NULL,
    [ProductID] int NOT NULL,
  )

INSERT INTO #DisplayOrderTmp ([ProductID])
  SELECT p.ProductID
  FROM Nop_Product p with (NOLOCK)
  LEFT OUTER JOIN Nop_Product_Category_Mapping pcm with (NOLOCK) ON p.ProductID=pcm.ProductID
  LEFT OUTER JOIN Nop_Product_Manufacturer_Mapping pmm with (NOLOCK) ON p.ProductID=pmm.ProductID
  LEFT OUTER JOIN Nop_ProductTag_Product_Mapping ptpm with (NOLOCK) ON p.ProductID=ptpm.ProductID
  LEFT OUTER JOIN Nop_RelatedProduct rp with (NOLOCK) ON p.ProductID=rp.ProductID2
  LEFT OUTER JOIN Nop_ProductVariant pv with (NOLOCK) ON p.ProductID = pv.ProductID
  LEFT OUTER JOIN Nop_ProductVariantLocalized pvl with (NOLOCK) ON pv.ProductVariantID = pvl.ProductVariantID AND pvl.LanguageID = @LanguageID
  LEFT OUTER JOIN Nop_ProductLocalized pl with (NOLOCK) ON p.ProductID = pl.ProductID AND pl.LanguageID = @LanguageID


I suspect that at one time the other columns were used for the OrderBy clause, but since the whole goal of this stored procedure is to select products and sort them, these extra columns in the temporary table are unused, and therefore unnecessary.

try this version of NOp_productloadallpaged i've done it myself and got good performance, please let me know what you think about it and if it works fast as you expect  :

USE [YOURDATABASENAME]

GO
/****** Object:  StoredProcedure [dbo].[Nop_ProductLoadAllPaged]    Script Date: 12/30/2010 15:48:38 ******/
SET ANSI_NULLS ON
GO
SET QUOTED_IDENTIFIER ON
GO


ALTER PROCEDURE [dbo].[Nop_ProductLoadAllPaged]
(
  @CategoryID      int = 0,
  @ManufacturerID    int = 0,
  @ProductTagID    int = 0,
  @FeaturedProducts  bit = null,  --0 featured only , 1 not featured only, null - load all products
  @PriceMin      money = null,
  @PriceMax      money = null,
  @RelatedToProductID  int = 0,
  @Keywords      nvarchar(MAX),
  @SearchDescriptions bit = 0,
  @ShowHidden      bit = 0,
  @PageIndex      int = 0,
  @PageSize      int = 2147483644,
  @FilteredSpecs    nvarchar(300) = null,  --filter by attributes (comma-separated list). e.g. 14,15,16
  @LanguageID      int = 0,
  @OrderBy      int = 0, --0 position, 5 - Name, 10 - Price, 15 - creation date
  @TotalRecords    int = null OUTPUT
)
AS
BEGIN
  
  
  
  declare @ParmDefinition nvarchar(500)
  declare @tmpval int
  declare @sql nvarchar(4000)
  declare @sqlrowcount nvarchar(4000)
  declare @orderbysql nvarchar(500)
  declare @SearchKeywords bit
  
  --init
  SET @SearchKeywords = 1
  IF (@Keywords IS NULL OR @Keywords = N'')
    SET @SearchKeywords = 0

  SET @Keywords = isnull(@Keywords, '')
  SET @Keywords = '%' + rtrim(ltrim(@Keywords)) + '%'

  SET @PriceMin = isnull(@PriceMin, 0)
  SET @PriceMax = isnull(@PriceMax, 2147483644)
  
  --filter by attributes
  SET @FilteredSpecs = isnull(@FilteredSpecs, '')
  CREATE TABLE #FilteredSpecs
  (
    SpecificationAttributeOptionID int not null
  )
  INSERT INTO #FilteredSpecs (SpecificationAttributeOptionID)
  SELECT CAST(data as int) FROM dbo.[NOP_splitstring_to_table](@FilteredSpecs, ',');
  
  DECLARE @SpecAttributesCount int  
  SELECT @SpecAttributesCount = COUNT(1) FROM #FilteredSpecs

  --paging
  DECLARE @PageLowerBound int
  DECLARE @PageUpperBound int
  DECLARE @RowsToReturn int
  
  SET @RowsToReturn = @PageSize * (@PageIndex + 1)  
  SET @PageLowerBound = @PageSize * @PageIndex
  SET @PageUpperBound = @PageLowerBound + @PageSize;
  

  
  



------------- CREATE Temporary Table structure to maintain it also after "sp_executeSQL" ---------------------
SELECT 0 as tot,
  0 as ROWNUMBER,
    p.ProductId  
    into #tmpPage
  FROM Nop_Product p with (NOLOCK)
  LEFT OUTER JOIN Nop_ProductLocalized pl with (NOLOCK) ON p.ProductID = pl.ProductID AND pl.LanguageID = @LanguageID
  WHERE 0=1




set @orderbysql = 'ORDER BY'
--raiserror(@orderbysql,16,1)

set @orderbysql =
CASE  when @OrderBy = 0 AND @CategoryID IS NOT NULL AND @CategoryID > 0
    THEN @orderbysql + ' pcm.DisplayOrder ASC,' else @orderbysql end




select @orderbysql =     
CASE WHEN @OrderBy = 0 AND @ManufacturerID IS NOT NULL AND @ManufacturerID > 0
    THEN @orderbysql + ' pmm.DisplayOrder ASC,' else @orderbysql END



select @orderbysql =     
CASE WHEN @OrderBy = 0 AND @RelatedToProductID IS NOT NULL AND @RelatedToProductID > 0
    THEN @orderbysql + ' rp.DisplayOrder asc,' else @orderbysql END



select @orderbysql =     
CASE WHEN @OrderBy = 0
    THEN @orderbysql + ' isnull(p.name,pl.name) asc,'
else @orderbysql end

--raiserror(@orderbysql,15,1)


select @orderbysql =     
CASE WHEN @OrderBy = 5
    THEN @orderbysql +  ' isnull(p.name,pl.name) asc,' else @orderbysql END
--raiserror(@orderbysql,16,1)


select @orderbysql =     
CASE WHEN @OrderBy = 10
    THEN @orderbysql + ' pv.Price asc,' else @orderbysql END
--raiserror(@orderbysql,16,1)

select @orderbysql =     
CASE WHEN @OrderBy = 15
    THEN @orderbysql + ' p.CreatedOn desc,' else @orderbysql END
--raiserror(@orderbysql,16,1)


--remove ending Comma    
select @orderbysql = LEFT(@orderbysql,LEN(@orderbysql)-1)
    
---------------- Create query that retrieve Products and populate Temporary table with results --------
set @sql =
N'INSERT INTO #tmpPage SELECT res.*  FROM Nop_Product nopp inner join
  (SELECT DISTINCT COUNT(*) OVER() as tot,
  ROW_NUMBER() OVER
    (' + @orderbysql +    
    ') as ROWNUMBER,
    p.ProductId    
      FROM Nop_Product p with (NOLOCK)
  LEFT OUTER JOIN Nop_Product_Category_Mapping pcm with (NOLOCK) ON p.ProductID=pcm.ProductID
  LEFT OUTER JOIN Nop_Product_Manufacturer_Mapping pmm with (NOLOCK) ON p.ProductID=pmm.ProductID
  LEFT OUTER JOIN Nop_ProductTag_Product_Mapping ptpm with (NOLOCK) ON p.ProductID=ptpm.ProductID
  LEFT OUTER JOIN Nop_RelatedProduct rp with (NOLOCK) ON p.ProductID=rp.ProductID2
  LEFT OUTER JOIN Nop_ProductVariant pv with (NOLOCK) ON p.ProductID = pv.ProductID
  LEFT OUTER JOIN Nop_ProductVariantLocalized pvl with (NOLOCK) ON pv.ProductVariantID = pvl.ProductVariantID AND pvl.LanguageID = ' + convert(nvarchar,@LanguageID) + '
  LEFT OUTER JOIN Nop_ProductLocalized pl with (NOLOCK) ON p.ProductID = pl.ProductID AND pl.LanguageID = ' + convert(nvarchar,@LanguageID) + '
  WHERE
    (
      (
        ' + convert(nvarchar,@ShowHidden) + '= 1 OR p.Published = 1
      )
    AND
      (
        p.Deleted=0
      )
    AND
      (
        ' + convert(nvarchar,@ShowHidden) + '= 1 OR pv.Published = 1
      )
    AND
      (
        ' + convert(nvarchar,@ShowHidden) + '= 1 OR pv.Deleted = 0
      )
    AND (
        ' + convert(nvarchar,@CategoryID) + 'IS NULL OR '  +convert(nvarchar,@CategoryID) + '=0
        OR (pcm.CategoryID='  +convert(nvarchar,@CategoryID) + ' AND ('  +convert(nvarchar,isnull(@FeaturedProducts,0)) + ' IS NULL OR pcm.IsFeaturedProduct='  +convert(nvarchar,isnull(@FeaturedProducts,0)) + '))
      )
    AND (
        '  +convert(nvarchar,@ManufacturerID) + ' IS NULL OR '  +convert(nvarchar,@ManufacturerID) + ' =0
        OR (pmm.ManufacturerID='  +convert(nvarchar,@ManufacturerID) + ' AND ('  +convert(nvarchar,isnull(@FeaturedProducts,0)) + ' IS NULL OR pmm.IsFeaturedProduct='  +convert(nvarchar,isnull(@FeaturedProducts,0)) + '))
      )
    AND (
        '  +convert(nvarchar,@ProductTagID) + ' IS NULL OR '  +convert(nvarchar,@ProductTagID) + '=0
        OR ptpm.ProductTagID='  +convert(nvarchar,@ProductTagID) + '
      )
    AND (
        '  +convert(nvarchar,@RelatedToProductID) + ' IS NULL OR '  +convert(nvarchar,@RelatedToProductID) + '=0
        OR rp.ProductID1='  +convert(nvarchar,@RelatedToProductID) + '
      )
    AND (
        '  +convert(nvarchar,@PriceMin) + ' IS NULL OR ' + convert(nvarchar,@PriceMin) + '=0
        OR pv.Price >  '  +convert(nvarchar,@PriceMin) + '   
      )
    AND (
        '  +convert(nvarchar,@PriceMax) + ' IS NULL OR ' + convert(nvarchar,@PriceMax) + '=2147483644 -- max value
        OR pv.Price < '  +convert(nvarchar,@PriceMax) + '
      )
     AND  (
        -- search standard content
        '  +convert(nvarchar,@SearchKeywords) + ' = 0 or
        (
         patindex('''  +convert(nvarchar,@Keywords) + ''', p.name) > 0
         or patindex('''  +convert(nvarchar,@Keywords) + ''', pv.name) > 0
         or patindex('''  +convert(nvarchar,@Keywords) + ''', pv.sku) > 0
         or ('  +convert(nvarchar,@SearchDescriptions) + ' = 1 and patindex('''  +convert(nvarchar,@Keywords) + ''', p.ShortDescription) > 0)
         or ('  +convert(nvarchar,@SearchDescriptions) + ' = 1 and patindex('''  +convert(nvarchar,@Keywords) + ''', p.FullDescription) > 0)
         or ('  +convert(nvarchar,@SearchDescriptions) + ' = 1 and patindex('''  +convert(nvarchar,@Keywords) + ''', pv.Description) > 0)          
        -- search language content
         or patindex('''  +convert(nvarchar,@Keywords) + ''', pl.name) > 0
         or patindex('''  +convert(nvarchar,@Keywords) + ''', pvl.name) > 0
         or ('  +convert(nvarchar,@SearchDescriptions) + ' = 1 and patindex('''  +convert(nvarchar,@Keywords) + ''', pl.ShortDescription) > 0)
         or ('  +convert(nvarchar,@SearchDescriptions) + ' = 1 and patindex('''  +convert(nvarchar,@Keywords) + ''', pl.FullDescription) > 0)
         or ('  +convert(nvarchar,@SearchDescriptions) + ' = 1 and patindex('''  +convert(nvarchar,@Keywords) + ''', pvl.Description) > 0)
        )
       )
    AND
      (
        '  +convert(nvarchar,@ShowHidden) + ' = 1
        OR
        (getutcdate() between isnull(pv.AvailableStartDateTime, ''1/1/1900'') and isnull(pv.AvailableEndDateTime, ''1/1/2999''))
      )
    AND
      (
        --filter by specs
        '  +convert(nvarchar,@SpecAttributesCount) + ' = 0
        OR
        (
          NOT EXISTS(
            SELECT 1
            FROM #FilteredSpecs [fs]
            WHERE [fs].SpecificationAttributeOptionID NOT IN (
              SELECT psam.SpecificationAttributeOptionID
              FROM dbo.Nop_Product_SpecificationAttribute_Mapping psam
              WHERE psam.AllowFiltering = 1 AND psam.ProductID = p.ProductID
              )
            )
          
        )
      )
    )
  ) res on res.ProductId = nopp.ProductId
  and res.ROWNUMBER >= '  +convert(nvarchar,@PageLowerBound) + '
  AND ROWNUMBER <= '  +convert(nvarchar,@PageUpperBound) + ''
  
  
  print(@sql)
  

  --Populate Temporary Table with Products search result
  execute sp_executesql @sql;
  
  
  --Assign Value to @TotalRecords, stored in column "tot" of every record of the temporary table #tmpPage
  SET @ParmDefinition = N'@totrecords int OUTPUT'
  SET @sql = 'SELECT @totrecords = MAX(tot) FROM #tmpPage'
  execute sp_executesql
  @sql,
  @ParmDefinition,
  @totrecords = @TotalRecords OUTPUT;
  
  SET @TotalRecords = isnull(@TotalRecords,0)
  
  --Perform SELECT to return exact columns that NopCommerce expected from old Stored Procedure
  SELECT DISTINCT tb1.* FROM
  (SELECT  TOP(@TotalRecords) p.ProductId,
    p.name as Name,
    p.ShortDescription,
    p.FullDescription,
    p.AdminComment,
    p.TemplateId,
    p.ShowOnHomePage,
    p.MetaKeywords,
    p.MetaDescription,
    p.MetaTitle,
    p.SEName,
    p.AllowCustomerReviews,
    p.AllowCustomerRatings,
    p.RatingSum,
    p.TotalRatingVotes,
    p.Published,
    p.Deleted,
    p.CreatedOn,
    p.UpdatedOn  
    FROM #tmpPage pc
    LEFT JOIN Nop_Product p on p.ProductId = pc.ProductId
  ORDER BY pc.ROWNUMBER) tb1
  --ORDER BY necessary cause getting from temporary table order is not garanted to be the one used to create ROWNUMBER values


  
      --Can Be Null if no records selected with the parameters passed to the stored procedure
  SET @TotalRecords = isnull(@TotalRecords,0)
  
  
  SET ROWCOUNT 0

  DROP TABLE #FilteredSpecs
  DROP Table #tmpPage
  
  
END

The query analizer / optomizer can be quite dumb at times. It turns out that

@Variablename = 1 OR ( statement )  
is much less efficient than
1 = @Variablename OR ( statement )
if i recall correctly it has something to do with evaluating against a constant instead of something that could change.
anyhow i switched all the references to this and saw a significant boost in performance.
e.g.
(
        @CategoryID IS NULL OR 0 = @CategoryID
        OR (pcm.CategoryID=@CategoryID AND (@FeaturedProducts IS NULL OR pcm.IsFeaturedProduct=@FeaturedProducts))
      )

second there is a huge performance problem if you have a large amount of product tags. apparently there is no index on the productTagId.

@ginopino

I tested your sql, i did not get back the correct amount of results. I tested your sp
with


DECLARE  @return_value int,
    @TotalRecords int

EXEC  @return_value = [dbo].[Nop_ProductLoadAllPaged2]
    @Keywords = NULL,
    @PageSize = 10,
    @TotalRecords = @TotalRecords OUTPUT

SELECT  @TotalRecords as N'@TotalRecords'

SELECT  'Return Value' = @return_value

GO


I only got 4 results in the first page and 5x the number of total records.