NotOneError caused by duplicate stuctural subscriptions

This issues was probable caused by the deletion of a team. It may released to bug 663947.

That page is not one that we are actively pursuing for our squad's feature work, but I suspect that it will still affect us on pages that we do care about, such as https://launchpad.net/webian/trunk/+subscriptions (some or all of this functionality is feature flagged, in addition to having the link to the page feature-flagged). That page does not oops for me, but it might in some circumstances, and we certainly did not expect to have to handle this case generally.

We should eliminate dupes with some SQL, and disallow these sorts of duplicate subscriptions in the database if at all possible. Then workflows that cause this situation to occur will make an OOPS that we can fix in the proper place.

Perhaps the duplication is inherent - e.g. structurally subscribe to
project group Foo, and to project Bar; then someone adds Bar to group
Foo - instant duplication, and /very/ hard to prevent even with
triggers.

Would using .first() be sufficient?

On Apr 6, 2011, at 9:16 PM, Robert Collins wrote:

> Perhaps the duplication is inherent - e.g. structurally subscribe to
> project group Foo, and to project Bar; then someone adds Bar to group
> Foo - instant duplication, and /very/ hard to prevent even with
> triggers.

That's interesting.

I was expecting this code to use the .__helper's .join, not the .args. According to the comments, the .args are for construction, while the .join is for finding matches.

I was further expecting this code to be about getting exact matches--that is, I expected that Curtis was describing a situation in which a given principal has two subscriptions to the exact same thing, not some kind of superset subscription as you describe. We have no evidence that this particular OOPS is one or the other, so far, AFAIK. Some db exploration would presumably answer that.

It's worth noting that the code we use for our own pages does use the helper's .join, rather than the .args, for filtering, which maybe is why it does not fall over.

So I'm left with these questions:

 * why does .getSubscriptions use the helper's .args, and not the .join?
 * is the situation here a subscription to an object and a subscription to its superset, as you describe, which I think we can handle; or is it two subscriptions to the exact same thing, which I would hope we already disallow at a low level?

Depending on the answers to these questions, this bug might not be that we need to solve for our story, though it is one that ought to be solved generally no matter what.

Gary

The URL I provided illustrated how I first encountered the problem. OOPS-1923M325 happened over the API. Lp is creating data that other parts of the app consider an integrity error.

I looked into this on staging. The specific example from the OOPS was not there, presumably because it had not yet made it over there yet from the production database. However, I constructed generic queries to look for duplicate structural subscriptions of all types.

https://pastebin.canonical.com/45871/

(The lines beginning with "CORRECT" are artifacts of me doublechecking my queries with the joins in the appropriate IStructuralSubscriptionTargetHelper implementations.)

As you can see in the pastebin, the staging database has dupes for Distributions, Products, ProjectGroups, and DistributionSourcePackages.

Note that this is not a scenario in which someone has a subscription that subsumes another. As Curtis says, I believe that LP is creating data that other parts of the app consider an integrity error, and that this is some concrete evidence of it.

We do not know what process is causing this.

Graham and I agree that this is what we would like to do.

1. clean up the data.
2. add no-dupe constraints in the DB for each of the seven types of StructuralSubscriptions
3. Wait for the OOPSes to roll in (if any)
4. Fix the problems in the tree if 3.

Robert, Stuart, could you please comment on this--the plan in particular? If you give approval to go forward with this, we might need help with building the constraints; I'm not sure yet.

I see a couple of idiosyncracies here.

Firstly,

    "sourcepackagename_requires_distribution" CHECK (sourcepackagename
IS NULL OR distribution IS NOT NULL)

in bug target constraints we permit distroseries + sourcepackagename:

    "bugtask_assignment_checks" CHECK (
CASE
    WHEN product IS NOT NULL THEN productseries IS NULL AND
distribution IS NULL AND distroseries IS NULL AND sourcepackagename IS
NULL
    WHEN productseries IS NOT NULL THEN distribution IS NULL AND
distroseries IS NULL AND sourcepackagename IS NULL
    WHEN distribution IS NOT NULL THEN distroseries IS NULL
    WHEN distroseries IS NOT NULL THEN true
    ELSE false
END)

So, this suggests a mismatch between structure and how folk may want
to subscribe. (Specifically, why do we let someone say 'natty only
please' or 'ubuntu python-subunit only please' but not 'natty
python-subunit ony please'.

I don't think this affects your issue, but perhaps a separate bug
should be filed?

Similarly:

    "one_target" CHECK (null_count(ARRAY[product, productseries,
project, distroseries, distribution, milestone]) = 5)

assumes that a subscription to a milestone is interesting *across all
of LP*, which is only true because milestone ids are not milestone
names: this may be a problem in the future [subscribe to the milestone
'foo' of the launchpad-project group, for instance]. Again, I think
this is separate bug worth a note in the bug tracker. [can't subscribe
to a milestone of a project-group].

---------------
SELECT StructuralSubscription.project, StructuralSubscription.subscriber
FROM StructuralSubscription
WHERE StructuralSubscription.project IS NOT NULL
GROUP BY StructuralSubscription.project, StructuralSubscription.subscriber
HAVING Count(*)>1;
 project | subscriber
---------+------------
      82 | 2
(1 row)
CORRECT (ProjectGroup)

Hah - this is me ;).
More data shows an interesting story:

 22643 | | | 82 | |
| | | 2 | 2 |
2010-07-09 05:59:22.012246 | 2010-07-09 05:59:22.012246
 22644 | | | 82 | |
| | | 2 | 2 |
2010-07-09 05:59:23.001104 | 2010-07-09 05:59:23.001104

Thats double-form submission or something, 1 second apart [under our
replication lag possibly].

I modified this query;
SELECT StructuralSubscription.product,
StructuralSubscription.subscriber, count(*)
FROM StructuralSubscription
WHERE StructuralSubscription.product IS NOT NULL
GROUP BY StructuralSubscription.product, StructuralSubscription.subscriber
HAVING Count(*)>1;
to see the frequency - also 2.

 SELECT *
FROM StructuralSubscription
WHERE product in (9868, 2461, 7534, 24395, 7533, 8269) and subscriber
in (3388985, 2212151, 1814750, 3391740, 1814750, 242763) order by
product, id;

shows a couple of dupes with multi day gaps, but others with subminute
gaps. One in particular caught my eye:

 25131 | 9868 | | | |
| | | 3388985 | 3388985 |
2010-09-16 12:37:32.187706 | 2010-09-16 12:37:32.187706
 25132 | 9868 | | | |
...

bug 769048 tracks the first separate concern Robert raised. bug 769050 tracks the second.

From Robert's comment 7 above:

"""
So I concur that:
 - there are duplicate rows
 - we should clean them up
 - there is a missing unique index - needs to be built across target +
sourcepackagename + subscriber

And probably redundantly, I'll note that
distro X, package N, subscriber Y
distro X, package M, subscriber Y
does not count as a dupe.
That is - the only dupes we have are on project and product subscriptions.
"""

It looks like cleaning up the data should be relatively straightforward. Then we can add a unique index across the fields Robert mentions. I would then use "bzr grep 'CONSTRAINT' --include='patch-*.sql' database/schema/" to find examples of adding the Product and Project constraints. Maybe we could have a single patch that cleans the data up and then adds the constraints.

However, I don't yet understand Robert's last observation, as I quoted above. I think it means that the last example in https://pastebin.canonical.com/45871/ does not indicate a problem, but I don't see why not. The last example seems valid to me. If I run a query with one of the results of the grouping, I get duplicates that look bad to me:

------
SELECT * FROM StructuralSubscription WHERE distribution = 1 AND sourcepackagename = 56261 AND subscriber = 674500;
------
  id | product | productseries | project | milestone | distribution | distroseries | sourcepackagename | subscriber | subscribed_by | date_created | date_last_updated
------+---------+---------------+---------+-----------+--------------+--------------+-------------------+------------+---------------+----------------------------+----------------------------
 8... | | | | | 1 | | 56261 | 674500 | 674500 | 2008-08-14 08:51:47.149622 | 2008-08-14 08:51:47.149622
 7... | | | | | 1 | | 56261 | 674500 | 674500 | 2008-07-07 14:58:54.969935 | 2008-07-07 14:58:54.969935
(2 rows)

AFAICT, then, I think we have dupes for Products, Projects, *and* DistributionSourcePackages. I'd need to get clarification from Robert on why we shouldn't consider the above a dupe.

A last note: in my pastebin, I made some sort of copy and paste error. Staging does not show any duplicates for distributions. That is:

"""lpmain_staging=> SELECT StructuralSubscription.distribution, StructuralSubscription.subscriber
lpmain_staging-> FROM StructuralSubscription
lpmain_staging-> WHERE StructuralSubscription.distribution IS NOT NULL AND StructuralSubscription.sourcepackagename IS NULL
lpmain_staging-> GROUP BY StructuralSubscription.distribution, StructuralSubscription.subscriber
lpmain_staging-> HAVING Count(*)>1;
 distribution | subscriber
--------------+------------
(0 rows)
"""

Therefore, there's not proof of a problem there.

> The last example seems valid to me. If I run a
> query with one of the results of the grouping, I get duplicates that
>look bad to me:
>
>------
>SELECT * FROM StructuralSubscription WHERE distribution = 1 AND sourcepackagename = 56261 AND subscriber = 674500;

Thats showing a dupe, because the sourcepackagename is the same; I was saying
distro D spn N, subscriber S
and
distro D spn M, subscriber S
N !=M
is not a dupe because its a subscription to a different package.

-Rob

OK, cool, thanks for the clarification Rob. AFAIK we do currently handle that correctly, and certainly, yes, we'll need to make sure that the new constraints continue to allow that.

Furthermore, to be clear,

distro D spn NULL subscriber S

needs to be allowed, because that is a distro subscription (but a second "distro D spn NULL subscriber S" subscription would be a dupe; we don't have any of those now).

Fixed in db-stable r10481 <http://bazaar.launchpad.net/~launchpad-pqm/launchpad/db-stable/revision/10481>.