| stub | lifeless: distro.all_distro_archive_ids issues a db query. Did you consider inlining that? | 06:21 |
|---|---|---|
| lifeless | what do you mean by inlining it ? | 06:23 |
| lifeless | Doing it as part of the query ? | 06:23 |
| stub | 'if not len(series_clauses)' seems an odd way of spelling 'if not series_clauses' | 06:23 |
| stub | lifeless: Yes. Embedding that logic in the SQL | 06:24 |
| lifeless | stub: I was assuming that other things will hit that cachedproperty | 06:24 |
| stub | It would depend if that cached_property is being evaluated elsewhere in the pages that use this I guess | 06:24 |
| lifeless | so that inlining it would be of little benefit, and perhaps a (tiny) pessimism | 06:24 |
| lifeless | but I have no data | 06:24 |
| lifeless | happy to revisit when we have the next round of scaling data | 06:25 |
| stub | lifeless: WHERE (single_item = 'foo') is valid, so you can simplify the OR generation for combined_clause | 06:28 |
| lifeless | ok, cool | 06:28 |
| stub | lifeless: The helpers we have are what Storm gives us, but I think it would be worse to mix and match Storm & raw SQL here. | 06:28 |
| lifeless | yeah | 06:29 |
| lifeless | I thought about that and cried | 06:29 |
| stub | This could all be Storm code, but I personally don't believe that it would be more readable or maintainable that way. | 06:29 |
| stub | esp. since you would be jumping to undocumented apis like 'group by' | 06:30 |
| lifeless | agreed | 06:30 |
| stub | lifeless: So you have a GROUP BY calculating the MAX(spph.id) for each spr.id, but you don't use it. | 06:40 |
| lifeless | yeah we do | 06:40 |
| lifeless | oh hmmm | 06:40 |
| lifeless | let me review | 06:40 |
| stub | I don't think that query does what you think that query does | 06:41 |
| lifeless | perhaps | 06:44 |
| lifeless | I think the max is redundant | 06:46 |
| lifeless | wgrant: what do you think, I think you touched the query shape right before me ? | 06:46 |
| lifeless | stub: so the query is transplanted; I'm pretty sure the multi-series change is good; if the query was already faulty, thats perhaps a separate problem ? | 06:52 |
| stub | SELECT DISTINCT ON (spr.id) spr.* | 06:53 |
| stub | FROM | 06:53 |
| stub | SourcePackageRelease AS spr, | 06:53 |
| stub | SourcePackagePublishingHistory AS spph, | 06:53 |
| stub | DistroSeries | 06:53 |
| stub | WHERE | 06:53 |
| stub | spph.sourcepackagerelease = spr.id | 06:53 |
| stub | AND spph.distroseries = DistroSeries.id | 06:53 |
| stub | AND spph.status IN (1,2,3) | 06:53 |
| stub | AND ( | 06:53 |
| stub | (spr.sourcepackagename IN (1,2,3 ) | 06:53 |
| stub | AND spph.archive IN (1,2,3) | 06:53 |
| stub | AND DistroSeries.distribution = 1 | 06:53 |
| stub | ) | 06:53 |
| stub | ) | 06:53 |
| stub | Bah | 06:53 |
| stub | Forgot the order by | 06:53 |
| stub | SELECT DISTINCT ON (spph.sourcepackagerelease) spr.* | 06:55 |
| stub | FROM | 06:55 |
| stub | SourcePackageRelease AS spr, | 06:55 |
| stub | SourcePackagePublishingHistory AS spph, | 06:55 |
| stub | DistroSeries | 06:55 |
| stub | WHERE | 06:55 |
| stub | spph.sourcepackagerelease = spr.id | 06:55 |
| stub | AND spph.distroseries = DistroSeries.id | 06:55 |
| stub | AND spph.status IN (1,2,3) | 06:55 |
| stub | AND ( | 06:55 |
| stub | (spr.sourcepackagename IN (1,2,3 ) | 06:55 |
| stub | AND spph.archive IN (1,2,3) | 06:55 |
| stub | AND DistroSeries.distribution = 1 | 06:55 |
| stub | ) | 06:55 |
| stub | ) | 06:55 |
| stub | ORDER BY spph.sourcepackagerelease DESC, spph.id DESC; | 06:55 |
| lifeless | AIUI we can only get one spph back anyway because of the status check | 06:56 |
| stub | So it is working by accident? | 06:56 |
| stub | Only one published with that status in that distroseries... guess not accident. | 06:56 |
| lifeless | stub: I'd be delighted to file a bug to review this query to make it more efficient | 06:57 |
| lifeless | you'll note the two implementations were different (because one had been tuned more recently) - it was that one that I extended | 06:57 |
| stub | If you file a bug, you need an XXX: in there stating the query is obviously broken | 06:59 |
| lifeless | I want to consult the soyuz folk first | 06:59 |
| stub | But I'd rather it got fixed now given we have diagnosed it. | 06:59 |
| lifeless | I'll land a trivial with an XXX if there is an issue | 06:59 |
| lifeless | stub: so you're proposing what you pasted? | 06:59 |
| stub | The query is broken, we know that. | 06:59 |
| stub | If you are correct about the status, my query is unnecessarily complicated. | 07:00 |
| lifeless | stub: line 132 in the diff in the mp has the old 2 generations ago query | 07:00 |
| stub | Right. So either the 'latest spph' is unnecessary due to the status field, or someone broke it. | 07:01 |
| lifeless | ok | 07:02 |
| stub | This becomes fairly neat Storm code too... | 07:06 |
| wgrant | lifeless: I don't think I've touched that code recently/ever. | 07:07 |
| wgrant | stub: There will be more than one Published occasionally. | 07:07 |
| wgrant | In the middle of a publisher run. | 07:07 |
| stub | lifeless: So it seems you transplanted the getCurrentSourcePackages query for a distroseries into your new getCurrentSourcePackages for a distribution | 07:10 |
| lifeless | stub: I did | 07:11 |
| lifeless | stub: and adjusted it | 07:11 |
| lifeless | wgrant: so the distroseries code I started with was in fact bong; the max(id) is needed in there as well as the status constraints ? | 07:12 |
| stub | But that breakage we identified may not matter for a distroseries, but could matter a great deal for a distro. | 07:12 |
| lifeless | stub: distros are already broken | 07:12 |
| lifeless | stub: because they return the most recent for the distro, not the most recent for the distro development focus - so backports can show up | 07:13 |
| stub | So we are making broken code break faster :) | 07:13 |
| lifeless | in a manner of speaking | 07:13 |
| lifeless | I'd like to delete distro{,Set}.getCurrentSourceReleases | 07:13 |
| lifeless | but thats a different task | 07:13 |
| lifeless | its not my intent to break the semantics of the functions with this patch | 07:14 |
| lifeless | and I'm glad you noticed | 07:14 |
| stub | But we are breaking semantics. DistroSet.getCurrentSourceReleases used to return the 'current' source release across all distroseries, including backports (which you say is a bug). The new implementation returns a random sourcepackagerelease with a particular status across all distroseries | 07:17 |
| stub | So you could get hardy's most recent spr when you asked for Ubuntu's most recent spr. | 07:18 |
| lifeless | 1,2 is pending and published | 07:18 |
| lifeless | right, agreed | 07:19 |
| lifeless | so as you say, its broken | 07:19 |
| stub | Suckiest thing is this indicates the tests are not sufficient ;) | 07:33 |
| lifeless | well | 07:34 |
| lifeless | I hadn't run all the tests | 07:34 |
| stub | reduce(lambda _, series:milestones.extend(series.milestones), self.series_list, []) | 07:35 |
| stub | You missed your calling as a Perl programmer? | 07:35 |
| stub | Or maybe lisp? | 07:35 |
| stub | lifeless: approved with minor comments - https://code.launchpad.net/~lifeless/launchpad/bug-717394/+merge/50541 | 07:56 |
| lifeless | stub: huh, I think that goes in the next branch | 08:05 |
| lifeless | stub: sorry about the diff in the -2 branch | 08:11 |
| lifeless | I forgot to give it a prereq | 08:11 |
| stub | lifeless: Can you resub easily? | 08:11 |
| lifeless | let me see | 08:12 |
| stub | Or did the other review get everything and this one is redundant? | 08:12 |
| lifeless | it used to end badly doing htat | 08:12 |
| lifeless | https://code.launchpad.net/~lifeless/launchpad/bug-717394-2/+merge/50557 | 08:13 |
| lifeless | stub: ^ | 08:16 |
| lifeless | stub: diff is there | 08:20 |
| stub | and reviewed | 08:20 |
| lifeless | stub: that leaves just https://code.launchpad.net/~lifeless/launchpad/bug-717394-4/+merge/50548 | 08:24 |
| lifeless | stub: did you have a fixed query for bug 636158 ? | 08:25 |
| _mup_ | Bug #636158: BugTask:+index times out with many bug tasks/nominations (eg. Bug #1) <lp-bugs> <pg83> <timeout> <Launchpad itself:Triaged> < https://launchpad.net/bugs/636158 > | 08:25 |
| stub | lifeless: What query needs fixing in that bug? | 08:43 |
| lifeless | stub: thats the getcurrentsourcereleases one | 08:44 |
| lifeless | that you were fiddling with | 08:44 |
| lifeless | which I broke | 08:44 |
| lifeless | I thought you had a working one I could slot in | 08:44 |
| stub | The ones I pasted before? | 08:44 |
| lifeless | ah | 08:44 |
| lifeless | I was'nt sure that that was complete | 08:44 |
| stub | SELECT DISTINCT ON (spph.sourcepackagerelease) spr.* | 08:44 |
| stub | FROM | 08:44 |
| stub | SourcePackageRelease AS spr, | 08:44 |
| stub | SourcePackagePublishingHistory AS spph, | 08:44 |
| stub | DistroSeries | 08:44 |
| stub | WHERE | 08:44 |
| stub | spph.sourcepackagerelease = spr.id | 08:44 |
| stub | AND spph.distroseries = DistroSeries.id | 08:44 |
| stub | AND spph.status IN (1,2,3) | 08:44 |
| stub | AND ( | 08:44 |
| lifeless | it seemed to have different spph.status ;) | 08:44 |
| stub | (spr.sourcepackagename IN (1,2,3 ) | 08:44 |
| stub | AND spph.archive IN (1,2,3) | 08:44 |
| stub | AND DistroSeries.distribution = 1 | 08:45 |
| stub | ) | 08:45 |
| stub | ) | 08:45 |
| stub | ORDER BY spph.sourcepackagerelease DESC, spph.id DESC; | 08:45 |
| lifeless | cool, I'll slot that in | 08:45 |
| lifeless | and similar for DistoSeries I presume ? | 08:45 |
| stub | Yes, you need to replace my placeholders :) | 08:45 |
| lifeless | thank you | 08:45 |
| stub | Yes, although we might be able to use a simpler query if the status is sufficient for getting 'current package' | 08:45 |
| stub | Although from what wgrant said earlier, that might very occasionally return two 'current packages' | 08:46 |
| lifeless | yes | 08:46 |
| lifeless | belts and bracers for now | 08:46 |
| stub | wgrant: That looks sane to you? ^^ | 08:48 |
| wgrant | Looking. | 08:50 |
| wgrant | If it does return two packages, I wouldn't worry too much. | 08:50 |
| wgrant | The callsites are just informational. | 08:50 |
| wgrant | Just pick one. | 08:50 |
| stub | lifeless: https://code.launchpad.net/~lifeless/launchpad/bug-717394-4/+merge/50548 reviewed | 08:51 |
| lifeless | stub: hi | 09:37 |
| stub | yo | 09:37 |
| lifeless | stub: I don't see how that query actually helps | 09:37 |
| lifeless | spph.sourcepackagerelease isn't going to be distinct if there are two candidates | 09:38 |
| lifeless | and the point is to get sprs for different names at the same time | 09:38 |
| stub | spph.sourcepackagerelease isn't distinct, which is why the DISTINCT ON is there. | 09:40 |
| lifeless | hardy and lucid have different spr's for their most recent spph | 09:40 |
| lifeless | we want only one of those two sprs | 09:40 |
| lifeless | a window function can do what we need | 09:41 |
| stub | So we get all (spph, spr) records matching, ordered by (spph.sourcepackagerelease DESC, spph.id DESC) and disgard all but the first matching one | 09:41 |
| lifeless | where do we discard ? | 09:41 |
| stub | DISTINCT ON | 09:41 |
| stub | Not DISTINCT | 09:42 |
| lifeless | I get that | 09:42 |
| lifeless | but I think spph.sourcepackagerelease is different than you think it is | 09:42 |
| lifeless | distinct on spr.sourcepackagename might do it | 09:43 |
| lifeless | will the order by take the first item reliably ? | 09:43 |
| stub | oic. we are getting the latest out of all spns.... | 09:43 |
| stub | you get an error if the order by doesn't match what is needed by the ORDER BY | 09:44 |
| lifeless | right, we want multiple sprs, one spr per spn | 09:44 |
| lifeless | I think I have it, trying now | 09:47 |
| stub | SELECT DISTINCT ON (spr.sourcepackagename) spr.* | 09:47 |
| stub | FROM | 09:47 |
| stub | SourcePackageRelease AS spr, | 09:47 |
| stub | SourcePackagePublishingHistory AS spph, | 09:47 |
| stub | DistroSeries | 09:47 |
| stub | WHERE | 09:47 |
| stub | spph.sourcepackagerelease = spr.id | 09:47 |
| stub | AND spph.distroseries = DistroSeries.id | 09:47 |
| stub | AND spph.status IN (1,2,3) | 09:47 |
| stub | AND ( | 09:47 |
| stub | (spr.sourcepackagename IN (1,2,3 ) | 09:47 |
| stub | AND spph.archive IN (1,2,3) | 09:47 |
| stub | AND DistroSeries.distribution = 1 | 09:47 |
| stub | ) | 09:47 |
| lifeless | yes, thats what I have though stormisified | 09:48 |
| stub | You got DISTINCT ON in Storm syntax? | 09:48 |
| lifeless | theres a hack | 09:48 |
| lifeless | grep for DISTINCT ON in lib | 09:48 |
| lifeless | stub: so, thats wrong | 10:19 |
| lifeless | its (spr.sourcepackagename, distroseries.distribution) in the distinct | 10:19 |
| lifeless | *that* gets use unique tuples matching the input parameter | 10:19 |
| === henninge_ is now known as henninge | ||
| === mrevell is now known as mrevell-lunch | ||
| === mrevell-lunch is now known as mrevell | ||
Generated by irclog2html.py 2.7 by Marius Gedminas - find it at mg.pov.lt!