Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Identify candidates for and add SQL indices #5159

Open
Piedone opened this issue Apr 15, 2015 · 17 comments
Open

Identify candidates for and add SQL indices #5159

Piedone opened this issue Apr 15, 2015 · 17 comments

Comments

@Piedone
Copy link
Member

Piedone commented Apr 15, 2015

Let's add some SQL indices where the most common queries would need it.

We had discussions about this before that adding such indices is up to who operates the site since suitable indices depend on typical usage. This is partially true, as for the existing queries of built-in modules we can safely assume that for most of the time suitable indices will be needed.

Furthermore modules could add indices to tables of modules they depend on just for their own queries if there are no suitable ones already. Not sure about this as this would probably add a lot of coupling.

It would be great if people with big databases and high load would share what SQL Server's Missing Indexes feature suggests.

@dcinzona
Copy link
Contributor

+1

1 similar comment
@MpDzik
Copy link
Contributor

MpDzik commented Apr 15, 2015

+1

@pszmyd
Copy link
Contributor

pszmyd commented Apr 15, 2015

+1, but it highly depends on a scenario. Too many indices is not good either.
Right now we have indices on columns Orchard queries on by default, afaik.

@Piedone
Copy link
Member Author

Piedone commented Apr 15, 2015

There are a few indices but we don't remotely have one for every suitable scenerio. E.g. there is no index on UserPartRecord.NormalizedUserName though this is directly queried in Orchard.Users.

@ASADeveloper
Copy link

+1

@sebastienros
Copy link
Member

-10

Ha ha, I win

@sebastienros
Copy link
Member

We must have missed some obvious one, right. We already have all the common ones taken care of. For instance we did the changes so that we could handle millions of content items. Users might be missing, you are right. It should be tactical ones though, not all, as @pszmyd is suggesting.

@sebastienros sebastienros added this to the Orchard 1.x milestone Apr 16, 2015
@Piedone
Copy link
Member Author

Piedone commented May 8, 2015

IdentityPart.Identifier would also need an index.

@sebastienros sebastienros modified the milestones: Orchard 1.x, dev Jun 9, 2015
@alberthajdu
Copy link

alberthajdu commented Jun 17, 2017

Hi all,

We have a tool for creating a lot of content items for testing purposes https://github.com/Lombiq/Orchard-Content-Stress-Test .
So I created 300k+ content items to test performance issues like this. I clicked through the dashboard and the UI to collect some data.

It looks like that UserPartRecord.NormalizedUserName and IdentityPart.Identifier need an index.

And maybe for ContentItemVersionRecord.Latest and ContentItemVersionRecord.Published also need an index. I'm not an expert but here's some data.
I used the script from here to get the missing indices ordered by cost.
index

An another script suggests these indices too

CREATE INDEX Missing_IXNC_Orchard_Users_UserPartRecord_NormalizedUserName_D9822 ON lombiqgitfork.dbo.Orchard_Users_UserPartRecord (NormalizedUserName);
CREATE INDEX Missing_IXNC_Orchard_Framework_ContentItemVersionRecord_Latest_ECEFE ON lombiqgitfork.dbo.Orchard_Framework_ContentItemVersionRecord (Latest) INCLUDE (ContentItemRecord_id);
CREATE INDEX Missing_IXNC_Orchard_Framework_ContentItemVersionRecord_Published_7E0D0 ON lombiqgitfork.dbo.Orchard_Framework_ContentItemVersionRecord (Published) INCLUDE (ContentItemRecord_id)

There is a composit index: IDX_ContentItemVersionRecord_Published_Latest so I couldn't find any slowness in the dashboard, but still, you can see it suggests the creation of these indices.
For example this is a query which uses only the Latest column and generated when I go to Admin/Contents/List

SELECT count(*) AS y0_
FROM Orchard_Framework_ContentItemVersionRecord this_
INNER JOIN Orchard_Framework_ContentItemRecord contentite1_ ON this_.ContentItemRecord_id = contentite1_.Id
INNER JOIN Common_CommonPartRecord commonpart2_ ON contentite1_.Id = commonpart2_.Id
WHERE contentite1_.ContentType_id IN (
                5
                ,13
                ,12
                )
        AND this_.Latest = 1

What do you think?

@alberthajdu
Copy link

@sebastienros what do you think? Sholud we leave alone the Published and Latest and create only the obvious UserPartRecord.NormalizedUserName and IdentityPart.Identifier?

@sebastienros
Copy link
Member

Creating indices really depends on the load and the queries applications do. In your example you are using a standard action, so I assume it's a good candidate for a default index.

I think it's fine to add the ones you suggest.

@alberthajdu
Copy link

@sebastienros it suggests the creation of indices with INCLUDE but it's unsupported in Orchard.

CREATE INDEX Missing_IXNC_Orchard_Framework_ContentItemVersionRecord_Latest_ECEFE ON lombiqgitfork.dbo.Orchard_Framework_ContentItemVersionRecord (Latest) INCLUDE (ContentItemRecord_id);
CREATE INDEX Missing_IXNC_Orchard_Framework_ContentItemVersionRecord_Published_7E0D0 ON lombiqgitfork.dbo.Orchard_Framework_ContentItemVersionRecord (Published) INCLUDE (ContentItemRecord_id);

If I run the above command and delete the existing IDX_ContentItemVersionRecord_Published_Latest index then the missing index feature stops suggesting anything when I filter by latest and published in the content list.

If I just add indices like everywhere else in Orchard then it doesn't stops suggesting the above.

table.CreateIndex("IDX_ContentItemVersionRecord_Published", "Published");
table.CreateIndex("IDX_ContentItemVersionRecord_Latest", "Latest");

So should we open a new issue for the feature of creating indices with INCLUDE statement? Or am I missing something?

@Piedone
Copy link
Member Author

Piedone commented Jun 24, 2017

How much do these latter indices help in your test?

@alberthajdu
Copy link

@Piedone If I add the 2 indices in the Orchard-way:

table.CreateIndex("IDX_ContentItemVersionRecord_Published", "Published");
table.CreateIndex("IDX_ContentItemVersionRecord_Latest", "Latest");

And measure the total cost with the mentioned script there's no difference. The total cost of the Published and Latest increasing with 20 every time I refresh on the content list either with or without the above indices.

If I delete all indices the total cost increases with 700 every time.

With the suggested include style indices and nothing else the script stops suggesting anything so this would be the perfect soultion.

My final conclusion is that the current state is good enough but as you can see not perfect.

@Piedone
Copy link
Member Author

Piedone commented Jun 25, 2017

I mean, if you add the indices with the INCLUDEs by hand as you suggested above, what is the difference apart from the script suggesting any more indices? How much does it cut down on which queries (or page loads)?

@BenedekFarkas
Copy link
Member

I just noticed that CommonPart's Migrations greatly differ between 1.10.x and dev - it looks like that the performance improvements added on 1.10.x did not make it into dev, probably due to an incorrect automatic merge.
Unfortunately this can really screw up the migrations if you upgrade from latest 1.10.x to latest dev, so we'll probably need to discuss this issue in detail (and how to avoid it in the future).

For this specific case. we can still add those 1.10.x changes to dev as a later update step. Also, on dev, the contents of UpdateFrom7 aren't integrated into Create (it still returns with version 7).

@MatteoPiovanelli-Laser
Copy link
Contributor

We noticed the same thing just yesterday. @ElenaRepository was doing a merge of 1.10.x into dev. I think she fixed it "the other way", by putting the migration form 1.10.x before the ones from dev? It's on one of our branches. I'll let her add to this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

9 participants