[00:08] <mwhudson> wgrant, StevenK: do you guys keep track of how many lines of source code you've killed from lp over the last little while?
[00:11] <cjwatson> I have a graph I update every now and again
[00:13] <cjwatson> http://people.canonical.com/~cjwatson/tmp/loc-cum.png
[00:13] <cjwatson> Data per revision in http://people.canonical.com/~cjwatson/tmp/loc
[00:14] <mwhudson> it started around 500k loc ?
[00:14] <mwhudson> so down about 15%
[00:14] <cjwatson> About a million, I think
[00:14] <mwhudson> ah
[00:14] <mwhudson> still a nice big number
[00:15] <wgrant> Depends on how you count, but by most metrics is 700KLOC-1MLOC
[00:31] <StevenK> mwhudson: I've killed close to 40k
[00:32] <StevenK> That last big drop on http://people.canonical.com/~cjwatson/tmp/loc-cum.png was me killing YUI 2.
[00:46] <lifeless> woo
[00:46] <lifeless> killing the source code gently
[00:52] <wgrant> StevenK: In https://code.launchpad.net/~stevenk/launchpad/index-previewdiff-merge_proposal/+merge/162915 do you also want to make date_created NOT NULL?
[00:53] <wgrant> And you need to rename the column in the ALTER TABLE to branch_merge_proposal
[00:53] <wgrant> You should do both NOT NULLs in the same statement to get them done in a single table scan.
[00:56] <StevenK> wgrant: Do I need DESC in the index too?
[00:56] <wgrant> StevenK: It only makes a very marginal difference in this case, so I wouldn't bother.
[00:56] <wgrant> It's important for the bug indices where we have a compound sort in opposing directions.
[00:59] <wgrant> StevenK: https://code.launchpad.net/~stevenk/launchpad/switch-bmp-to-previewdiff-merge_proposal/+merge/162916 still pulls out all of the preview diffs just to get the last one
[00:59] <wgrant> I'd do
[00:59] <wgrant> @cachedproperty
[00:59] <wgrant> def preview_diff(self):
[00:59] <wgrant>     return self._preview_diffs.first()
[01:00] <StevenK> wgrant: Ah, but isn't first the earliest diff?
[01:00] <wgrant> I can think of an easy solution to that.
[01:01] <StevenK> Asc(date_created) ?
[01:01] <wgrant> (use .last(), or reverse the sort)
[01:01] <StevenK> Ah, so .last() does exist
[01:03] <StevenK> wgrant: Index fixed
[01:03] <wgrant> StevenK: You also probably need to alter the preloading to populate preview_diff rather than preview_diffs
[01:03] <wgrant> Anything that wants multiple BMPs isn't going to care about their historical diffs, just their current ones.
[01:04] <StevenK> wgrant: Except that I'm pulling all previewdiffs
[01:04] <wgrant> That there is precisely the problem.
[01:04] <wgrant> It's unnecessarily pulling an unbounded volume of data.
[01:05] <wgrant> It's potentially a large set, particularly before we have pruning implemented.
[01:07] <StevenK> wgrant: I agree, but at the time I couldn't think of a way to make certain we only return the latest diff
[01:13] <wgrant> StevenK: DISTINCT ON
[01:14] <wgrant> SELECT DISTINCT ON (branch_merge_proposal) * FROM previewdiff WHERE branch_merge_proposal IN (1, 2, 3, 4) ORDER BY date_created;
[01:14] <wgrant> ish
[01:14] <wgrant> That'll return the oldest for each MP, but you get the idea
[01:15] <wgrant> I believe I added the .config(distinct=PreviewDiff.branch_merge_proposal) syntax to storm a while ago, so that should be easy.
[01:41] <StevenK> wgrant: Both index and switch-bmp have been pushed, and the diff has regenerated.
[01:44] <wgrant> StevenK: ITYM Desc
[01:44] <wgrant> In the previewdiff preloading
[01:44] <wgrant> Asc is the default
[01:44] <wgrant> I believe that what you have there will grab the oldest, not the newest
[01:44] <wgrant> Which perhaps suggests that your preloading test is incomplete.
[01:44] <StevenK> Preloading test? :-)
[01:45] <StevenK> There's a preloading test?
[01:45] <wgrant> Hopefully :)
[01:45] <wgrant> If not then it's not difficult to add.
[01:45] <StevenK> wgrant: You tell funny jokes. I'll add one.
[03:50] <wgrant> StevenK, lifeless: Do you recall why ~canonical-launchpad exists?
[03:51] <lifeless> No
[03:51] <lifeless> is it private?
[03:51] <wgrant> No
[03:51] <wgrant> AFAICT it was only used to own production-auditor, which I've fixed.
[03:51] <lifeless> it says '
[03:51] <lifeless> Canonical employees working directly on Launchpad.
[03:51] <lifeless> '
[03:51] <wgrant> Which is roughly the definition of ~launchpad
[03:51] <lifeless> yeah
[03:51] <lifeless> there are specific branch owning teams already
[03:52] <lifeless> which the new project policy page references
[03:52] <wgrant> Exactly
[03:52] <wgrant> And this isn't a member of any teams, and only had privileges in one project
[03:52] <wgrant> So I'm not sure why it exists
[03:52] <wgrant> Doesn't own any branches, not subscribed to any bugs...
[04:56] <StevenK> wgrant: switch-bmp is ready for you again.
[05:03] <wgrant> StevenK: Oh, that preloading sort should probably actually be on (bmp, date_created)
[05:03] <wgrant> It'll work as it is now, but potentially slowly and it doesn't really make sense
[05:24] <StevenK> wgrant: .order_by(PreviewDiff.branch_merge_proposal, Desc(PreviewDiff.date_created) ?
[05:24] <wgrant> StevenK: Right
[05:37] <StevenK> wgrant: Is that your only remaining concern?
[05:37] <wgrant> StevenK: I thiiiiiink so, apart from the pruning thing
[05:37] <wgrant> Which we at least need a plan for before this lands
[05:38] <StevenK> wgrant: Given your idea that we want only one previewdiff per MP in the short term, I'm happy to put a pipe at the end that adds it
[05:41] <wgrant> StevenK: Sounds good
[05:41] <wgrant> I'll go over the MPs again for a final check in a sec
[06:01] <StevenK> wgrant: I want GROUP BY for this query to only show old previewdiffs?
[06:02] <wgrant> StevenK: DISTINCT ON and GROUP BY are usually mutually exclusive
[06:02] <wgrant> They perform a similar role, except that GROUP BY gives you aggregates.
[06:04] <StevenK> I don't want DISTINCT branch_merge_proposals, though
[06:05] <wgrant> StevenK: DISTINCT ON branch_merge_proposal
[06:06] <wgrant> Not DISTINCT branch_merge_proposal
[06:06] <StevenK> To handle the case that we have 2 old previewdiffs to be pruned.
[06:06] <wgrant> Oh
[06:06] <wgrant> To show old ones
[06:06] <StevenK> Yes
[06:06] <wgrant> You want neither
[06:06] <wgrant> You'll need window functions or a subquery
[06:07] <StevenK> I can see how a window function can help, how does a subquery?
[06:07] <wgrant> You can use a subquery to select the latest previewdiff for the MP and then use NOT IN
[06:07] <wgrant> Or !=
[06:16] <StevenK> Bah
[06:16] <StevenK> ERROR: window function requires an OVER clause
[06:16] <StevenK> And so I add one:
[06:17] <StevenK> ERORR: window functions not allowed in WHERE clause
[06:18] <wgrant> Sure, you usually have to use a subquery
[06:18] <wgrant> So you return the window function result in the select list of a subquery, and filter on that in the outer query
[06:24] <StevenK> wgrant: http://paste.ubuntu.com/5646915/
[06:25] <wgrant> StevenK: Right
[06:25] <StevenK> It even works
[06:49] <StevenK> wgrant: .config(distinct=PreviewDiff.branch_merge_proposal) is not making use of DISTINCT ON
[06:51] <wgrant> StevenK: Hm, it possibly needs to be a tuple
[06:51] <wgrant> I forget
[06:51] <wgrant> I only wrote it
[06:52] <wgrant> 39	+ if isinstance(select.distinct, (tuple, list)):
[06:52] <wgrant> 40	+ tokens.append(
[06:52] <wgrant> 41	+ "ON (%s) " % compile(select.distinct, state, raw=True))
[06:53] <StevenK> Excellent
[08:12] <cjwatson> StevenK: YUI 2> Yeah, I went and looked at that commit :)
[08:13] <StevenK> cjwatson: Haha, as a "What the hell just happened?" sort of check?
[08:16] <cjwatson> Not really, I just tend to look at big negative commits out of interest