[00:02] lifeless: Morning [00:02] lifeless: I started to take a look at the caching [00:02] lifeless: Is this what you meant? https://code.launchpad.net/~huwshimi/launchpad/avatars-everywhere-712894/+merge/81430 [00:03] (I've pushed changes which will be in the diff) [00:03] lifeless: Is there more that I need to do? [00:05] lifeless: You'll have to bear with me, as I really don't know what I'm doing here :) [00:07] heh, sure [00:07] huwshimi: thats pretty close [00:08] I'll give a longer answer in the review [00:08] lifeless: Thanks :) [00:13] done, -> optometrist, bbiab [00:54] %#$@ [00:54] die, postgres, die [00:54] ? [00:54] What about it? [00:55] The planner is making stupid decisions. [00:55] * wgrant pastes. [00:57] lifeless: http://paste.ubuntu.com/731544/ [00:57] First plan, for a single policy. [00:58] Very sensible. [00:58] Straight double nested loop down three indices. [00:59] Second one, also a single policy but not specifying the ID directly (this is how I initially tried it): terrible terrible plan doing a manual person sort afterwards. It should have known it would get at most one row, but perhaps the policy ID I hardcoded is in the stats so it gets special treatment. [00:59] Third one, hardcoding the three IDs. [00:59] Even worse plan. [00:59] (these are Ubuntu's three access policies, on real data) [00:59] wgrant: a request for these pastes in future [00:59] wgrant: please wrap the queries. [01:00] lifeless: I normally do, but was too grumpy this time. [01:00] I tried a CTE, but it doesn't work. Even if it returns just the one row. [01:00] I guess because it plans beforehand. [01:01] It's probably seeing that the 97822 policy is special and planning accordingly, I guess. [01:02] so id=97822 == policy=1 ? [01:02] But if it knows it's going to dominate, then why does it ignore it when I specify 3 IDs... [01:02] Ah, yes, sorry. [01:02] 97822 == distro 1, policy 1 [01:02] Which is Ubuntu private non-security. [01:02] is policy an enum ? [01:02] Yes. [01:03] It may be better to explode it here. [01:03] But only because the planner is being obtuse. [01:03] It's perfectly feasible to do the same simple nested loop down three indices, just with an ANY on the apg filter. [01:03] look at the estimates [01:03] its estimating 2, then 45 [01:04] 2nd plan [01:04] Yeah. [01:04] first one its estimating 57K rows - and considering 1.2M [01:04] but the actual execution is fast [01:05] this is on df ? [01:05] Yes. [01:05] I have the queries to set up the data in a minute or two, if you want. [01:06] the second plan has a lower cost estimate [01:06] and the third lowest of all [01:06] Yeah, because the row count estimates are off. [01:06] so the interesting question is why [01:06] have you analyzed ? [01:06] By a factor of 3. [01:06] Er, 3 orders of magnitude. [01:06] Yes. [01:06] VACUUM ANALYZE, ANALYZE, etc. [01:06] Several times. [01:06] No change after the initial one. [01:06] scale 1000 ? [01:06] or the 100 df is running ? [01:06] 100 [01:07] care to try [01:07] I guess. [01:07] Worth a try. [01:07] its a thing to rule out [01:09] so the first one is potentially a bad plan too [01:09] but we expect the data sizes to stay modest [01:09] what does it do if you say offset 500 to it ? [01:09] [curiosity] [01:09] I'm not sure that's important, but sure. [01:10] 680ms, so it's pretty linear. [01:10] As expected from the plan. [01:12] Doing a sort-key-based offset is very fast, also as expected. [01:12] That plan is perfect for the main disclosure management view. [01:13] (unless the set of observers is small) [01:15] person_sorting_idx is only 57MB, and presumably very hot, so it may even be fast enough when there aren't many. [01:16] I suspect the first batch is actually slower than the rest. [01:17] Because it's going to have to walk through all the usernames that start with digits. [01:17] Roughly none of which are going to be relevant, because sensible people don't generally have usernames that start with digits. [01:17] UPDATE person set name = '8wgrant' where name = 'wgrant'; commit; [01:18] I actually mean displayname. [01:18] But yes. [01:31] rofl [01:31] https://github.com/juuso/BozoCrack [01:35] Heh [01:35] It is a useful strategy sometimes. [01:40] lifeless: So, any query suggestions? [01:41] sorry was sidetracked [01:41] uhm [01:41] (distribution, policy) is unique [01:42] but id is what shows up on the other tables [01:42] Yes. [01:42] where are your data generating scripts? [01:42] (and do they work on temp tables ;)) [01:44] They are temp tables, yes. [01:44] wgrant: try this http://paste.ubuntu.com/731583/ [01:44] Just dropped the fks and it all works. [01:44] bah [01:44] not limit 0 [01:44] offset 0 [01:45] http://paste.ubuntu.com/731585/ <- forgive the awful code to create artifact-specific permissions. There's probably a better way to do it, but everything else I tried was unbelievably slow on DF. [01:50] lifeless: Erm, explicit offset 0? [01:50] Isn't that redundant? [01:50] (also, that's still 3.5s) [01:51] optimisation barrier [01:51] I think that's the problem. [01:52] I suspect it chooses the index-scan-only plan because the ID it's looking for dominates the stats. [01:52] Except not. [02:06] what do you mean by 'dropped the fks and it all works' [02:06] The original table definitions had foreign keys to person/bug/product/etc. [02:06] But temp tables can't have fks to permanent tables. [02:06] So I dropped the constraints. [02:06] oh right, of course ;) [02:07] I presumed that (bugsummary experience most recently ;P) [02:08] hah [02:08] Time: 27666.227 ms [02:09] Oh? [02:09] first run of second plan [02:09] Ah [02:10] SELECT DISTINCT ON (person_sort_key(person.displayname, person.name)) person.id FROM accesspolicygrant JOIN accesspolicyuse ON accesspolicyuse.id = accesspolicygrant.policy JOIN person ON person.id = accesspolicygrant.person WHERE accesspolicyuse.distribution = 1 AND accesspolicyuse.id=97822 ORDER BY person_sort_key(person.displayname, person.name) LIMIT 75; [02:10] id [02:10] ---- [02:10] (0 rows) [02:10] Your IDs will be different :) [02:10] ah, doh [02:11] SELECT id, policy FROM accesspolicyuse WHERE distribution=1; [02:11] we'll need to iterate on abels code [02:11] to paginate on a function sort [02:12] Yes. [02:12] I had already looked at that a little. [02:12] did you look at nuking the functional index? [02:13] Just using eg. person.name for these queries? Yes, didn't help. [02:14] wgrant: Is this what you meant for fixing that XSS problem? http://paste.ubuntu.com/731598/ [02:15] huwshimi: !!! someone did it right the first time. Yes, that's what I was meaning. [02:15] wgrant: Hah! It might look right, but it provides a mangled output [02:16] Oh? [02:16] Ah [02:17] You may need to return .escapedtext, depending on how you're using it. [02:17] wgrant: It returns this: http://paste.ubuntu.com/731600/ [02:17] If it's in a TALES formatter, it probably will need .escapedtext. [02:17] wgrant: Ah [02:17] Yeah. [02:17] Sorry, forgot to mention that. [02:18] wgrant: That's fine :) [02:19] wgrant: Perfect, working now :) [02:19] Great! [02:21] wgrant: .name wouldn';t help here, but for abels work... [02:22] registrant | 1 | 4 | 0 [02:22] we might want to drop that [02:22] ? [02:23] totally unused colum on person [02:23] s/drop/fix/ [02:23] also language [02:24] 'meh' [02:28] wgrant: can you paste me [02:28] SELECT attname, most_common_vals, most_common_freqs FROM pg_stats WHERE tablename='accesspolicyuse'; [02:29] actually, nvm [02:29] http://paste.ubuntu.com/731614/ anyway. [02:31] thats very interesting [02:31] I wonder why distribution is blank [02:31] Distributions are extremely rare. [02:31] 40 vs 33000 [02:32] SELECT attname, most_common_vals, most_common_freqs FROM pg_stats WHERE tablename='accesspolicyuse' and attname='distribution'; [02:32] attname | most_common_vals | most_common_freqs [02:32] --------------+---------------------------------------------------------------------------------------------------+------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- [02:32] distribution | {1,2,3,4,5,6,7,8,9,10,11,12,13,14,15,16,17,18,19,20,21,22,23,24,25,26,27,28,29,30,31,32,33,34,35} | {3.05157e-05,3.05157e-05,3.05157e-05,3.05157e-05,3.05157e-05,3.05157e-05,3.05157e-05,3.05157e-05,3.05157e-05,3.05157e-05,3.05157e-05,3.05157e-05,3.05157e-05,3.05157e-05,3 [02:32] Even a stats target of 1000 might not catch a distribution. [02:32] ... [02:32] e.g. its not blank for me [02:32] You probably still have a huge target. [02:32] * wgrant tries. [02:33] 105 apu entries with no product [02:33] 98K where it is [02:33] 0.1% [02:33] 1-0.001^1000 chance of not seeing one [02:34] IIRC [02:34] WTF [02:34] Now product is empty. [02:34] distribution has lots. [02:36] 1000 is the stats it keeps, not the rows it consults [02:36] to determine most common it has to consult more rows than it keeps [02:36] Oh. [02:36] Still doesn't explain it. [02:36] we're looking at the rows it keeps, and if that was random over the population we could estimate by frequency [02:37] but this is explicitly the most common values per attribute [02:37] so yeah, I dunno [02:37] And on APU it's 3 tuples for each non-NULL value. [02:39] tell me something, why does your script generate 98K policy uses [02:39] is that because all products get security ? [02:40] It currently just creates public, private, security for every pillar. [02:40] Until we have a sensible way to manage comment editing, we probably have to allow private bugs for all projects. [02:44] 4 / 1346 RootObject:+login [02:47] EXPLAIN ANALYZE SELECT person.id FROM person WHERE id IN (SELECT person FROM accesspolicygrant where policy IN (SELECT id from accesspolicyuse where distribution=1 and policy=1)) ORDER BY person_sort_key(person.displayname, person.name) LIMIT 75; [02:47] 780ms [02:50] wgrant: I think your first plan is a fluke the more I look at it [02:50] Oh? [02:50] wgrant: the indices are not in the same order [02:50] It may be a fluke, but the others are crap :) [02:50] one is person sort [02:51] the other is person [02:51] Yes. [02:51] it can't dual iterate [02:51] No. [02:52] it can? [02:52] But it happens that (in this case) the coverage is good enough that it's not a problem. [02:52] It can't, AFAIK. [02:52] double negs :) [02:52] so [02:52] I think a better question isn't 'why are the other plans slow' [02:52] its 'what do we want to actually query' [02:53] thus the query above [02:53] or [02:54] a less extreme [02:54] EXPLAIN ANALYZE SELECT person.id FROM person WHERE id IN (SELECT person FROM accesspolicygrant, accesspolicyuse WHERE accesspolicygrant.policy=accesspolicyuse.id AND distribution=1 and accesspolicyuse.policy=1) ORDER BY person_sort_key(person.displayname, person.name) LIMIT 75; [02:54] Which is 1180ms [02:57] interestingly [02:57] you realise that you are querying outside of both your partial indices [02:58] Yes. [02:58] I added a new non-partial one. [02:58] is NULL -> 4ms [02:58] Which would possibly be better done by making another one non-partial, but perhaps not. [02:58] is not NULL -> 2000ms [02:58] On what? [02:58] EXPLAIN ANALYZE SELECT DISTINCT ON (person_sort_key(person.displayname, person.name)) person.id FROM accesspolicygrant JOIN accesspolicyuse ON accesspolicyuse.id = accesspolicygrant.policy JOIN person ON person.id = accesspolicygrant.person WHERE accesspolicyuse.distribution = 1 AND accesspolicyuse.policy = 1 AND artifact is NOT NULL ORDER BY person_sort_key(person.displayname, person.name) LIMIT 75; [02:58] artifact? [02:59] Well, yes. [02:59] There are far fewer where artifact is NOT NULL [02:59] That's not so much the indices as the volume of data. [02:59] Your thing now has to sort a total of 1 person. [02:59] Which is not a significant challenge. [02:59] different plans [03:00] not unsurprising [03:01] ok so to rephrase the problem [03:01] 'there are 57K people with explicit access within Ubuntu and we want to sort them by fields only on Person' [03:01] Yes. [03:02] by definition we need to either read *all people* in sort order filtering against the set of grants - which will for random names mean we read ~ all people [03:02] or we need to read all grants (57K) and then cherry pick people [03:02] Yes. [03:02] which is much more selective and what its doing [03:02] so why is it slow? [03:03] I don't know. I wouldn't expect materialising 57k rows from such a narrow table to be slow at all. [03:03] -> Nested Loop (cost=3.39..66.16 rows=13 width=25) (actual time=14.369..1910.529 rows=57946 loops=1) [03:03] Which plan is this? [03:03] EXPLAIN ANALYZE SELECT DISTINCT ON (person_sort_key(person.displayname, person.name)) person.id FROM accesspolicygrant JOIN accesspolicyuse ON accesspolicyuse.id = accesspolicygrant.policy JOIN person ON person.id = accesspolicygrant.person WHERE accesspolicyuse.distribution = 1 AND accesspolicyuse.policy = 1 AND artifact is NOT NULL ORDER BY person_sort_key(person.displayname, person.name) LIMIT 75; [03:03] [03:03] but any of them start to look like this [03:03] eparse [03:04] explain analyze SELECT DISTINCT ON (person_sort_key(person.displayname, person.name)) person.id FROM accesspolicygrant JOIN accesspolicyuse ON accesspolicyuse.id = accesspolicygrant.policy JOIN person ON person.id = accesspolicygrant.person WHERE accesspolicyuse.distribution = 1 AND accesspolicyuse.id=98234 ORDER BY person_sort_key(person.displayname, person.name) LIMIT 75; [03:04] [03:04] is weird [03:04] What about it is weird? The plan? [03:05] yes [03:05] Hmm. [03:06] I guess the non-indexed sort of 57k people is not going to be terribly fast, because person_sort_key is PL/Python. [03:06] But I'm pretty sure it happened with a straight string sort too. [03:07] I'll just reply to your mail then test ;) [03:07] I think materialising the sort column may well help [03:07] Did anything useful come from the discussion this morning? [03:07] yes, we cleared things up [03:07] FSVO [03:07] And for now. [03:07] Hah [03:08] Does that mean I can continue with my branch to hide private teams in subteam of? [03:08] ANd probably by trebling the scope of the project. [03:08] StevenK: yes, the confusion was that that page shows super and sub [03:08] StevenK: see bug 405277 which really covers the visibility rules [03:08] <_mup_> Bug #405277: Private teams are not able to join other teams (public or private)

< https://launchpad.net/bugs/405277 > [03:09] wgrant: If it's a death march until March, what difference does December next year make? :-( [03:09] StevenK: you probably also want to hide proposed memberships from private teams, unless the private team has signed off already. [03:10] I really want to finish this branch that kills strict_component from {check,verify}Upload since it's close to done, but wgrant is ignoring me. :-P [03:10] Bah, that could have been said here rather than in PM. [03:11] You were busy here :-P [03:11] StevenK: I don't understand what you're saying. [03:11] So, there is two things [03:12] One test failure is test_checkUpload_without_strict_component. I'm concerned that is a buggy test since strict_component is dead and is possibily relying on behaviour that is removed. [03:14] Hm. It's expecting checkUpload() to pass, but it now raises NoRightsForComponent. [03:14] s/raises/returns/ [03:14] Since returning exceptions is sexy or something [03:15] It's no longer a useful or possible test. [03:16] So it should be removed. [03:17] The other test failure is from the upload processor, which is not rejecting a package that should be. [03:17] I think it is because checkUpload() is returning an exception and we do not reject because the package is NEW. [03:18] pastebin the failure [03:18] And diff [03:18] AssertionError: !=: [03:18] reference = "Not permitted to upload to the RELEASE pocket in a series in the 'CURRENT' state." [03:18] actual = '' [03:18] Ah, indeed. [03:18] So, we need to catch the no privileges exception. [03:19] Well, "catch" [03:19] Check for it. [03:19] Yes. [03:19] If checkUpload has failed due to a lack of permissions, then do the NEW check. [03:19] This strikes me as ugly [03:19] Welcome to Soyuz. [03:21] wgrant: Any exception from checkUpload is lack of perms? [03:21] No. [03:21] eg. the pocket error that you quoted [03:21] NoRightsForArchive() == lack of permissions [03:22] NoRightsForComponent() == lack of permissions [03:22] NoRightsForArchive is a lie and should probably die, or be moved into NascentUpload. [03:22] NoRightsForComponent is stupid and too specific. [03:23] CannotUploadToPPA is arguably permissions [03:23] However, I agree ArchiveDisabled is not. [03:23] It's not a matter for agreement. [03:24] So, how should this code be structured? [03:24] And do you still need to see the diff? [03:27] I would probably rework verifyUpload to raise a single exception when there are insufficient upload rights. [03:27] Except perhaps preserving NoRightsForArchive, but implementing it in a non-buggy fashion. [03:27] NoRightsForComponent has no right to exist. [03:28] Isn't it used for, say, partner? [03:29] Partner doesn't need to behave well. [03:29] As long as it vaguely works for 6 months. [03:30] Hm, partner is a seperate Archive anyway [03:30] In some ways. [03:30] It isn't Archive.id 1, is what I mean [03:30] Which means it has its own ArchivePermission entries [03:30] Yes. [03:30] But that's almost all it means. [03:32] NoRightsForComponent is only checked due to checkArchivePermission(person, component), is that check pointless too? [03:33] That would be cheating. [03:35] I don't really want to rewrite verifyUpload() in this branch [03:35] It's only like 10 lines. [03:35] It's currently 30, so that sounds like an improvment [03:36] I'm tempted to say 'prove it' :-P [03:40] wgrant: ok, the call [03:40] wgrant: so the two bits we were getting stuck/confused on were: [03:40] Person:+index shows sub and super; this was just confusion. super should not be shown. sub should always be shown. [03:40] StevenK: ^ [03:40] and bug subscriptions should always be shown too [03:41] no spying! [03:41] wallyworld_: ^ [03:41] That was my understanding. [03:42] lifeless: so we don't *ever* want to hide private subscribers? [03:42] super should not be shown because the teams a person are in should not be disclosed just because you know the person -> visibility is not granted to non-participants of a Person against their memberships. [03:43] subteams should always be shown (if you can see the team they are a member of) because the subteam could otherwise give you an invisible project owner. e.g. public project, public owning team, no members visible. But really there is a private team lurking inside the public team. [03:43] we don't want that-> we show the existence of the private team. [03:44] wallyworld_: the rule is, if you can see the bug, you can see the name, displayname, url, of its subscribers (including structural subs) [03:44] ok. but if you click on the link, it will 404? [03:44] lifeless: aaaaaaa [03:44] wallyworld_: there are a few drivers for this. One is the guiding principle that interacting with a public object discloses private objects. [03:44] lifeless: We're changing privileges based on admin status? [03:44] wallyworld_: it will give a 200 and a little details. [03:45] wallyworld_: which will require separate work. [03:45] ok,so the normal +index view with a bunch of stuff not rendered [03:45] wallyworld_: the second is that if anyone replies to the bug via mail, and they are subscribed via a private team, they are very likely to disclose the private team anyhow due to the footer we include, which 'Reply' in many modern mail clients includes. [03:45] wallyworld_: yes [03:45] lifeless: How do you propose that Person:+index knows if you have partial visibility to it? [03:46] wgrant: theres a something like 10 clause EXISTS union we need to write. [03:46] wgrant: AFAICT it should be fast. [03:46] I was about to say that that was not an answer. [03:47] only 10 clauses? surely we could try for 20 :-P [03:47] We would have to join bugsubscription->bug->accesspolicyartifact->accesspolicyuse->accesspolicygrant->teamparticipation, and that's just for one part of it. [03:48] wgrant: its not trivial unfeasible. [03:48] It is, however, trivial silly. [03:48] I have been thinking about this for a year or so [03:49] and I disagree with you :) [03:49] So. [03:49] 9 [03:49] bah [03:49] Say that one day in future we decide that being a monolithic webapp is actually a stupid idea, and SOA is cool. [03:49] how about that huh [03:50] \o/ [03:50] How does that work now? [03:50] We call out to 50 services? [03:50] in parallel, yes [03:50] ... [03:50] Like fun that scales [03:50] (and cancel the calls when one answers) [03:50] This also puts the LP services in a strongly privileged position. [03:50] StevenK: it actually scales very very well [03:51] StevenK: because each service can cache answers to these questions when appropriate [03:51] StevenK: 'can user X see object Y' is a -stock- query any service that owns data has to be able to answer efficiently [03:51] lifeless: But 50 parallel calls? Times 50 users? [03:51] StevenK: where do you get times 50 users ? [03:51] * wallyworld_ gets out the popcorn [03:51] Out of the air [03:52] StevenK: so, this is the sequence: [03:52] traversal [03:52] oh this is private [03:52] check rules [03:52] -> denied [03:52] -> full [03:52] -> restricted [03:52] where denied is a failure to meet full or restricted [03:52] full depends on the object, say for Person, it is granted by: is_member or is_owner [03:53] restricted is granted by 'common ground', so we need to find *a* object where: [03:53] user.expand_through_team_participation() can see && object subscribed-to, owns, etc. [03:54] so one call to the team membership service, to get all the teams of user [03:54] thats fast, we know it is. [03:54] and then for each data store that might hold an object the Person we are looking at owns [note we don't need TP expansion of it] [03:55] we ask for exists(discloses=Person, viewer=TP_expanded_user) [03:56] will it be dead trivial to write? Probably not. Is it beyond our current schema, or an SOA - not at all. [03:56] its also very cachable if we need to. [03:56] though that might be terrifyingly large. [04:11] wallyworld_: StevenK: I've commented on both your bugs. [04:11]