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