SQLObjectResultSet.__nonzero__() implementation does not strip result ordering.

Bug #246200 reported by Celso Providelo
4
Affects Status Importance Assigned to Milestone
Launchpad itself
Invalid
Undecided
Unassigned
Storm
Fix Released
Low
Jamu Kakar

Bug Description

The current implementation of SelectResultSet.__nonzero__ (used in bool(result_set)) is inefficient because it doesn't remove the ordering bits.

A lot of time is spent ordering the result set that will not affect the result of the operation, for instance bool(IArchive.getPublishedSources()).

The original implementation of __nonzero__ in SQLObject does remove the ordering part correctly and this is the fastest alternative, it's faster then count(), any() or first();

Related branches

Revision history for this message
Celso Providelo (cprov) wrote :

See query #19 on OOPS-919EB43

Revision history for this message
James Henstridge (jamesh) wrote : Re: Archive index pages timeout because of inefficient storm.sqlobject __nonzero__

While that query is taking quite a while, it isn't clear that removing the ORDER BY would stop that page from timing out. There is a 19 second gap between statement #32 and the one where the TimeoutError was raised.

That should be investigated as well as fixing Storm to remove the ORDER BY clause.

Revision history for this message
Celso Providelo (cprov) wrote :

Thanks for replying, James.

Yes, of course, there are other serious issues that needs to be sorted in order to eliminate the timeouts we are experiencing.

The initial report mentions the inefficient __nonzero__ implementation, specifically, because it surely has implications that go beyond that page.

Revision history for this message
James Henstridge (jamesh) wrote :

We should definitely improve the SQLObjectResultSet.__nonzero__ implementation.

That said, if __nonzero__() returns True, do you go ahead and repeat the full query? If so, then it'd be better to restructure your page to just go ahead and do the query once and handle zero rows case appropriately.

There is no __nonzero__() in the native Storm ResultSet class as its use usually involves repeating queries for no good reason.

Revision history for this message
Celso Providelo (cprov) wrote :
Revision history for this message
Celso Providelo (cprov) wrote :

If the __nonzero__ is True we issue a similar query, but at that time its restricted to the package-search form (default) values.

We issue that query because it was supposed to be the simplest/quickest way to figure out when a PPA is inactive (zero source publications) and then easily modify the layout to avoid issues other empty or pointless queries. However, I admit it's a little cumbersome for active PPAs.

Curtis Hovey (sinzui)
tags: added: tech-debt
Changed in soyuz:
status: New → Invalid
Jamu Kakar (jkakar)
Changed in storm:
milestone: none → 0.17
assignee: nobody → Jamu Kakar (jkakar)
importance: Undecided → Low
status: New → In Progress
Jamu Kakar (jkakar)
Changed in storm:
status: In Progress → Fix Committed
Jamu Kakar (jkakar)
Changed in storm:
status: Fix Committed → Fix Released
To post a comment you must log in.
This report contains Public information  
Everyone can see this information.

Other bug subscribers

Remote bug watches

Bug watches keep track of this bug in other bug trackers.