Merge lp:~michael.nelson/launchpad/distro-series-difference-schema into lp:launchpad/db-devel

[Michael Nelson]

Add DistroSeriesDifference table and related DistroSeriesDifferenceMessage.

Overview
=======
This branch is a schema patch to support displaying packages with different versions between two distroseries (specifically, a derived distroseries and its parent series).

Details of the UI at:
https://dev.launchpad.net/LEP/DerivativeDistributions

Details
=====
Some details regarding the implementation and schema design were discussed on a long-ish email thread:

https://lists.launchpad.net/launchpad-dev/msg04408.html

The decisions that came out of that discussion were:

1) Lets just store the derived series which references its parent series (rather than generalising it with two references to distroseries - we can always generalise later if and when we need to).
2) Store the source_package_name, rather than two references to SPPHs, as the SPPH references can be out of date (ie. a newer version could be published before the record is updated again). We'll have properties on the model which will always look up the latest SPPHs based on the distroseries's and source package name.
3) The actual diff off the two packages is not required (nullable), and will be generated on request. Also, once its created, its not necessarily a diff of the current packages (new versions could be uploaded since the diff was generated). Hence naming it latest_package_diff rather than simply package_diff.
4) The activity_log field was initially a comment field, but it will be used and appended to (1) by scripts, when new uploads are detected that change the state, or (2) when users add comments (eg. "We're waiting for version 1.4"), hence renaming it activity_log. I don't see a reason to add a separate comment model - but you might.
5) Initially we had just one enum for the type of difference (UNIQUE_TO_DERIVED_SERIES, MISSING_FROM_DERIVED_SERIES, DIFFERENT_VERSIONS) so that the view can query for all the records of a certain type. We were planning on having an 'ignored' flag, but after the above email discussion, it became apparent that other options would be required (IGNORED_ALWAYS, RESOLVED etc.), so the status column was added to support this enum instead of the ignored bool.

No sampledata changes.

The following branch adds the basic model code:
https://code.edge.launchpad.net/~michael.nelson/launchpad/distro-series-difference-basic-model/+merge/33885

[Stuart Bishop (stub)]

Its surprising you don't want delete permissions. How does the derived distribution sync its changes up with its parent distribution?

Having a column called derived_series is confusing. The derived distro series is the child. Having it reference its parent via a column called derived_series seems wrong (The derived distro series is derived from the derived distro series, which is silly). What is the parent distribution of a derived distribution called anyway? Parent or is there some other term?

We should add an index on last_package_diff for garbage collection purposes.

patch-2208-07-0.sql

review: Needs Information (db)

[Stuart Bishop (stub)]

Confused myself. This is all fine. The difference references the derived distro series it is for. The derived distro series references its parent distro series.

Add the index and you are good to go from my pov.

review: Approve (db)

[Michael Nelson (michael.nelson)]

On Fri, Aug 27, 2010 at 12:03 PM, Stuart Bishop
<email address hidden> wrote:
> Review: Approve db
> Confused myself. This is all fine. The difference references the derived distro series it is for. The derived distro series references its parent distro series.
>
> Add the index and you are good to go from my pov.

Thanks Stuart. Based on our conversation [1] I've updated to remove
the activity_log column and instead added a joining table for messages
for the DistroSeriesDifference (r9690, incremental attached). I've
based it on the similar BugMessage table, but please check it in case
I've missed something.

[1] 12:04 < noodles775> stub: Why would we want delete permissions? At
the moment I'm planning that these differences will end in a resolved
state, but we wouldn't want to delete them until the derived
distroseries is itself deleted (if it ever is).
12:05 < stub> The derived distribution has a patched version of
firefox. Tomorrow it decides that it should just use the parents
version of firefox.
12:07 < noodles775> Great, so assuming the parents version has a
higher revno, they upload it, this record is marked as resolved and
the activity log is updated, but the record is not deleted, as
tomorrow the parent might upload a new version, and the previous
comments/activity is still useful.
12:07 < stub> ok
12:08 < stub> How big do you think the comments and activity will grow?
12:08 < stub> It will be a pain to split out into a structured format later.
12:10 < stub> If it is append only, storing it as a single text blob
isn't ideal. If it is more a whiteboard or a copy of a text file
stored in a branch, that would be fine I guess.
12:10 < noodles775> On average 4 or 5 lines. If a particular
difference goes through resolved->needs attention-> resolved it would
be a few more.
12:10 < stub> But over time
12:10 < noodles775> Yes, it is just like a whiteboard.
12:11 < stub> ok
12:11 < noodles775> Yep, so if overtime it went through that cycle
(resolved->needs attention->resolved) *lots*, there would be 2 lines
for each change.
12:11 < noodles775> Do you think its worth adding a separate comment model?
12:12 < noodles775> (from memory the code/bugs guys made the comment
model re-usable... I'll check).
12:14 < noodles775> stub: oh, and I think I misunderstood your comment
above. I am making the activity_log append only.
12:17 < stub> So we would probably be better off with a separate table
containing the activity, one row per addition. Or even a link table
between distroseriesdifference and message same as our other comment
spools.
12:18 < noodles775> Yep, just looking at the message table... I'll do that then.
12:18 < noodles775> Thanks stub.
12:19 < stub> That gives you the infrastructure for attachments, email
interfaces etc. if we want them in the future.
12:20 < noodles775> Yeah, given that the table already exists, it
sounds crazy not to use it.

[Brad Crittenden (bac)]

review: Approve (code)

[Robert Collins (lifeless)]

Michael, 'latest_package_diff' implies 'the newest' but as you say
uploads could be made that mean its not the latest diff after all.

I haven't looked closely yet, but it seems to me that when you
generate a diff you know the versions you generated it against, and
you should store those, then it *is* a package_diff, not a 'latest_'
one, and we can query easily to see if it is or isn't the latest one.

[Michael Nelson (michael.nelson)]

Hi Robert,

Yes, those versions are stored with the PackageDiff object itself, but do you mean that we should store the exact versions additionally with this DistroSeriesDifference? We were doing that originally, but I'd switched to store simply the source package name as a view displaying these records would need to check and update them anyway, so it I thought it saner to have properties on the model that lookup the latest published version (cheap when we already know *which* packages have differences). Here's the email msg where I brought this up:

https://lists.launchpad.net/launchpad-dev/msg04454.html

And yep, I was using 'latest' in the sense that it is the most recently generated package diff, and irrespective of the above, we could rename this simply to package_diff and automatically remove it when it no longer matches the current published versions - I'd not thought of that.

I'll update latest_package_diff -> package_diff, but let me know about the first issue (storing of exact versions within DistroSeriesDifference in addition to PackageDiff).

Cheers,
M.

[Stuart Bishop (stub)]

Drop the UNIQUE (distro_series_difference, message) constraint, and declare the message column UNIQUE instead.

Drop the index on the message column - one will get created implicitly when the column is declared UNIQUE.

[Michael Nelson (michael.nelson)]

On Mon, Aug 30, 2010 at 9:41 AM, Stuart Bishop
<email address hidden> wrote:
> Drop the UNIQUE (distro_series_difference, message) constraint, and declare the message column UNIQUE instead.
>
> Drop the index on the message column - one will get created implicitly when the column is declared UNIQUE.

Thanks Stuart. Done in the incremental (as well as renaming
last_package_diff->package_diff).

=== modified file 'database/schema/comments.sql'
--- database/schema/comments.sql	2010-08-25 16:25:02 +0000
+++ database/schema/comments.sql	2010-08-30 09:12:45 +0000
@@ -503,6 +503,19 @@
 COMMENT ON COLUMN DistributionSourcePackageCache.changelog IS 'A concatenation of the source package release changelogs for this source package, where the status is not REMOVED.';
 COMMENT ON COLUMN DistributionSourcePackageCache.archive IS 'The archive where the source is published.';
 
+-- DistroSeriesDifference
+COMMENT ON TABLE DistroSeriesDifference IS 'A difference of versions for a package in a derived distroseries and its parent distroseries.';
+COMMENT ON COLUMN DistroSeriesDifference.derived_series IS 'The derived distroseries with the difference from its parent.';
+COMMENT ON COLUMN DistroSeriesDifference.source_package_name IS 'The name of the source package which is different in the two series.';
+COMMENT ON COLUMN DistroSeriesDifference.package_diff IS 'The most recent package diff that was created for this difference.';
+COMMENT ON COLUMN DistroSeriesDifference.status IS 'A distroseries difference can be needing attention, ignored or resolved.';
+COMMENT ON COLUMN DistroSeriesDifference.difference_type IS 'The type of difference that this record represents - a package unique to the derived series, or missing, or in both.';
+
+-- DistroSeriesDifferenceMessage
+COMMENT ON TABLE DistroSeriesDifferenceMessage IS 'A message/comment on a distro series difference.';
+COMMENT ON COLUMN DistroSeriesDifferenceMessage.distro_series_difference IS 'The distro series difference for this comment.';
+COMMENT ON COLUMN DistroSeriesDifferenceMessage.message IS 'The comment for the distro series difference.';
+
 -- DistroSeriesPackageCache
 
 COMMENT ON TABLE DistroSeriesPackageCache IS 'A cache of the text associated with binary packages in the distroseries. This table allows for fast queries to find a binary packagename that matches a given text.';
@@ -533,13 +546,13 @@
 can be changed without restarting Launchpad
 <https://dev.launchpad.net/LEP/FeatureFlags>';
 
-COMMENT ON COLUMN FeatureFlag.scope IS 
+COMMENT ON COLUMN FeatureFlag.scope IS
     'Scope in which this setting is active';
 
-COMMENT ON COLUMN FeatureFlag.priority IS 
+COMMENT ON COLUMN FeatureFlag.priority IS
     'Higher priority flags override lower';
 
-COMMENT ON COLUMN FeatureFlag.flag IS 
+COMMENT ON COLUMN FeatureFlag.flag IS
     'Name of the flag being controlled';
 
 -- KarmaCategory

=== added file 'database/schema/patch-2208-07-0.sql'
--- database/schema/patch-2208-07-0.sql	1970-01-01 00:00:00 +0000
+++ database/schema/patch-2208-07-0.sql	2010-08-30 09:12:45 +0000
@@ -0,0 +1,24 @@
+SET client_min_messages=ERROR;
+
+CREATE TABLE DistroSeriesDifference (
+    id serial PRIMARY KEY,
+    derived_series integer NOT NULL CONSTRAINT distroseriesdifference__derived_series__fk REFERENCES distroseries,
+    source_package_name integer NOT NULL CONSTRAINT distroseriesdifference__source_package_name__fk REFERENCES sourcepackagename,
+    package_diff integer CONSTRAINT distroseriesdifference__package_diff__fk REFERENCES packagediff,
+    status integer NOT NULL,
+    difference_type integer NOT NULL
+);
+CREATE INDEX distroseriesdifference__derived_series__idx ON distroseriesdifference(derived_series);
+CREATE INDEX distroseriesdifference__source_package_name__idx ON distroseriesdifference(source_package_name);
+CREATE INDEX distroseriesdifference__status__idx ON distroseriesdifference(status);
+CREATE INDEX distroseriesdifference__difference_type__idx ON distroseriesdifference(difference_type);
+CREATE INDEX distroseriesdifference__package_diff__idx ON distroseriesdifference(package_diff);
+
+CREATE TABLE DistroSeriesDifferenceMessage(
+    id serial PRIMARY KEY,
+    distro_series_difference integer NOT NULL CONSTRAINT distroseriesdifferencemessage__distro_series_difference__fk REFERENCES distroseriesdifference,
+    message integer NOT NULL CONSTRAINT distroseriesdifferencemessage__message__fk REFERENCES message UNIQUE
+);
+CREATE INDEX distroseriesdifferencemessage__distroseriesdifference__idx ON distroseriesdifferencemessage(distro_series_difference);
+
+INSERT INTO LaunchpadDatabaseRevision VALUES (2208, 07, 0);

=== modified file 'database/schema/security.cfg'
--- database/schema/security.cfg	2010-08-27 18:49:22 +0000
+++ database/schema/security.cfg	2010-08-30 09:12:45 +0000
@@ -134,6 +134,8 @@
 public.distributionmirror               = SELECT, INSERT, UPDATE, DELETE
 public.distributionsourcepackage        = SELECT, INSERT, UPDATE, DELETE
 public.distributionsourcepackagecache   = SELECT
+public.distroseriesdifference           = SELECT, INSERT, UPDATE
+public.distroseriesdifferencemessage    = SELECT, INSERT, UPDATE
 public.distroserieslanguage             = SELECT, INSERT, UPDATE
 public.distroseriespackagecache         = SELECT
 public.emailaddress                     = SELECT, INSERT, UPDATE, DELETE