[06:21] <stub> lifeless: distro.all_distro_archive_ids issues a db query. Did you consider inlining that?
[06:23] <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:24] <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:25] <lifeless> happy to revisit when we have the next round of scaling data
[06:28] <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:29] <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:30] <stub> esp. since you would be jumping to undocumented apis like 'group by'
[06:30] <lifeless> agreed
[06:40] <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:41] <stub> I don't think that query does what you think that query does
[06:44] <lifeless> perhaps
[06:46] <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:52] <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:53] <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:55] <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:56] <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:57] <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:59] <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.
[07:00] <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:01] <stub> Right. So either the 'latest spph' is unnecessary due to the status field, or someone broke it.
[07:02] <lifeless> ok
[07:06] <stub> This becomes fairly neat Storm code too...
[07:07] <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:10] <stub> lifeless: So it seems you transplanted the getCurrentSourcePackages query for a distroseries into your new getCurrentSourcePackages for a distribution
[07:11] <lifeless> stub: I did
[07:11] <lifeless> stub: and adjusted it
[07:12] <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:13] <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:14] <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:17] <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:18] <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:19] <lifeless> right, agreed
[07:19] <lifeless> so as you say, its broken
[07:33] <stub> Suckiest thing is this indicates the tests are not sufficient ;)
[07:34] <lifeless> well
[07:34] <lifeless> I hadn't run all the tests
[07:35] <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:56] <stub> lifeless: approved with minor comments - https://code.launchpad.net/~lifeless/launchpad/bug-717394/+merge/50541
[08:05] <lifeless> stub: huh, I think that goes in the next branch
[08:11] <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:12] <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:13] <lifeless> https://code.launchpad.net/~lifeless/launchpad/bug-717394-2/+merge/50557
[08:16] <lifeless> stub: ^
[08:20] <lifeless> stub: diff is there
[08:20] <stub> and reviewed
[08:24] <lifeless> stub: that leaves just https://code.launchpad.net/~lifeless/launchpad/bug-717394-4/+merge/50548
[08:25] <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:43] <stub> lifeless: What query needs fixing in that bug?
[08:44] <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:45] <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:46] <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:48] <stub> wgrant: That looks sane to you? ^^
[08:50] <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:51] <stub> lifeless: https://code.launchpad.net/~lifeless/launchpad/bug-717394-4/+merge/50548 reviewed
[09:37] <lifeless> stub: hi
[09:37] <stub> yo
[09:37] <lifeless> stub: I don't see how that query actually helps
[09:38] <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:40] <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:41] <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:42] <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:43] <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:44] <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:47] <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:48] <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
[10:19] <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
=== henninge_ is now known as henninge
=== mrevell is now known as mrevell-lunch
=== mrevell-lunch is now known as mrevell