Uncovered over at #921210: Fix performance of "My issues" and #928110-3: Expose tracker2 tables and columns to views...

If your query includes node.status <> 0 in WHERE instead of node.status = 1, it forces the query to be a range query, which makes it less likely you'll be able to use an index.

{node}.status (for obvious and good reasons) is just reusing the generic boolean filter and argument handlers. However, I think we should subclass those specifically for this field to make it do the more restrictive = 0 vs. = 1 in the query() methods, since that's going to potentially speed up a lot of queries. Just about every node view on earth includes this filter (or should), so I think the aggregate performance win outweighs the slight coding burden of a separate handler for this field.

I'll try to roll a patch in the near future...

Comments

premanup’s picture

Yes, it would be very nice patch.

premanup’s picture

Status: Active » Needs review
StatusFileSize
new580 bytes

I think it should be a good start.

dawehner’s picture

Status: Needs review » Needs work

Mh. I dont' like to write such a thing into the handler.

You could make this configurable via a definition flag.

dww’s picture

Assigned: dww » Unassigned

Oh yeah, interesting. Doesn't need to be a whole separate handler, could just be a definition flag. I like it.

That said, I have no time to work on this anytime soon, so I'm going to have to unassign myself. :/

dagmar’s picture

Version: 6.x-2.x-dev » 6.x-3.x-dev
Status: Needs work » Needs review
StatusFileSize
new1.51 KB

With a UI option. Maybe it needs a better #description.

bojanz’s picture

Should we make this visible to the user? I don't think so, it's not the user's decision to make. Just makes the UI more confusing, with no gain.
Would be better for it to just be a key and value in the definition array (written by the developer).

dww’s picture

Status: Needs review » Needs work

Yeah, -1 to approach #5. We should just do what I said in #4. Whether the query should use = 1 or <> 0 is up to the developer assigning this handler to a particular field in a table, not the end user constructing a view.

dagmar’s picture

Status: Needs work » Needs review
StatusFileSize
new1.51 KB

Oh, I see. What about this patch?

bojanz’s picture

I like it.

dawehner’s picture

StatusFileSize
new1.51 KB

Here is a new version which uses a space instead of a "_". This is more like the style of the other flags.

bojanz’s picture

Status: Needs review » Reviewed & tested by the community

Let's see what Earl thinks.

dww’s picture

Assigned: Unassigned » merlinofchaos
Status: Reviewed & tested by the community » Needs review
StatusFileSize
new3.59 KB
new3.1 KB

Yeah, that's definitely the idea. I'm not thrilled with "use_equal" as the definition key for this, but I can't come up with anything better off the top of my head. Maybe someone else has a bright idea. ;)

Here it is re-rolled to fix the query in the "Published or admin" filter, too. Also, re-rolled for the D7--3 branch. The D6 patch applies cleanly to both D6--2 and D6--3.

Bumping this up to merlinofchaos for a final review (and feedback on use_equal). If he likes it, I can commit to all 3 branches.

Cheers,
-Derek

dww’s picture

Assigned: merlinofchaos » dww
Status: Needs review » Needs work

Whoops, sorry, cross-post. I'll re-roll #12 for the space instead of _...

dww’s picture

Status: Needs work » Needs review
StatusFileSize
new3.59 KB
new3.1 KB
dww’s picture

Assigned: dww » merlinofchaos

Yeah, let's get a final okay from Earl, then I'll commit.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

The difference looks fine :)

merlinofchaos’s picture

Status: Reviewed & tested by the community » Fixed

Committed to all branches.

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.

__mark’s picture

Does that mean it was also committed to views2?

I have 2.12, but queries are still using node.status <> 0. I've tried searching, but I can't find anything on the '<>' operator. Does it mean that views is looking for any status that is greater than or less than 0? What is the reason for looking for anything other than 0 or 1?

Sorry, I'm just learning about sql queries, and at the moment this isn't making any sense to me.

merlinofchaos’s picture

It was committed to all branches, but there hasn't been a full release; 2.12 was only a security release of 2.11 with a single patch.

__mark’s picture

Ok. I checked out the latest code from cvs and views is still filtering 'published' with <> 0. I tried clearing all caches, removing and adding the 'published' filter, and I even tried applying the patch from #2.

I also tried searching for '<>' throughout all of the module's code, to see if I could find where I could manually hardcode in an = 1, but the only file that seemed related to the 'published' filter was the one that the patch from #2 affects.

__mark’s picture

I figured it out, I think. Somehow while using a GUI instead of the command line, a copy of the views directory ended up in an acl module's folder, and views was using that code, instead of what was in the views directory.

I doubt anyone else is dumb enough to make that mistake, so this may just actually be a complete waste of screen real estate.

merlinofchaos’s picture

I doubt anyone else is dumb enough to make that mistake, so this may just actually be a complete waste of screen real estate.

Don't doubt that -- it happens all the time. It's easy enough to do by mistake and once you've done it, it's hard to tell it's happened.