| wgrant | StevenK: That's going to issue several queries for each filter | 03:21 |
|---|---|---|
| StevenK | wgrant: I'm not sure I can do it in one query given the number of tables involved | 04:01 |
| StevenK | wgrant: But suggestions welcome | 04:04 |
| StevenK | wgrant: DB patch with ON DELETE CASCADE? | 04:11 |
| wgrant | StevenK: I'd add a batch filter deletion method | 04:11 |
| StevenK | wgrant: So you're okay with multiple queries, say 4, but not 4 * number of filters ? | 04:14 |
| wgrant | StevenK: Right, a constant number of queries is necessary | 04:15 |
| wgrant | You can't sensibly do less than one per table. | 04:15 |
| wgrant | But one per table per filter is excessive and banned. | 04:15 |
| StevenK | wgrant: http://pastebin.ubuntu.com/5682621/ | 04:28 |
| wgrant | StevenK: I'd replace the list comp and helper method with a for loop | 04:30 |
| wgrant | And you can hopefully kill off BSF.delete() now | 04:30 |
| wgrant | Or at least reimplement it in terms of the deleteMultiple | 04:30 |
| wgrant | If you can't remove it entirely | 04:30 |
| StevenK | wgrant: http://pastebin.ubuntu.com/5682645/ | 04:41 |
| wgrant | StevenK: Right | 04:42 |
| StevenK | wgrant: The MP has updated | 04:48 |
| wgrant | StevenK: What about the structsub deletion behaviour in BSF.delete()? That was probably a misfeature, but it may need to be replaced. | 04:56 |
| StevenK | wgrant: No tests fail with it removed | 04:57 |
| StevenK | And I agree about it being a misfeature | 04:58 |
| StevenK | Well, bin/test -vvt structuralsubscription doesn't show any failures | 04:59 |
| wgrant | StevenK: No tests fail, but does the UI work? | 04:59 |
| wgrant | The code appears to implement an invariant that there's always a filter for every structsub | 05:00 |
| wgrant | It implements it racily, but it's still likely that something depends on it | 05:00 |
| StevenK | wgrant: I can create a structsub | 05:01 |
| StevenK | And change the filter | 05:01 |
| StevenK | And delete both filters and create a new one | 05:03 |
| StevenK | (Using Person:+structural-subscriptions) | 05:03 |
| wgrant | StevenK: Yeah, but we'd have the odd behaviour that you could have a sub without a filter, which probably does nothing | 05:10 |
| StevenK | wgrant: The UI actually copes with that. | 05:11 |
| StevenK | Launchpad | 05:11 |
| StevenK | Bug mail is not filtered; mail for Foo Bar about Launchpad will always be sent. | 05:11 |
| StevenK | Create a new filter | 05:12 |
| StevenK | wgrant: http://people.canonical.com/~stevenk/structsub.jpg | 05:16 |
| wgrant | StevenK: But does the UI lie? | 05:20 |
| wgrant | that's probably from the migration; I'm not sure if the backend will actually cope | 05:21 |
| wgrant | AIUI it just looks for matching BSFs | 05:21 |
| StevenK | wgrant: The Launchpad structsub was created by me | 05:21 |
| StevenK | The other two are from sampledata | 05:21 |
| wgrant | StevenK: How did you create it? | 05:21 |
| StevenK | Product:+subscribe | 05:21 |
| wgrant | And you didn't manually delete the BSF? | 05:22 |
| StevenK | I did, but via Person:+structural-subscriptions | 05:22 |
| wgrant | Right, so have you tested how the backend handles it? | 05:22 |
| wgrant | I suspect that that UI code is just a leftover from the migration when not everything had a BSF yet | 05:23 |
| StevenK | wgrant: Person:+structural-subscriptions was written for structsubs only, +subscriptions is the legacy stuff | 05:23 |
| wgrant | +structural-subscriptions is legacy | 05:23 |
| wgrant | +subscriptions is not legacy | 05:23 |
| wgrant | +s-s predates BSF | 05:24 |
| wgrant | So it is not a valid argument for anything | 05:25 |
| wgrant | (structsubs date back to '07 or so; it's just BugSubscriptionFilter and co that are new) | 05:25 |
| StevenK | wgrant: If I resurrect _has_other_filters, to restore the old behaviour, I can't move delete to using deleteMultiple, since deleteMultiple makes a choice at the end if it removes itself, or its parent structsub | 05:32 |
| wgrant | StevenK: We first need to establish what the model invariants must be | 05:33 |
| wgrant | Then we can work out implementation of deletion | 05:33 |
| StevenK | wgrant: http://pastebin.ubuntu.com/5682717/ | 05:38 |
| wgrant | StevenK: Possibly, but I'd prefer to discover what the model constraints actually are | 05:41 |
| StevenK | wgrant: I've been trying to work out which code determines if a notification will be sent based on filtering to see if it will gracefully cope with no filters, but I'm having trouble finding it | 05:46 |
| wgrant | StevenK: Let me see | 05:48 |
| wgrant | I hadn't noticed before, but it's actually more careful than any other LP code to be non-racy | 05:48 |
| wgrant | So it seems to be pretty serious about maintaining that invariant. | 05:48 |
| wgrant | source = IStore(StructuralSubscription).using( | 05:50 |
| wgrant | StructuralSubscription, | 05:50 |
| wgrant | Join(BugSubscriptionFilter, | 05:50 |
| wgrant | BugSubscriptionFilter.structural_subscription_id == | 05:50 |
| wgrant | StructuralSubscription.id), | 05:50 |
| wgrant | QED | 05:50 |
| StevenK | Ah, then there is UI bug too | 05:51 |
| StevenK | The filter page allows you to delete the last filter | 05:51 |
| StevenK | Maybe that's okay since it will destroy the structsub too | 05:51 |
| wgrant | StevenK: Right, that's fine, because that's handled at the model level | 05:52 |
| wgrant | That's the whole point of the code I'm questioning the removal of | 05:52 |
| StevenK | wgrant: Then my resurrection that I pasted should be fine | 06:00 |
| wgrant | StevenK: Sort of. | 06:00 |
| wgrant | StevenK: It's an extremely confusing interface | 06:00 |
| wgrant | And very likely to go wrong | 06:00 |
| wgrant | Oh, also it doesn't actually do anything | 06:01 |
| StevenK | wgrant: Oh? | 06:02 |
| StevenK | Well, delete doesn't have to call into deleteMultiple | 06:02 |
| wgrant | StevenK: deleteMultiple deletes all BSFs, then if delete_self then it deletes all BSFs. | 06:03 |
| StevenK | wgrant: Not exactly. The filters aren't deleted, the internals are | 06:04 |
| wgrant | StevenK: Oh, I see, I misunderstood | 06:05 |
| wgrant | That's still O(len(filters)) queries | 06:05 |
| wgrant | => fail | 06:05 |
| StevenK | wgrant: How? | 06:05 |
| StevenK | We loop over the kinds, not the filters | 06:06 |
| wgrant | BSF.delete | 06:06 |
| wgrant | Calls deleteMultiple for each BSF | 06:06 |
| wgrant | Then checks for each BSF that there are other BSFs | 06:06 |
| wgrant | Then if not it deletes the structsub | 06:06 |
| StevenK | wgrant: Right. But deleting the structsub calls deleteMultiple directly | 06:07 |
| wgrant | StevenK: This seems extremely convoluted. I'd probably have a method which deletes BSFs, then deletes any corresponding SSs if they have 0 BSFs. | 06:09 |
| wgrant | Simple | 06:09 |
| wgrant | Foolproof | 06:09 |
| wgrant | Fast | 06:09 |
| wgrant | Short | 06:09 |
| wgrant | So the same as BSF.delete is today, except operating over more than one BSF | 06:09 |
| StevenK | wgrant: Right, and then delete calls that. Do I need to call _has_other_filters for locking? | 06:10 |
| wgrant | You'll want to do the same sort of locking, yes | 06:11 |
| wgrant | And Storm doesn't have support for SELECT FOR UPDATE, so you'll probably have to use SQL or define a Select derivative. | 06:11 |
| StevenK | wgrant: Bah, now I'm having trouble crafting a query to find the structsubs with no filters | 06:21 |
| wgrant | StevenK: SELECT * FROM structuralsubscription WHERE NOT EXISTS (SELECT 1 FROM bugsubscriptionfilter WHERE bugsubscriptionfilter.structuralsubscription = structuralsubscription.id) FOR UPDATE; | 06:24 |
| wgrant | With an added ID constraint to ask for just the structsubs that you've changed | 06:24 |
| wgrant | No, not quite | 06:24 |
| wgrant | Won't take enough of a lock | 06:24 |
| wgrant | Invert the NOT EXISTS | 06:25 |
| StevenK | I'm currently locking BSF only, currently | 06:25 |
| wgrant | Then do set difference in Python | 06:25 |
| wgrant | That's useless | 06:25 |
| wgrant | BSF isn't the table that we want to lock | 06:25 |
| wgrant | Also, this SELECT FOR UPDATE stuff will probably fail in pg9.3 | 06:25 |
| wgrant | Ah, no, it will work | 06:26 |
| wgrant | Because of the backward compat changes that they made late. | 06:26 |
| StevenK | wgrant: http://pastebin.ubuntu.com/5682802/ I'm on the right track? | 06:28 |
| wgrant | StevenK: That sort of thing | 06:38 |
| wgrant | However | 06:38 |
| wgrant | I've just realised that the original code isn't as safe as it thinks it is. | 06:39 |
| StevenK | wgrant: If can change that SELECT FOR UPDATE to return id rather than *? | 06:45 |
| wgrant | StevenK: That won't affect the locking. | 06:46 |
| wgrant | It still won't do what the author expected. | 06:46 |
| StevenK | wgrant: Even the query you pasted above? | 06:47 |
| wgrant | StevenK: Even that query. | 06:48 |
| wgrant | Do you understand what it tries to do? | 06:48 |
| StevenK | It tries to lock all structsub rows given a bunch of filter ids | 06:48 |
| wgrant | Yes, but to what end? | 06:50 |
| StevenK | So they don't change under us? | 06:50 |
| wgrant | Not quite | 06:51 |
| StevenK | wgrant: What then? | 06:52 |
| wgrant | StevenK: It is trying to block multiple concurrent BSF.delete()s for the same SS | 06:52 |
| wgrant | Not to prevent an FK violation | 06:52 |
| wgrant | But to prevent two concurrent deletions from deleting the last two BSFs for an SS, with neither transaction noticing that it's the last. | 06:52 |
| === tasdomas_afk is now known as tasdomas | ||
| wgrant | Except that, because the SS row isn't actually being updated, the transactions can run concurrently as long as one commits before the other does the SELECT FOR UPDATE. | 06:53 |
| wgrant | So it fails to prevent the race | 06:54 |
| StevenK | wgrant: So what can we do instead? | 06:57 |
| wgrant | StevenK: I think the only completely safe method is to do an actual UPDATE, even if it changes nothing | 07:04 |
| wgrant | Or we could update the UI to say that something with no filters does nothing | 07:04 |
| wgrant | And then get garbo to prune them daily | 07:04 |
| wgrant | I don't think there's another way to obtain a lock before the snapshot is frozen. | 07:06 |
| wgrant | Yeah, that shouldn't be possible, because you'd need a snapshot to find the tuples that you need to lock | 07:07 |
| wgrant | So you can't obtain the lock before snapshotting | 07:07 |
| wgrant | Oh | 07:17 |
| wgrant | Or we just select the BSFs for SHARE | 07:17 |
| wgrant | And of course I totally misread the code because it uses UPDATE when it actually wants SHARE. | 07:20 |
| wgrant | So it is safe. | 07:20 |
| wgrant | StevenK: SELECT id FROM structuralsubscription WHERE id IN (4) AND EXISTS (SELECT 1 FROM bugsubscriptionfilter WHERE bugsubscriptionfilter.structuralsubscription = structuralsubscription.id FOR SHARE); | 07:25 |
| wgrant | The FOR SHARE in the subquery will prevent a concurrent transaction from updating or deleting the BSFs that we use as evidence for keeping the SS | 07:26 |
| StevenK | wgrant: Ah, but you're missing one thing -- we have the BSF ids, not the SS ids. | 07:39 |
| wgrant | StevenK: It is not a huge issue to obtain the SS IDs | 07:40 |
| === Ursinha-afk is now known as Ursinha | ||
| === wedgwood_away is now known as wedgwood | ||
| === tasdomas is now known as tasdomas_afk | ||
| === Ursinha_ is now known as Ursinha | ||
| === Ursinha is now known as Ursinha-afk | ||
| === Ursinha-afk is now known as Ursinha | ||
| === cinerama_ is now known as cinerama | ||
| StevenK | wgrant: http://pastebin.ubuntu.com/5685381/ Branch from yesterday, against -r submit: for readability | 23:24 |
| wgrant | StevenK: I'd grab the IDs from the FOR SHARE query, and use them to exclude SSs from deletion, rather than reselecting them potentially differently. Also, you shouldn't be using sqlvalues any more. | 23:44 |
| wgrant | Otherwise I think that's good | 23:45 |
| wgrant | I've got to run out for a few hours now; will be back lunchish. | 23:45 |
| === wedgwood is now known as wedgwood_away | ||
Generated by irclog2html.py 2.7 by Marius Gedminas - find it at mg.pov.lt!
