[03:21] <wgrant> StevenK: That's going to issue several queries for each filter
[04:01] <StevenK> wgrant: I'm not sure I can do it in one query given the number of tables involved
[04:04] <StevenK> wgrant: But suggestions welcome
[04:11] <StevenK> wgrant: DB patch with ON DELETE CASCADE?
[04:11] <wgrant> StevenK: I'd add a batch filter deletion method
[04:14] <StevenK> wgrant: So you're okay with multiple queries, say 4, but not 4 * number of filters ?
[04:15] <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:28] <StevenK> wgrant: http://pastebin.ubuntu.com/5682621/
[04:30] <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:41] <StevenK> wgrant: http://pastebin.ubuntu.com/5682645/
[04:42] <wgrant> StevenK: Right
[04:48] <StevenK> wgrant: The MP has updated
[04:56] <wgrant> StevenK: What about the structsub deletion behaviour in BSF.delete()? That was probably a misfeature, but it may need to be replaced.
[04:57] <StevenK> wgrant: No tests fail with it removed
[04:58] <StevenK> And I agree about it being a misfeature
[04:59] <StevenK> Well, bin/test -vvt structuralsubscription doesn't show any failures
[04:59] <wgrant> StevenK: No tests fail, but does the UI work?
[05:00] <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:01] <StevenK> wgrant: I can create a structsub
[05:01] <StevenK> And change the filter
[05:03] <StevenK> And delete both filters and create a new one
[05:03] <StevenK> (Using Person:+structural-subscriptions)
[05:10] <wgrant> StevenK: Yeah, but we'd have the odd behaviour that you could have a sub without a filter, which probably does nothing
[05:11] <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:12] <StevenK> Create a new filter
[05:16] <StevenK> wgrant: http://people.canonical.com/~stevenk/structsub.jpg
[05:20] <wgrant> StevenK: But does the UI lie?
[05:21] <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:22] <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:23] <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:24] <wgrant> +s-s predates BSF
[05:25] <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:32] <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:33] <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:38] <StevenK> wgrant: http://pastebin.ubuntu.com/5682717/
[05:41] <wgrant> StevenK: Possibly, but I'd prefer to discover what the model constraints actually are
[05:46] <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:48] <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:50] <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:51] <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:52] <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
[06:00] <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:01] <wgrant> Oh, also it doesn't actually do anything
[06:02] <StevenK> wgrant: Oh?
[06:02] <StevenK> Well, delete doesn't have to call into deleteMultiple
[06:03] <wgrant> StevenK: deleteMultiple deletes all BSFs, then if delete_self then it deletes all BSFs.
[06:04] <StevenK> wgrant: Not exactly. The filters aren't deleted, the internals are
[06:05] <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:06] <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:07] <StevenK> wgrant: Right. But deleting the structsub calls deleteMultiple directly
[06:09] <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:10] <StevenK> wgrant: Right, and then delete calls that. Do I need to call _has_other_filters for locking?
[06:11] <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:21] <StevenK> wgrant: Bah, now I'm having trouble crafting a query to find the structsubs with no filters
[06:24] <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:25] <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:26] <wgrant> Ah, no, it will work
[06:26] <wgrant> Because of the backward compat changes that they made late.
[06:28] <StevenK> wgrant: http://pastebin.ubuntu.com/5682802/ I'm on the right track?
[06:38] <wgrant> StevenK: That sort of thing
[06:38] <wgrant> However
[06:39] <wgrant> I've just realised that the original code isn't as safe as it thinks it is.
[06:45] <StevenK> wgrant: If can change that SELECT FOR UPDATE to return id rather than *?
[06:46] <wgrant> StevenK: That won't affect the locking.
[06:46] <wgrant> It still won't do what the author expected.
[06:47] <StevenK> wgrant: Even the query you pasted above?
[06:48] <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:50] <wgrant> Yes, but to what end?
[06:50] <StevenK> So they don't change under us?
[06:51] <wgrant> Not quite
[06:52] <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:53] <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:54] <wgrant> So it fails to prevent the race
[06:57] <StevenK> wgrant: So what can we do instead?
[07:04] <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:06] <wgrant> I don't think there's another way to obtain a lock before the snapshot is frozen.
[07:07] <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:17] <wgrant> Oh
[07:17] <wgrant> Or we just select the BSFs for SHARE
[07:20] <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:25] <wgrant> StevenK: SELECT id FROM structuralsubscription WHERE id IN (4) AND EXISTS (SELECT 1 FROM bugsubscriptionfilter WHERE bugsubscriptionfilter.structuralsubscription = structuralsubscription.id FOR SHARE);
[07:26] <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:39] <StevenK> wgrant: Ah, but you're missing one thing -- we have the BSF ids, not the SS ids.
[07:40] <wgrant> StevenK: It is not a huge issue to obtain the SS IDs
[23:24] <StevenK> wgrant: http://pastebin.ubuntu.com/5685381/ Branch from yesterday, against -r submit: for readability
[23:44] <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:45] <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.