=== wgrant changed the topic of #launchpad-dev to: http://dev.launchpad.net/ | On-call reviewer: - | Firefighting: - | Critical bugs: ~160 [00:42] Hmmmm [00:43] Not really happy with this query [00:43] And it seems BulkUpdate does not love list types [00:44] Indeed, probably not [00:45] I don't like the NULLs, either [00:45] ? [00:46] wgrant: http://pastebin.ubuntu.com/1422314/ [00:46] Right, those will want to be removed. [00:47] (potentially by coalescing the potentially null array with an empty one) [00:48] Hmmmm, can I just use COALESCE for that? [00:49] Yes [00:51] Hmmm, it will only return the first, though [00:51] Hm? [00:52] "The COALESCE function returns the first of its arguments that is not null." [00:52] If I feed it an array of {NULL,0.9,1:0.9} ... [00:52] * StevenK plays [00:53] Hmmmm? [00:53] You'd either remove NULL from the resultant array, or coalesce a null array [00:53] Not coalesce an array that contains null [00:54] So my query as it stands is okay? [00:56] Perhaps something like https://pastebin.canonical.com/80039/ [01:01] * StevenK searches for his phone for 2FA [01:07] [(1, '{0.9}')] [01:07] Looks like success to me [01:07] Except for that quoting [01:08] It's probably because it's a debversion[], not a text[], so psycog2 doesn't know what to do with it [01:12] TypeError: Expected unicode, found : '{' [01:12] That's the error I'm getting from the BulkUpdate [01:13] But searchable_versions = List(type=Unicode()) because type=StringCol() didn't look right [01:13] BulkUpdate may not cope with lists [01:14] You'll need to check [01:14] Yeah, I'm about to dig in and see what UPDATE statement it is creating, but I wasn't sure if my List() defintion was sane [01:17] Hmmm [01:17] Perhaps it's my fault, since it's barfing over my values list [01:23] [[, ]] [01:23] Well, that's very handy, storm. Thanks. [01:25] there's something oddly satisfying about typing "dput -fu" [01:25] bigjools: you need to get out more :) [01:25] poppy deserves it [01:26] wgrant: I think you might be right. [01:26] lifeless: said without a trace of irony :) [01:26] (Pdb) p values[0][1]._value [01:26] '{0.9}' [01:26] Me thinks a ListVariable wants something more list-like [01:26] trevormosey must be really famous [01:26] bigjools: Heh, yes [01:27] There's a bug for that [01:27] * bigjools blames the dput ppa script [01:27] putting in that ~ by default was a huge mistake [01:28] That always trips me up too [01:31] bigjools: Well [01:31] bigjools: It probably comes from the ppa.launchpad.net HTTP layout [01:31] That was the first place to drop the tilde [01:31] * StevenK doesn't like the thought of results[1][1:-1].split(',') [01:31] StevenK: No [01:31] StevenK: Perhaps try casting the array to a text[] [01:32] See if psycopg2 interprets it then [01:32] If you manually parse the array I will strangle you :) [01:32] wgrant: You'll have to get in line behind me [01:32] I will strangle myself first [01:32] wgrant: that was also stupid :( [01:32] bigjools: Certainly [01:33] my fault, sadly [01:33] Really? [01:33] It was very early in your time... [01:33] I had the opportunity to fix it when we added multi-ppas [01:33] True [01:33] wgrant: ARRAY[]::text[] == bang [01:33] StevenK: Bang? [01:33] If that's what you mean [01:33] That seems like casting the empty array [01:34] yeah it was early in my time [01:34] Which will fail because you're trying to append it to a debversion[] [01:34] Cast the entire result array [01:35] Multi-PPAs would indeed have been the best time to add them [01:35] Although I thought that feature was somewhat influenced by the initial implementation that was removed before PPAs were released. [01:35] If I recall correctly from my bug... [01:36] Hmmmm [01:37] http://pastebin.ubuntu.com/1422367/ [01:37] Holy fuck, it works [01:37] Perhaps I shouldn't be so surprised [01:38] Oh, so my BulkUpdate hacks just worked with a list when the type was correct? [01:38] wgrant: So it seems [01:38] I'm disappointed [01:38] I put the entire block in ()::text[] which I thought was dubious at best, but worked [01:39] Well, we're stuffing them into a text[] column in the end, and we don't want to perform version comparisons on them [01:39] So it's the right thing to do [01:39] wgrant: Right, but I've not had to do manual casting in postgres before, so I was guessing [01:40] Ah [01:40] * StevenK ponders commiting evil on DF === jtv1 is now known as jtv === jtv2 is now known as jtv [03:07] wgrant: Didn't we have a PQM check that you couldn't modify anything outside database/ in a DB branch? Or did that die? [03:35] StevenK: Sometimes you need to modify tests and similar [03:35] We rely on people thinking about deployment === almaisan-away is now known as al-maisan === al-maisan is now known as almaisan-away [04:12] StevenK: https://code.launchpad.net/~wgrant/launchpad/improved-html_escape/+merge/138902 [04:13] 52-56 can be done without my eyes bleeding, surely? [04:13] Not really. [04:13] How? [04:14] Looping over replacements? [04:15] Mmm, it'll be slightly shorter. [04:17] wgrant: What about invalid unicode for line 51? [04:17] (or longer) [04:17] StevenK: It's hideous but I didn't change it [04:17] So you didn't [04:17] We assume that all messages are a unicode or an ASCII bytestring. [04:18] Because WCPGW, right? [04:18] No [04:18] Because what else can you do? [04:18] Well [04:18] Be sane [04:18] It's a reasonable assumption, and it will crash if it's not valid [04:18] Which is all that it's reasonable to do [04:19] % bzr grep 'import cgi' | wc -l [04:19] 30 [04:20] I guess they're next on the chopping block? [04:20] Indeed [04:20] But it's a big diff [04:20] So it's a separate branch [04:20] two weeks ago I moved this stuff into its own module [04:20] This branch makes it usable generally [04:20] The next two branches eliminate the other four escaping mechanisms [04:21] wgrant: r=me [04:21] Thanks [04:22] wgrant: Adding "I will be destroying all other escaping mechanisms in terms of LoC gain in a future branch" would have been nice, but meh [04:59] Dear mawson, where does it hurt? [05:02] Well [05:02] that's a few more test failures than I expected [05:02] On buildbot? [05:02] Yeah [05:02] 110 or so [05:02] Mostly due to apostrophes [05:02] Oh yes, aren't you a naughty boy [05:21] wgrant: You're working on the 111 bad tests? [05:22] Yes [05:22] wgrant: How do you feel about reviewing my +298,-60 garbo branch? :-) [05:22] I may end up reverting, but I suspect I'll be able to fix the tests before anyone else wants to land stuff [05:22] Maybe :) [05:22] It depends on 40-1 being deployed before it can land, but I can work on making it wgrant-approved first [05:28] 2012-12-10 05:28:29 DEBUG2 [PopulatePackageUploadSearchableVersions] Iteration 591 (size 2155.0): 5.698 seconds [05:28] Daaaaaaaaaaaamn [05:31] Right, no trigram indices and the rows should be much smaller :) [05:31] Trigram indices are awful [05:31] So they should only be used where necessary [05:32] I am incredibly curious just how long it will take to create the index on DF when the columns are fully populated [05:32] Yeah [05:32] wgrant: They're awful for performance? But in this case good for searching performance? [05:32] You'll probably want a GIN index on the searchable_versions [05:33] StevenK: They perform awfully in all respects [05:33] They're just less awful than a seq scan when doing a substring match [05:33] That's the only respect in which they have any value [05:34] Oh. So they're totally awful, but they're best we have for the problem. [05:34] Right [05:34] And it's not feasible to get much better [05:34] It's a hard problem [05:34] So it should be avoided [05:34] By not supporting substring matches unless it's unavoidable [05:38] wgrant: I don't have branches for the indicies yet, only a diff that gives me the TGRM for searchable_names [05:39] Right [05:43] wgrant: Hmmm, since searchable_versions is TEXT[], can I ask for rows where that column has more than one value? [05:43] StevenK: Why do you want them? [05:43] wgrant: Curosities sake, not for code [05:44] Was wondering if there was a short cut [05:44] a GIN index won't do that for you. You'd have to index array_length(searchable_versions) directly to do that efficiently. [05:45] wgrant: You misunderstand. I only want to check rows on DF manually, not in the code at all [05:45] Ah [05:45] However WHERE array_length(searchable_versions) > 1 will probably work then [05:45] WHERE array_length(searchable_versions) > 1? [05:45] Yeah [05:46] ERROR: function array_length(text[]) does not exist [05:46] ... but it's an array, you still DBMS? [05:46] Oh [05:46] s/still/silly/ [05:47] array_length(searchable_versions, 1) [05:47] Needs a dimension [05:47] (and it's 1-based, because who needssanity) [05:48] Database developers are all insane, anyway [05:56] blarfgghergherger [05:56] manual escaping ftl [05:56] some of these errors have been being double-escaped for years [06:19] Applause [06:20] Hm? [06:20] Well [06:20] LaunchpadValidationError is used by lots of fields [06:20] It HTML-escapes all errors pushed through it, so you can push through markup if you need to [06:21] But the fields are also used by eg. XML-RPC and email interfaces [06:21] Where HTML-escaping is not right [06:21] This has historically gone largely unnoticed, as only <, >, & were escaped globally [06:21] And they're relatively rare characters [06:21] s/And/As/ [06:22] For now I'm going to take the easy way out and change the error message to not contain an apostrophe... [06:22] :) [06:24] Naughty [06:24] I try. [06:28] TMI. [06:41] wgrant: https://code.launchpad.net/~stevenk/launchpad/populate-searchables-for-pu/+merge/138906 [06:52] 17 to go... [06:53] Failures? [06:53] Yes [06:54] Ooh [06:54] A real bug, finally [07:02] 1920 lines of diff later and all but three should be fixed... [07:06] Haha [07:06] That's going to be fun to review [07:08] StevenK: Do you perhaps want to split the garbo job into a separate branch? [07:08] So it can be more easily reverted later [07:08] And to make it easier to review [07:09] (also, I'd say "addSearchableName" rather than "appendSearchableNames") [07:11] addBuild is hideous; pls rewrite [07:11] appendSearchable* will want to check that the given string isn't already in there [07:12] It's probably best to parse the string into a set, then add the new names and reserialise [07:14] wgrant: I can split out the two garbo jobs if you wish, but they're very well contained [07:14] This is true [07:14] They could also use a comment or two :) [07:14] They have two each, I though? [07:14] Well, the tests do. :-) [07:14] Doesn't seem to [07:14] It's just; [07:14] MASSIVE BLOB OF THE MOST HIDEOUS SQL KNOWN TO MAN [07:15] MASSIVE BLOB OF STORM MOLESTATION [07:15] fin [07:15] :) [07:15] Although I guess they'll be gone soon [07:15] s/BLOB/TEMPORARY BLOB/ [07:15] wgrant: I'm quite proud of the two SQL statements we cooked up. [07:15] Speaking of which [07:15] They are HIDEOUS and eat babies, but they work and are performant [07:15] Why not do them both at once? [07:16] THey're going to be rewriting the same rows [07:16] Might as well do it in one hit [07:16] wgrant: Because I went to change the SQL statement in Populate...SearchableNames and it nearly bit my hand off [07:17] You should just be able to keep the two expressions separate, but in the same findspec. [07:17] Mmmmm [07:20] Right, so your complaints are: addBuild is terrible, and I should fix it, appendSearchable* should be renamed and parse the existing values if any to a set and then add the new bits, the garbo jobs should be put on a diet, IE, from two to one, and the new one could use some comments? [07:21] Right [07:21] Anything else? [07:21] The comments are going to be useful for people wanting to write similar garbo jobs in future [07:21] It otherwise looks sensible [07:21] Except the comments will die shortly [07:21] Though the bit that sets searchable_* from the PCJ could do with a comment saying the rest will be added when the PU. is added [07:21] StevenK: Sure, but they'll be in history [07:22] Perhaps I should put the comments in the caching job? [07:22] Since that is sticking around [07:22] Perhaps so [07:22] wgrant: The rest will be added when the PU is? EPARSE [07:23] StevenK: The PU[SBC] [07:23] I don't think we call any of the three add methods on a PCJ'd PU [07:24] The binaries from any builds will get a new PU [07:24] Is that method only used for PCJs? [07:24] Yeah [07:25] Indeed [07:25] That's fine, then [07:35] Ooh. [07:35] I finally upgraded. [07:35] This means I can setup LP in an LXC. [07:36] Haha [07:38] I guess "Warning: Permanetly added the RSA host key for IP address '' to the list of known hosts." means my laptop hasn't hit up LP for a long while [08:49] wgrant,StevenK: we don't have a use case for substring matching on versions in the upload queue, but we do have a use case for exact matching. Are you preserving that facility, or are we going to need to perpetrate a workaround? [08:49] cjwatson: Exact matching is easy and cheap and is being preserved [08:50] Excellent [08:50] I will strongly resist any attempt to preserve substring matching :) [08:50] (That use case being "queue accept -e packagename/version") [08:50] yep [08:50] Which reminds me, I should flip the -e default in the queue client, maybe after all this lands [08:50] After I'm done, source='foo', version='bar', exact_match=False will do a substring match for foo, but an exact match for bar [08:51] That's fine for our needs, yes [08:51] And I think what we were expecting to happen anyway [08:51] It doesn't right now [08:51] exact_match=True should be the default everywhere [08:51] If your client does not already, pls fix :) [08:51] I know, it only doesn't because: [08:52] (a) it was a translation of the LP script which was insane [08:52] Heh [08:52] Yeah [08:52] LP has some bad history there [08:52] eg. getPublishedSources, IIRC [08:52] (b) exact matching actually turns out to be quite inconvenient without SPN matching on build uploads [08:52] cjwatson: I'll be fixing b [08:52] Ah, indeed [08:53] Exactly, so I should be able to flip the default after this lands [08:53] 'This' [08:53] It's a mess of seven branches :-) [08:53] I scoff at your attempt to impose reality on my grammatical choices [08:54] Hah [09:05] aloha === Nigel_ is now known as G === almaisan-away is now known as al-maisan === benji___ is now known as benji === al-maisan is now known as almaisan-away === BradCrittenden is now known as bac === benji changed the topic of #launchpad-dev to: http://dev.launchpad.net/ | On-call reviewer: benji | Firefighting: - | Critical bugs: ~160 [14:01] I am going to assume that bcsaller is not in this morning and that we can clear up any remaining issues with his review of https://codereview.appspot.com/6909046/ later and land my branch. [14:01] and I am going to talk about it in the wrong channel, is that cool with everyone? [14:07] benji: that's cool and I approve [14:07] rick_h_: good, now I have an excuse for my crosstalk === slank_away is now known as slank [14:43] benji, Morning. Is there any reason for me to not mark BjornT's merge proposal for lp2kanban as approved so Tarmac can land it: https://code.launchpad.net/~bjornt/lp2kanban/bugs-to-cards/+merge/135909 [14:43] ? [14:43] gmb`: not that I know if. I believe it is good to go. [14:44] Awesome. === gmb` is now known as gmb [14:44] * gmb kills the imposter [14:45] Oh dear. [14:48] gmb, as a poster I find that concerning. on another note, are you helping BjornT land his lp2kanban branch? If not I can try to do so [14:49] gary_poster, I thought lp2kanban was all Tarmac goodness these days. [14:50] If not, then I'm happy to take care of it. [14:50] gmb, dunno, forgot. I was going to try to add BjornT to whatever group he needed to be in in order to land the code... [14:51] looking to see what that would be... [14:51] gary_poster, ~launchpad. [14:51] oh :-/ [14:51] (Well, that's who owns the trunk branch) [14:51] right [14:51] So I'll land it now. [14:53] ok thanks gmb [15:05] Wow, rocketfuel-get takes a while... Remind me, why did we write all this code/ [15:05] ? [15:08] gmb, that reminds me... [15:09] gmb, gary_poster is there a rule to remove tarballs from lp-sourcecode deps? I went to remove a few last week, then panicked when I realised other projects (not ~launchpad) use it [15:10] sinzui, Really? Which projects? [15:10] oops tools [15:10] maybe others [15:10] Urgh. [15:10] sinzui, I know of no rule. The proper solution has always been to not use a branch and instead have tarballs hosted somewhere internally and never delete anything. [15:10] yeah I think other projects too [15:11] Robert wanted them to be shared [15:11] This is an unintended downside, I think === almaisan-away is now known as al-maisan [15:35] bac, I've added a section for Green Squad to the lp2kanban config; can you update it on the Canonistack instance please? [15:36] gmb: sure, in an hour or two. on standup now [15:36] bac, Sure, that's fine. Thanks. [15:37] gmb: actually the config should be added to lp-tarmac-configs project not lp2kanban [15:37] gmb: the one in lp2kanban is an example only [15:38] bac, lp-tarmac-configs is the one I've updated. [15:38] Sorry, didn't make that clear. [15:39] gmb: just for fun, you could login, poweroff, and then bin/startmeup. would be nice for someone else to exercise that path. [15:40] bac, I would if I could remember to what I should ibe logging in. === bdmurray_ is now known as bdmurray === matsubara is now known as matsubara-lunch === deryck is now known as deryck[lunch] === matsubara-lunch is now known as matsubara === deryck[lunch] is now known as deryck === Ursinha is now known as Ursinha-afk === Ursinha-afk is now known as Ursinha === BradCrittenden is now known as bac [20:52] benji: you have time to review https://code.launchpad.net/~jcsackett/launchpad/no-private-releases/+merge/139092 ? [20:52] benji, do you have time to review https://code.launchpad.net/~sinzui/launchpad/person-owned-teams/+merge/139091 [20:53] jcsackett: wins! [20:53] * jcsackett laughs [20:54] jcsackett and sinzui: sure, but first you invoked the rarely used simultanious review smackdown [20:54] I am wearing glasses [20:55] You cannot hit a four-eyed developer even if he is complete an utter bastard [20:55] sinzui: i am also four eyed. i think that nixes it. :-P === mwhudson_ is now known as mwhudson [20:55] however, i like sinzui. if i must hit him to be reviewed first, i'll wait to be second. :-P [20:56] ok, then sinzui must hit you to be reviewed first [20:56] * jcsackett laughs [20:56] there must be smack in the smack down [20:56] how? [20:56] I shall invoke the one-hand clapping smack [20:57] tell you what. sinzui can drive down hear to smack me. in the meantime, benji can review my branch. :-P [20:57] s/hear/here/ [20:57] * sinzui claps [20:57] heh [20:58] * sinzui has pulled a muscel and may have broken a nail [21:05] jcsackett and sinzui: both of your branches are reviewed. I won't tell you which I did first. [21:05] thank you benji [21:05] my pleasure [21:06] thanks, benji. [21:06] any time [21:12] OOPS-3de41986b624ef0f542b92653ce1cfe8 <- ? [21:14] lifeless, timout inserting a bug. looks like a row lock [21:14] \o/ [21:14] bugs 1088650 and 1088651 appear either private or missing, to me [21:15] I tried again and got ...2 [21:17] lifeless, I can see the first, not the second, and I don't have power to confirm if the private-is-404 rule [21:17] interesting, cool. [21:18] I am suffering cat-in-lapsy [21:18] it is very distracting [22:05] deryck: Hi === benji changed the topic of #launchpad-dev to: http://dev.launchpad.net/ | On-call reviewer: - | Firefighting: - | Critical bugs: ~160 [22:31] hi wgrant [22:32] deryck: I see that a couple of your DB changes have model changes as well. [22:32] That's forbidden because we can't confirm it's safe (we don't deploy DB and app changes at the same time) [22:32] In particular, one of your branches makes a column not null at the same time as it sets a default in the model. [22:32] wgrant, I don't think my revs do. but maybe abel's does. I didn't look at his honestly, sorry. [22:32] Ah [22:33] 'royal your' :P [22:33] Well, yes :) [22:33] wgrant, ah, no that is mine. I checked with stub and thought it was cool. [22:33] wgrant, we already protect against in the model, and I did some query checks to make sure we were safe. [22:35] zAh [22:35] It's actually safe, because there's a default in the DB [22:35] But we forbid model changes in the same branch to avoid any confusion [22:35] And ensure that the devel code and db-devel DB can coexist [22:35] Rather than having to reason about it manually [22:36] wgrant, ok, sorry about that. I assume it's okay to deploy this time, so long as I don't do it again? :) [22:36] Indeed. [22:36] ok, thanks. sorry again. [22:36] abel's branch is also OK to deploy, but it's unnecessarily mixing DB and app changes, so it has to be manually verified.