/srv/irclogs.ubuntu.com/2012/10/12/#launchpad-dev.txt

StevenKwgrant: I'm not certain that the info query in _getDirectAndDuplicateSubscriptions is to blame. The slow query is using ORDER BY and DISTINCT00:04
StevenKwgrant: It's pretty much the same query, though00:07
StevenKwgrant: Yeah, the long query is from IBug.getSubscribersForPerson00:20
StevenKAnd just took 4 minutes on DF00:21
wgrant:D00:22
wgrantSo, explanalyze it00:22
StevenKThat was what took 4 minutes00:22
wgrantWhat's the plan?00:22
StevenKwgrant: http://pastebin.ubuntu.com/1274106/ and http://pastebin.ubuntu.com/1274107/00:24
StevenKwgrant: Still distracted?00:38
wgrantStevenK: Sorry, food was required.01:01
wgrantLet's see01:01
wgrantStevenK: So, do you see in the plan what I described on the call this morning?01:02
StevenKThat's okay, I amused myself by destroying Bug{Security,Visibility}Change.01:03
wgrantGreat01:03
StevenKy% bzr di | diffstat -s01:03
StevenK 7 files changed, 36 insertions(+), 236 deletions(-)01:03
wgrant:)01:04
StevenKwgrant: So I can recall you said there are two ways postgres can look up the information, either by the bug or the team01:04
StevenKAnd it's obviously going via the bug due to:01:04
StevenK                     ->  Index Scan using bug_pkey on bug  (cost=0.00..0.62 rows=1 width=4) (actual time=1.729..1.729 rows=0 loops=147850)01:04
StevenK                           Index Cond: (id = bugsubscription.bug)01:04
StevenK                           Filter: ((id = 429322) OR (duplicateof = 429322))01:05
wgrantStevenK: Not quite.01:05
wgrantThe plan is largely executed from inside to out, top to bottom01:05
wgrantSo it starts with the innermost topmost thing01:05
wgrantIn this case, the teamparticipation index scan01:05
wgrantSo it's finding all teamparticipations for the person in question01:06
wgrantAnd if you look at the right, you'll see "rows=27"01:06
wgrantSo it finds 27 teamparticipation rows01:06
wgrantfor that person01:06
StevenKwgrant: Right01:10
wgrantThen it you go up to the parent node01:10
StevenKwgrant: And that feeds into the bugsubscription index with loops=2701:10
wgrantAnd see it's a nested loop01:10
wgrantA nested loop has two nodes: the second is executed once for every row in the first node01:10
wgrantSo for each teamparticipation record, it finds the relevant bugsubscriptions01:10
wgrantAnd there are 27 rows, so loops=2701:10
StevenKAh, right01:11
wgrantThe row count there is an average01:11
wgrantSo, now we exit the first nested loop01:11
wgrantAnd find ourselves in a second one01:11
wgrantWith 150000 rows to process01:11
wgrant*Now* we query the bug for each of those 150000 bugsubscriptions01:12
wgrantAnd check whether it matches our filter01:12
wgrantSo that's roughly 150000 random seeks01:12
wgrant(although in reality it'll be cached, it's still not good)01:12
StevenKRight01:12
wgrantOnce we find the bugs that match, we do one final nested loop to join the people so we can return Person.name01:12
wgrantSo, what's the problem here?01:12
StevenKwgrant: That bug number is the worst of the lot with 1.1k dupes01:13
wgrantRight01:13
wgrantIs there an alternate strategy that postgres could use to get the same result?01:13
StevenKwgrant: I guess we'd ideally like to answer the bugsubscriptions for the bug and the person in one shot using an index01:13
wgrantNot really01:13
wgrantThe problem here is that it has to do 150000 index lookups01:14
wgrantIf we can minimise that step, we win01:14
StevenKIt wouldn't be that many01:14
StevenKSince it filter the index based on the information we want01:14
wgrantAn alternate strategy is to first ask for all the duplicate bugs, then for each bug look for the subscriptions, and compare them to a list of teams that we participate in01:15
wgrantThat works out to roughly one index scan and a hash lookup for each bug01:15
StevenKwgrant: Using a JOIN, or what are you thinking?01:15
wgrantWell, once we've devised a plan, we need to trick postgres into using it01:16
wgrantWe can't tell it directly.01:16
=== matsubara is now known as matsubara-afk
StevenKwgrant: And we do that by tweaking the query?01:18
wgrantRight01:18
wgrantIt requires a bit of experimentation01:18
wgrantOften you can force it to do your bidding using subqueries or CTEs01:19
wgranteg. a common thing is to convince it to build a hash of the teams we're looking for01:20
wgrantBy using "01:20
wgrantWITH teams AS (SELECT team FROM teamparticipation WHERE person = 1191103)"01:20
StevenKwgrant: http://pastebin.ubuntu.com/1274170/01:22
wgrantThat's a different query, isn't it?01:22
wgrantAlthough it is quite related01:22
wgrantAnd a similar plan01:22
StevenKYeah, it's different01:22
StevenKBug.id IN (SELECT id FROM BUG WHERE id = 429322 OR duplicateof = 429322)01:22
wgrantRight01:23
StevenKVersus Bug.id = 429322 OR Bug.duplicateof = 42932201:23
wgrantNote also in both plans where postgres goes wrong01:23
wgrantAfter the nested loop against bug:01:23
StevenKYeah, in the same place01:23
wgrant                     ->  Nested Loop  (cost=0.00..1172.79 rows=139 width=16) (actual time=0.063..2313.480 rows=147850 loops=1)01:23
wgrantThe first parenthesised bit is the estimate01:23
wgrantThe second is the actual01:23
wgrantSo it expected it'd get 139 bugs01:23
wgrantBut actually got 14785001:23
StevenKIt estimated ~150 rows and got back ~150k ?01:23
wgrantRight01:24
wgrantIts plan was a good one given the estimates it had01:24
StevenKwgrant: How does the teamparticipation CTE look?01:25
wgrantStevenK: Monumentally unsuccessful01:26
StevenKMaybe a bug CTE?01:26
wgrantStevenK: That might be worth a try01:28
wgrantIt could convince it to use a hash for the bug lookups01:28
StevenKAnd the query in IPersonSubscriptionInfo._getDirectAndDuplicatesSubscriptions has the same estimate versus actual factor difference, so the same change will probably make both queries performant01:28
wgrantRight :)01:28
StevenKWITH bugs AS (SELECT id FROM bug WHERE id = 429322 OR duplicateof = 429322)01:29
wgrantRight01:29
StevenKWhere does that go in the query, though?01:29
wgrantCTEs always go at the start01:29
wgranteg01:30
wgrantWITH teams AS (SELECT team FROM teamparticipation WHERE person = 1191103), bugs AS (SELECT id FROM bug WHERE duplicateof = 429322)01:30
wgrantSELECT BugSubscription.id, BugSubscription.bug01:30
wgrant[...]01:30
StevenKand then WHERE Bug.id IN bugs ?01:30
wgrantBug.id IN (SELECT id FROM bugs)01:30
wgrantIt appears as a table01:30
wgrantNot an expression itself01:30
wgrant(also note that the CTE I quoted ignores the bug.id check, just to simplify this initial experimentation)01:31
wgrantSo it only counts duplicates, not the bug itself01:31
StevenKhttp://pastebin.ubuntu.com/1274176/01:31
StevenKIt still sucks as estimating01:32
wgrantIt's never going to estimate well01:32
wgrantBecause this is an utterly extraordinary case01:32
wgrantWe have to force its hand01:32
StevenKLet me try the two CTEs like you suggest01:33
wgrantThat sounds like a good plan01:33
StevenKwgrant: http://pastebin.ubuntu.com/1274179/01:38
wgrantStevenK: You have a pointless bug join in your main query01:40
wgrantStevenK: You can just say bugsubscription.bug IN (SELECT id FROM bugs)01:40
wgrantStevenK: Any luck?01:48
StevenKwgrant: Sorry http://pastebin.ubuntu.com/1274200/01:53
wgrantStevenK: Oh01:54
wgrantStevenK: You have this person join there that's unconstrained01:55
StevenKwgrant: I don't need Person.id IN ... ?01:55
StevenKI wasn't really sure about that bit01:55
wgrantYou perhaps want "AND Person.id = BugSubscription.person" instead01:55
wgrantSo Person is actually joined against BugSubscription01:55
StevenKwgrant: http://pastebin.ubuntu.com/1274207/01:58
StevenKThat one came back quicker than I was expecting. :-)01:58
wgrantIndeed, that's more like it01:59
wgrantNow, index-only scans would make a pretty nice difference here01:59
wgrantSadly we don't have them yet01:59
wgrantStevenK: It may be helpful to see what's going on by using EXPLAIN (ANALYZE ON, VERBOSE ON) instead of EXPLAIN ANALYZE02:00
wgrantTHen it'll show the columns it grabs from each result node02:00
StevenKwgrant: http://pastebin.ubuntu.com/1274209/02:01
wgrantSo we can now see exactly what it's doing02:02
wgrantStevenK: It's still trying to go TeamParticipation->BugSubscription->Bug, as you can see02:02
wgrantBut that's faster now since it's just a hash lookup on a thousand bugs02:02
wgrantNot an index lookup02:02
wgrantIt may be worth trying to convince it to start at the bugs, then go to the bugsubscriptions02:03
StevenKSo you want faster still?02:03
wgrantPerhaps by creating a CTE of bugsubscriptions02:03
wgrantStevenK: Right02:03
wgrantStevenK: This is still quite slow02:03
wgrantFast enough to work02:03
wgrantBut it'd be nice if we could get it a bit faster02:03
StevenKNod02:03
StevenKwgrant: Can you have a play, I need to get lunch?02:04
wgrantSure02:04
wgrantWell02:04
wgrantI already have had a play02:04
StevenKwgrant: Oh?02:05
lifelessmight be better to nab the subscriptions and do a constrained cross product with person02:06
lifelessmove the person manipulation out of the inner loops02:07
lifelesswhat you have there is 8 seconds on prod02:08
lifelessmm, you are already.02:10
* lifeless fiddles02:10
lifelessbtw - http://paste.ubuntu.com/1274216/02:10
wgrantI have a reasonable solution myself, I'm trying to walk StevenK to it :)02:10
lifeless               ->  Nested Loop  (cost=0.07..857.65 rows=4 width=19) (actual time=0.447..622.478 rows=147850 loops=1) is the issue, cold cache effects will hurt you02:11
wgrantlifeless: Hm, which query is that from?02:11
wgrantYes02:11
lifelesswgrant: http://pastebin.ubuntu.com/1274209/02:11
wgrantThat's what we're trying to eliminate02:11
wgrantOh, that one, right02:11
* lifeless tries w/out person02:12
=== Ursinha is now known as Ursinha-afk
lifelessthat distinct you have is distinctly odd02:13
lifelesswhy do you want order by person.name ?02:13
wgrantI haven't looked at that particular query, so I don't know02:13
wgrantI was looking at the other one02:13
lifelessif its the portlet02:13
wgrantThe core loop is similar02:13
lifelessthen we're not slicing02:13
lifelessso bring them all back and sort in appserver.02:14
lifeless1200ms02:14
lifeless300ms hot02:14
wgrantRight, about 1.5x faster than DF, as usual02:15
lifelessthe person including one comes down to 500ms hot02:15
lifeless229ms02:15
lifeless344ms02:15
lifelessnew one is 285,193, 400 - significantly faste02:15
lifelesshe says from probably too few trials02:16
lifeless5000 lookups on bugsubscription_person_idx02:17
lifeless11ms there, 35ms on bug hash lookups02:17
lifelessstill 159K rows scanning bugsubscriptions02:18
lifelessunion time perhaps02:18
lifelesshttp://paste.ubuntu.com/1274226/ is what I just played with for ref02:19
wgrantHuh02:20
wgrantThat's a bit slow02:20
wgrantThat sort of thing was like 500ms on DF an hour ago02:20
wgrantOr is it subtly different somehow...02:20
lifelessunion is faster at the innermost layer02:21
* lifeless tries again02:22
lifelessnah, needs bigger hammer02:23
wgrantYeah02:25
wgrantI have a 15ms hammer02:25
wgrantI'm pretty sure you can't do much better than that02:25
lifelessgnar hash join diaf02:26
wgrant:D02:26
wgrantThen you disable hashjoin02:26
wgrantAnd it does mergejoin instead :)02:27
wgrantAh02:29
wgrantAnd this thing is even <100ms on the worst bug in existence02:29
lifelesswait, wtf is this doing02:30
lifelessits expanding all the teams?02:30
lifelessI mean, if its the portlet, *why are we expanding teams* ?02:30
=== Ursinha-afk is now known as Ursinha
lifelessor is it 'teams I am a member of that are subscribed'02:31
lifelessthat must be it02:31
wgrantThat's the one02:31
wgranthttp://pastebin.ubuntu.com/1274107/ is similarly misbehaved, but without the insane distinct/order02:32
wgrantThe core misbehaviour is identical02:32
wgrantIt endeavours to identify those duplicate bugs to which the user or their teams has a bugsubscription02:32
lifelesswheee, seq scan on bugsub02:33
lifelessdiaf again. Srs for reals.02:33
wgrantAh02:40
wgrantAnd with another tweak it's down to 20ms for the worst bug ever02:40
lifelessyou have become very good at this02:41
lifelessyou should blog about it02:41
wgrantHah02:41
lifelessseriously02:41
lifelessI'm down to 13ms02:41
lifelesswhats the worst bug ever ?02:41
lifelessor is that bug, person combo I've been using the worst case already ?02:42
wgrantOn DF, 429322 and 419488 are probably at the top of their respective badness dimensions02:42
lifeless7ms02:43
lifelessfor the pair I've been testing02:43
lifelesswhich is bug 42933 and person 119110302:43
_mup_Bug #42933: Kubuntu Installer crashed trying to manually edit partitions table -- Reproduceable <ubiquity (Ubuntu):New> < https://launchpad.net/bugs/42933 >02:43
lifelesswgrant: http://paste.ubuntu.com/1274251/02:44
lifelessperson 419488 gives me 0.168ms02:45
wgrantI mean bug 41948802:45
wgrantOne has the most dupes, the other has the most dupe subscribers02:45
lifelessah, stay with person 1.1M ?02:45
wgrantRight02:45
lifeless14ms02:46
lifelesswgrant: I'm curious what you came up with02:46
lifelesswgrant: and serious about bloggin :)02:46
wgrantsec02:49
wgrantjust trying with another bug02:49
wgrant492322 is very strange02:49
wgrantEr, 42932202:50
wgrantIt has 1107 duplicates02:50
wgrantBut only 35 dupe subscribers, apparently02:51
wgrant659438 has around 1000 of each02:51
wgrantlifeless: http://paste.ubuntu.com/1274256/ is what I had02:52
lifeless333 cold, 18 hot02:52
wgrantOn 659438 the performance is basically identical to yours02:52
wgrant26-27ms02:52
lifelessfor 65943802:52
wgrantI can probably inline two of those CTEs02:52
lifelessas low as 1102:52
wgrantI haven't tried since createing the bugsub CTE02:52
wgrantWhich is the critical bit02:52
wgrantAh02:53
wgrantSo yours is basically the same02:53
lifelessI think you'll trigger hash joins02:53
wgrantJust unformatted and with joins instead02:53
lifelessI threw a union in there02:53
wgrantCAn you try mine on prod/staging with 429322, 419488 and 659438?02:53
lifeless52,12,2002:54
wgrant:(02:54
lifeless19,11,1202:54
lifeless39,16,1302:55
wgrantOh02:55
wgrantThose are three runs from each?02:55
wgrantSo mine's pretty competitive02:55
lifelessyes02:55
wgrantYay02:55
lifelessyah, and a bit simpler02:55
lifelessbut returning different data02:55
wgrantIs it? :/02:56
wgrantOh, just the missing decorations like person and distincts and such02:56
lifelessI'm returning the person02:56
wgrantI ripped them out since they weren't relevant to optimising the core02:56
lifelessyou're returning the subscription id and the bug02:56
wgrantYeah02:56
lifeless42, 29, 3602:57
=== Ursinha is now known as Ursinha-afk
wgrantI was worried that this technique would be terrible for people with few teams on bugs with tonnes of dupe subs02:57
lifelessdoing 6* with changed return column to bugsubscription.person02:57
wgrantBut it seems to be pretty fast02:57
wgrantHm02:57
wgrantMysterious02:57
lifelessI get 66,11,25 doing three of mine on it02:58
lifelessmore variance02:58
lifeless10,27,27 repeating02:58
wgrantAnyway, a bit better than what we had before02:59
lifelessinduitable02:59
wgrantThough I think we might have scared StevenK away02:59
lifelessy02:59
StevenKI just got back from lunch02:59
StevenKSince it's raining cats and dogs, I had to drive02:59
lifelessteamparticipation_person_idx + bug_duplicateof_idx + bugsubscription_bug_idx and a hash on the directsubscribers result.03:00
wgrantlifeless: I had this 20ms solution before you started investigating, FWIW :P03:00
wgrantRight, that's basically the optimal plan, AFAICT03:00
StevenKExcellent, so now you share :-P03:00
lifelessfor you, teamparticipation_person_idx a BitmapOr on bug_duplicateof_idx, and - here is the difference - a HashAggregate on bugs + bugsubscription_bug_idx scan.03:00
lifelessand then you also on top o fthat do a hash for the teamwork03:01
lifelesswgrant: I know, but you were teasing StevenK with a solution you had.03:01
lifelesswgrant: which btw is slightly cruel.03:02
wgrantI hadn't revealed I had a good solution yet!03:03
wgrantI was hoping to walk StevenK through optimising that query, but I had to know I was going the right direction first :)03:04
StevenKI didn't think he was teasing03:07
StevenKHe was waiting for me to catch up03:08
StevenKAdmitedly, he could have landed and QA'd his solution before I got there03:08
wgrantlifeless: Inlining my bugs/teams CTEs as joins gets removes the excess hashes.03:08
lifelessmentoring is good03:08
lifelesswgrant: /query me the new one03:08
StevenKwgrant: Ah ha, so we want Joins03:08
wgrantStevenK: Well, yes and no. The key is a CTE03:09
StevenKOh, so BOTH. A CTE and we join on it?03:09
lifelesswgrant: interestingly I have that union *because* without it I got big dirty great hashes on bugs03:09
wgrantlifeless: Interesting03:10
lifeless258ms03:10
wgrantlifeless: I've run into that before, but it doesn't end up as a problem here03:10
wgrantHuh03:10
lifeless15ms03:10
wgrantThat very query03:10
wgrantAh :)03:10
lifeless14ms03:10
wgrantIt's 24ms on DF03:10
lifelessfwiw mine is still strictly faster :P03:10
wgrantfuuuu03:11
wgrantWhere's the time?03:12
wgrantMight be able to save a couple of ms with a slightly different TP join03:12
lifelesshttp://paste.ubuntu.com/1274276/03:13
lifelessboth in one paste03:14
lifelessbest of the runs I just did for each.03:14
wgrantYours is actually 1.5ms slower on DF03:14
wgrantIntriguing, thanks03:14
lifelessinconsequential diff on teams03:14
wgrantYeah03:14
wgrantI'm not sure how your union wins03:15
wgrantBut it does03:15
lifelessdirectsubscribers is 3ms faster03:15
lifelessunions are good03:15
wgrantRight, but it should be inconsequential here03:15
wgrantIt's probably the extra heap scan on bug03:16
wgrantBecause the bitmapor needs a recheck03:16
wgrantYeah, that likely explains it03:16
wgrantIt's trivial in just about every case03:16
lifelessit is inconsequential, its like 20% on a query with 5000% variance03:17
wgrantBut we are competing over trivial differences here, exactly :)03:17
lifelesswhose competing :)03:17
lifeless<- ego.03:17
wgrantHeh03:17
lifelessright, time to mail silbs03:18
wgrant:(03:18
StevenKIndeed :-(03:19
wgrantHm03:20
wgrantYou might be getting away without the recheck because of your limited select list03:20
wgrantYep03:21
wgrantI think03:21
wgrant... or not03:22
wgrantlifeless: Your query has the heap scan when run on DF03:22
wgrantAround the bug_duplicateof_idx scan03:22
wgrantSame as mine...03:22
wgrantAnd that indeed explains the extra 2ms03:23
lifelesswgrant: combine both and get a win ?03:25
StevenKHah, this might mean that we can destroy self._subscriber_cache in IBug03:25
StevenKIf the query in getSubscribersForPerson is performant, we can just check03:26
lifelessStevenK: long as you don't destroy they 'and folk unsubscribing, unassigning etc still get notified hack that that cache serves'03:26
StevenKlifeless: The only use of _subscriber_cache was in another method03:27
StevenKlifeless: So I don't think that cache serves that function03:31
StevenKI think that cache serves that function because the query was a pig03:31
wgrantStevenK: It'll be fast, but still not as fast as looking at a local cache03:31
wgrantWe don't want to run this query more than once a request03:31
StevenKAww03:31
wgrantIt's still not superfast, and probably cannot be03:31
wgrant20ms is quite a while03:31
wgrantOr 14ms or whatever the latest result was03:31
wgrantAnd, in general, you don't want to be repeating queries at all03:32
StevenKRight03:33
StevenKSo I'll leave it alone, sadly03:33
StevenKINNER JOIN is the same as LEFT JOIN, isn't it?03:33
wgrantNo03:34
wgrantLEFT JOIN is shorthand for LEFT OUTER JOIN03:34
wgrantA plain JOIN is an INNER JOIN03:34
StevenKRight03:34
wgrantNo, a RIGHT JOIN is also an outer join :)03:34
StevenKBleh03:35
StevenKBlah, lifeless' query is too simple03:36
wgrantYeah, lifeless' doesn't give you enough to get the subs/bugs back03:36
wgrantJust the teams03:36
StevenKAnd I don't understand your query either :-(03:38
StevenKRight03:42
StevenKNow I have a query, I need to translate it to Storm03:42
wgrantYou just want the bug IDs, right?03:42
StevenKWhich is messy since you're using the CTE as the main table in FROM03:43
wgrantI forget the original query03:43
wgrantRight, that is a tad awkward03:43
StevenKIBug.getSubscribersForPerson() wants (Person, BugSubscription)03:43
wgrantAh, I forgot about that method03:43
wgrantLet's see what it actually does03:43
StevenKPopulates a cache using DRS and results the results03:43
StevenKreturns the results even03:44
wgrantStevenK: Look what it does with the the BugSubscription, though03:49
wgrant        def cache_subscriber(row):03:49
wgrant            subscriber, subscription = row03:49
wgrant            if subscription.bug_id == self.id:03:49
wgrant                self._subscriber_cache.add(subscriber)03:49
wgrant            else:03:49
wgrant                self._subscriber_dups_cache.add(subscriber)03:49
wgrant            return subscriber03:49
wgrantIt doesn't even really care about the bug ID; it just cares about whether it's this bug or not03:49
StevenKThat's what the DRS does, what about IBug.getSubscribersForPerson()'s callsites?03:50
wgrantStevenK: They don't get to see the subscription03:50
wgrantDRS just says "return subscriber"03:50
wgrantThey get the person03:50
StevenKRight, okay, it really wants bug.id03:50
wgrantAnd it's somewhat inefficient to query each person out03:51
wgrantIt might be worth just grabbing (Person.id, Bug.id), then querying the distinct people03:51
wgrantBut then this leads to something interesting03:52
StevenKwgrant: Why? It's always one person.03:52
wgrantIf you have multiple subscriptions to dupes, it will return the person many times03:52
wgrantStevenK: No, it may be a team03:52
StevenKAh03:52
wgrantAnd, in the pathological case we discovered yesterday, it will be crash-bugs-universe 400 times03:52
wgrantPerhaps look at revising the callsites03:52
wgrantI suspect they don't need the full functionality of this method, and it can be cut back03:53
StevenKlib/lp/bugs/browser/bug.py only cares if it's empty03:53
wgrantI suspect that it's the same with a lot of the others03:54
wgrantBut I haven't looked03:54
StevenKlib/lp/bugs/browser/bugsubscription.py probably wants distinct anyway03:54
wgrantI can't see much of a legitimate use for the detail03:54
wgrants03:54
StevenKlib/lp/bugs/browser/bugtask.py seems to want to use it as a preload if it's the callsite I'm thinking of03:54
wgrant        # This line of code keeps the view's query count down,03:55
wgrant        # possibly using witchcraft.03:55
wgrantEncouraging.03:56
wgrantIt doesn't make very much sense03:56
wgrantSo I'd look at the bug and delete it03:56
wgrantOh03:56
wgrantThe bug is actually just saying we should delete it03:56
wgrantIt's two years old now03:56
wgrantNuke03:56
wgrantPray03:56
StevenKDestroy the function?03:56
StevenKbugsubscription's use of it is ... interesting03:57
wgrantDestroy the preloading callsite in browser.bugtask03:57
wgrantIt doesn't care about the output, at least03:57
StevenKlib/lp/bugs/browser/bug.py is fine, since it doesn't care about the output either03:58
wgrantRight, so browser.bugsubscription just wants distinct people03:58
StevenKRight03:58
wgrantWhich ends up being what lifeless' thing does03:59
wgrantBut it may also use the extra caching side-effect03:59
wgrantWhich lifeless' doesn't satisfy03:59
StevenKbugsubscription doesn't, no03:59
StevenKOther things in model/bug want it03:59
StevenKwgrant: Oh, it alreayd does distinct on Person.name, bugsubscription.person_id04:01
wgrantStevenK: What does?04:01
wgrantOh, so it does04:01
wgrantwtf is it trying to do04:02
wgrantI assumed it was distinct on person, bugsubscription04:02
StevenKwallyworld: https://code.launchpad.net/~stevenk/launchpad/destroy-old-bugactivity/+merge/12933904:04
StevenKwgrant: I don't get it either04:09
StevenKGiven the only thing that cares about its output is a bugsubscription browser property that wants distinct people we can probably just make it sane04:11
wgrantYou should also track down everything that uses the cache04:12
wgrantForget what the function does today04:12
wgrantJust look at what the function and cache callsites need04:13
StevenKThe cache callsites are all in model/bug04:13
StevenKThey're using the cache to work out if the person is subscribed or not and if it is this bug or one of the dupes04:13
StevenKpersonIsDirectSubscriber uses _subscriber_cache and _unsubscribed_cache04:14
wgrantDoes anything care about the actual list of subscribed teams?04:15
wgrantPossibly +subscribe04:15
wgrantI suspect most things just care about "am I or my teams subscribed to this bug?" and "am I or my teams subscribed to any of this bug's duplicates?"04:15
StevenKpersonIsSubscribedToDuplicate uses _subscriber_dups_cache and _unsubscribed_cache04:15
StevenKThat's it04:16
wgrantSo04:16
wgrantMake a list of the function and cache callsites04:16
wgrantAnd what they need04:16
wgrantWhat each needs, that is04:16
wgrantThen we can work out what the function has to do04:16
StevenKwgrant: http://pastebin.ubuntu.com/1274325/04:21
wgrantThere's also the browser.bug and browser.bugtask callsites04:22
StevenKbrowser.bugtask is gone04:22
wgrantIt may be gone04:22
wgrantIt probably is04:22
wgrantBut we should check if the lack of cache might confuse things04:23
wgrantOK, so I agree with your assessment of those callsites04:25
StevenKwgrant: http://pastebin.ubuntu.com/1274328/04:25
wgrantStevenK: Right. So now we need to look at their callsites to see what's done with them04:27
wgrantMessy :(04:27
StevenKwgrant: The two cache callsites are easy, they're just a boolean, so the cache is entirely internal04:28
wgrantStevenK: It's not quite that simple04:28
wgrantStevenK: They only return a boolean, sure04:28
wgrantBut they take a person argument04:28
wgrantwat04:29
wgrantAnyway04:29
StevenKDigging04:29
wgrantBugSubscriptionSubscribeSelfView.shouldShowUnsubscribeFromDupesWarning calls personIsDirectSubscriber and personIsSubscribedToDuplicate04:30
wgrantIndirectly, with all of the user's participated teams04:30
wgrantThat may in fact be the only place it's called with anyone other than the current user04:32
StevenKAh04:34
StevenKwgrant: So what should we do04:55
wgrantStevenK: Fix the world04:55
StevenKHah04:56
wgrantMost pages probably only care if the user is subscribed04:57
wgrantSomehow04:57
wgrantNot whether a specific team is subscribed04:57
StevenKWe want to all this trouble to write a query, so let's make use of it for IBug.getSubscribersForPerson and IPersonSubscriptionInfo._getDirectAndDuplicateSubscriptions?04:58
wgrantOther than like +subscribe, basically everything else probably just wants to know if there's a participated subscription for the bug, or for its dupes04:58
wgrantRight, either way the reworked query is necessary there :)04:58
wgrantSo it may be best to work on that first04:58
wgrantNow, IIRC that just wants bug.id?04:58
StevenK        info = store.find(04:59
StevenK            (BugSubscription, Bug, Person),04:59
wgrantThat's what it asks for04:59
wgrantWhat it asks for and what it wants are often not the same thing04:59
StevenKIt adds the bug to a set04:59
StevenKIt adds the subscriber, bug and bugsubscription to another set05:00
wgrantAh, it uses it to build the annotations, right05:00
wgrantStill don't know why that's there, but I guess we should preserve it05:00
lifelessor nuke with prejuidice05:01
lifelessWCPGW05:01
StevenKlifeless: Then you deal with the fallout05:01
StevenKI'd like to get this landed before EOD due to holidays05:02
lifeless<sarcastic response elided>05:02
wgrantOh right, holidays05:02
StevenKwgrant: Yes! And we're so damn close!05:03
StevenKWhich I'm in favour of just fiddling the two queries and then review/landing05:03
wgranthttp://paste.ubuntu.com/1274347/05:05
wgrantMay be a reasonably easy way to stormify it05:05
wgrantYou can grab BugSubscription, Bug, Person in the outer query as before05:06
wgrantBut all the filtering is done in the CTEs05:06
wgrantI think the CTEs have to be split like that, or it'll try to optimise05:06
StevenKwgrant: You assume I've done CTEs with Storm before ...05:06
* StevenK finds a nice example in sharingservice05:07
wgrantStevenK: Grep for store.with_05:08
wgrantYeah05:08
wgrantsharingservice's isn't too bad05:08
wgrantThere's also a good one in I forget where05:08
wgrantbugsummaryrebuild is a nice example of using complex CTEs to pull complex data out05:09
wgrantBut we don't need that here.05:09
wgrantStill, the bugsummaryrebuild stuff might be a good example05:10
StevenKwgrant: How do I shoehorn the join in Select() ?05:11
wgrantStevenK: You can either specify it explicitly with eg. tables=[Foo, Join(Bar, Bar.id == Foo.bar_id)], or tables=[Foo, Bar], where=(Bar.id == Foo.bar_id)05:12
StevenKwgrant: http://pastebin.ubuntu.com/1274363/ and then I'm stuck05:20
wgrantStevenK: http://pastebin.ubuntu.com/1274369/ might work05:24
wgrantI haven't tested it05:24
wgrantAnd it's only slightly different from yours05:24
wgrantBut I think it might work05:24
StevenKExcept for the hanging Person.id which is what I was actually asking about05:25
wgrantOh05:25
wgrantPerson.id == BugSubscription.personID05:25
wgrantOr person_id05:25
wgrantI forget if it's new or old05:25
wgrantAnd Bug.id == BugSubscription.bugID05:26
StevenKperson_id05:26
StevenKwgrant: So refactor the with_statement out and make use of it in model/bug?05:28
wgrantStevenK: Right, probably05:28
wgrantStevenK: postgres doesn't try very hard to optimise across CTEs, so they usually act as a handy optimisation barrier05:29
wgrantSo you can more reliably substitute them into another query and have them behave as you expect05:29
StevenKRight, pyflakes is happy at least05:37
wgrantAnd the queries work?05:41
wgrantYou probably also want to throw the branch at DF05:41
wgrantTo confirm05:41
StevenKI'm even earlier than that, I'm running tests.05:42
wgrantWell, if the query compiles at all then that's good enough for me05:43
StevenKSo far it doesn't, and I've been fixing it05:43
wgrantAh05:43
StevenKTypeError: __init__() takes exactly 3 arguments (5 given)05:44
StevenKStill getting that05:44
wgrantFrom which constructor? Select()?05:44
StevenK    with_statement = generate_subscription_with(bug, person)05:44
StevenK  File "/home/steven/launchpad/lp-branches/no-more-o-n-queries-bug-dupes/lib/lp/bugs/model/bug.py", line 2847, in generate_subscription_with05:44
StevenK    where=[TeamParticipation.personID == person.id]))05:44
StevenKTypeError: __init__() takes exactly 3 arguments (5 given)05:44
StevenKSo ... I guess?05:45
wgrantStevenK: Oh05:45
wgrantYou need a second With()05:45
StevenKOh, handy.05:46
nigelblifeless: are you leaving Canonical?05:46
wgrant[With('foo', ...), With('bar', ...)]05:46
wgrantI think05:46
StevenKProgrammingError: relation "all_bugscriptions" does not exist05:48
StevenKOh, facepalm05:48
* StevenK goes to wipe the egg from his face05:48
lifelessnigelb: I have left as of ~2 hrs back05:48
lifelessStevenK: nice typo05:48
wgrantbugscriptions05:48
wgrantThat's new one05:48
wgranta new one05:48
nigelblifeless: oh!05:49
StevenKlifeless: Oh, thanks. I try.05:49
nigelblifeless: congrats! What next? :)05:49
StevenKThis is what I get for rushing05:49
lifelessnigelb: I start at HP Cloud Services on Monday05:49
* nigelb blinks05:49
nigelbNice!05:49
StevenKProgrammingError: column reference "id" is ambiguous05:50
StevenKLINE 1: ...ption, Person WHERE BugSubscription.id IN (SELECT id FROM bu...05:50
StevenKHaha05:50
StevenKI guess that wants bugsubscription.id05:50
lifelessnigelb: I hope so :)05:51
lifelessnigelb: be a real PITA to go through the effort of leaving... and then have it be a bad choice :)05:51
nigelbHehe05:51
StevenKYou should find out by Monday afternoon :-P05:51
nigelbBeen there, done that :)05:51
spmo/ nigelb05:52
nigelbHey spm!05:54
StevenKwgrant: So now I'm getting an AssertionError from PSI.add05:54
wgrantPackageSetInclusion?05:55
wgrantOh, PersonSubscriptionInfo?05:55
StevenKYeah05:55
wgrantWhat is the assertionerror?05:55
StevenK    assert principal.is_team, (principal, self.person)05:55
StevenKAssertionError: (<Person at 0x122a14d0 name12 (Sample Person)>, <Person at 0x12b71c90 person-name-100517 (Person-name-100517)>)05:55
wgrantSounds liek the query might not be returning just relevant people05:56
StevenKSo my query is buggy :-(05:58
wgrantStevenK: What is the query?06:02
StevenKwgrant: http://pastebin.ubuntu.com/1274401/06:05
wgrantStevenK: I'm surprised that postgres accepts it06:06
wgrant"(SELECT all_bugsubscriptions" probably wants to be "(SELECT all_bugsubscriptions.id"06:06
wgrantI'm not sure what it's interpreting the former as06:06
wgrantBut it might not be what you want06:06
StevenKHeh06:07
StevenKwgrant: Same AssertionError06:08
StevenKWith AS (SELECT all_bugsubscriptions.id FROM all_bugsubscriptions ...06:09
wgrantStevenK: Oh06:10
wgrantWHERE BugSubscription.id IN06:10
wgrant    (SELECT bugsubscription.id06:10
wgrant     FROM bugsubscriptions)06:10
wgrantSpot the issue06:10
StevenKHahahaha06:10
StevenKwgrant: Shall I revert the SELECT all_bugsubscriptions bit then?06:11
StevenKOr that's fine too?06:11
wgrantYou should need both fixes06:11
wgrantIt even returns the same number of rows on DF06:12
wgrantWIth those fixes06:12
StevenKTotal: 35 tests, 0 failures, 0 errors in 48.975 seconds.06:13
StevenK(test_bug_views)06:13
wgrant:)06:13
StevenKIt neatly exercised both queries, totally by accident06:13
wgrant(that query is also confirmed fast on DF)06:14
StevenK:-D06:14
StevenKSo, commit and push?06:14
wgrantMight as well06:15
StevenKRight, done06:20
* StevenK waits for the branch scanner06:20
StevenKwgrant: https://code.launchpad.net/~stevenk/launchpad/no-more-o-n-queries-bug-dupes/+merge/12935506:32
=== micahg_ is now known as micahg
czajkowskimorning07:53
adeuringgood morning08:03
=== lifeless_ is now known as lifeless
=== maxb_ is now known as Guest72551
rick_h_morning10:50
czajkowskirick_h_: ello10:52
=== gary_poster is now known as gary_poster|away
=== gary_poster|away is now known as gary_poster
=== wgrant changed the topic of #launchpad-dev to: Assign me
wgrantblah13:09
=== wgrant changed the topic of #launchpad-dev to: http://dev.launchpad.net/ | On call reviewer: - | Firefighting: - | Critical bugs: ~260
czajkowskiwgrant: we can assign you everything if thats what you want13:11
=== micahg_ is now known as micahg
adeuringbac, sinzui, flacoste: could one of you please review my latest attempt for the security adapter for IProduct: https://code.launchpad.net/~adeuring/launchpad/authentication-for-private-products-2/+merge/129459 ?15:45
* sinzui looks15:47
flacosteadeuring: i'll leave someone else to actually review the code, but the explanation you give in the MP looks very good to me15:50
adeuringthanks :)15:50
sinzuiadeuring, looks good. I think we can simplify one test. overall r=me15:51
* sinzui update MP with smaller test15:51
adeuringsinzui: thanks15:51
=== micahg_ is now known as micahg
czajkowskisinzui: a question from potential user on commerical, I'm unsure of. in what form code is stored15:57
czajkowskilocally at your site -- is there any encryption ?15:57
sinzuino15:57
czajkowskithat answers that then15:58
czajkowskithank you15:58
=== Ursinha-afk is now known as Ursinha
abentleygary_poster: I believe I've found a bug in Zope: http://pastebin.ubuntu.com/1275277/17:27
gary_posterabentley, hi.  I'm actually mildly curious--those two particular parts of Zope are historically rock solid, with expectation bugs if anything--but I'm in crunch mode till this upcoming Tuesday's demo17:28
gary_posterbugs in the expectations of the code, is what I mean17:29
gary_posterI mean people expect something different than it does17:29
gary_postersigh :-)17:29
abentleygary_poster: What I can tell is that the adaptation of ProductSeries to IInformationType (which returns a Product) has the side effect of changing the Checker for the Product so that Checker.set_permissions does not include 'bugtracker'.17:36
abentleyEven though I don't use the instance of Product that's returned.17:36
abentleygary_poster: When you do have time, I've pushed the example to lp:~abentley/launchpad/adaptation-checker-insanity17:44
abentleyderyck, rick_h_, adeuring: Adaptation (i.e. to IInformationType) has side effects that look like a bug in Zope.  http://pastebin.ubuntu.com/1275277/17:47
deryckabentley, hmm, yeah, that looks odd.17:49
abentleyderyck: So if you see ForbiddenAttribute, it doesn't necessarily mean that the attribute isn't exposed in configure.zcml.17:50
deryckabentley, can you work around it?17:51
abentleyderyck: Yes.  First I'll see if using IProductUtil helps.  If not, I can just check self.product.information_type.17:51
abentleys/IProductUtil/IPackagingUtil/17:52
deryckok, cool.17:52
=== deryck is now known as deryck[lunch]
gary_posterack abentley17:53
sinzuibac, Do you have time to review a rewrite of a docttest? https://code.launchpad.net/~sinzui/launchpad/nomination-investigation-1/+merge/12948918:11
bacsinzui: actually, no.  i'm trying to land two branches.  maybe later.18:12
bachi statik!18:12
sinzuithanks bac18:12
abentleyrick_h_: When I go to change information_type for a product, I see radio buttons, not the choice widget.  And it includes choices it shouldn't (Public Security, Private Security...)18:25
rick_h_abentley: heading out the door, but shoot me the info of where you see this and I'll look.18:39
abentleyrick_h_: https://launchpad.dev/firefox/+edit18:39
rick_h_https://qastaging.launchpad.net/launchpad/+edit I see the edit, only the three options18:41
rick_h_firefox a projectgroup? maybe I missed something there18:41
abentleyrick_h_: No, firefox is a product, same as elsewhere.18:51
=== deryck[lunch] is now known as deryck
abentleyrick_h_: If I disable JavaScript, I see the same thing on qastaging.20:09
abentleyderyck: If you're around, could you please review https://code.launchpad.net/~abentley/launchpad/no-proprietary-linked-products/+merge/129507 ?20:19
deryckabentley, sure20:22
abentleyderyck: Thanks.20:22
deryckabentley, np20:22
deryckabentley, r=me20:47
abentleyderyck: Thanks.20:47
=== almaisan-away is now known as al-maisan
=== al-maisan is now known as almaisan-away
=== Ursinha_ is now known as Ursinha

Generated by irclog2html.py 2.7 by Marius Gedminas - find it at mg.pov.lt!