Code review comment for lp:~michael.nelson/launchpad/627957-differences-schema-update

On Mon, Sep 13, 2010 at 10:04 AM, Stuart Bishop
<email address hidden> wrote:
> Review: Approve db
> (14:43:29) stub: noodles775: versien isn't spelled that way
> (14:44:26) noodles775: hrm... "base version to derived versien." I'll update it.

Done.

> (14:45:56) stub: noodles775: So source_version and parent_source_version are debian version strings? If so we might want a CHECK constraint to ensure that.
> (14:46:40) stub: ALTER TABLE DistroSeriesDifference ADD CONSTRAINT valid_source_version CHECK (valid_debian_version(source_version));
> (14:47:02) noodles775: stub: yes they are. Thanks.

Added.

> (14:49:37) stub: noodles775: With the new UNIQUE constraint, you can drop the distroseriesdifference__derived_series__idx index.
> (14:50:25) stub: noodles775: The name of the new UNIQUE constraint should be distroseriesdifference__derived_series__source_package_name__key
> (14:51:16) noodles775: stub: Great - I didn't realised the two-column index makes the single-column foreign-key index redundant. And yep, I'll rename the constraint.

Done.

> (14:51:16) stub: So a NULL diff means it hasn't been calculated yet I assume. What do NULL versions mean? Probaly worth clarifying this in comments.sql
> (14:51:58) noodles775: stub: yes, different types of differences... (unique to the derived series, missing from the derived series etc.). Should there be a constraint to ensure that both are not null together?
> (14:52:40) noodles775: stub: er, ignore that (I was thinking about the versions fields).
> (14:52:46) stub: If that is what our code expects, then yes. ALTER TABLE DistroSeriesDifference ADD CONSTRAINT valid...
> (14:52:47) stub: ok
> (14:53:49) stub: noodles775: patch-2208-12-0.sql
> (14:54:11) noodles775: *sigh*. I need a tea. So yes, you had asked about NULL versions, so we should have a constraint to ensure both versions are not null.
> (14:54:15) noodles775: Thanks.
> (14:55:27) stub: If only one or the other can be NULL: ALTER TABLE DistroSeriesDifference ADD CONSTRAINT valid_versions CHECK (source_version IS NULL != parent_source_version IS NULL)
> (14:55:42) noodles775: Great, thanks.
> (14:56:54) stub: If we just want to check at most one is NULL: ALTER TABLE DistroSeriesDifference ADD CONSTRAINT valid_versions CHECK (coalesce(source_version, parent_source_version) IS NOT NULL)
> (14:58:39) stub: or just 'source_version IS NOT NULL OR parent_source_version IS NOT NULL' is better
> (14:58:53) noodles775: Yep.
> (14:58:55) stub: I'm not sure which of those two rules is appropriate
> 15:00
> (15:00:22) stub: Can they both be NULL? Can they both be NOT NULL?
> (15:00:48) noodles775: They cannot both be null, but they can both be NOT NULL.
> (15:02:49) stub: ok... so CHECK (source_version IS NOT NULL OR parent_source_version IS NOT NULL)

After thinking a bit more about this, I didn't add the constraint to
stop both the versions being NULL, after realising that it would be a
valid way to resolve a difference that was unique to the derived
series. (ie. if resolving a difference for a package that is unique to
the derived series means deleting that version from the derived
series).

Incremental http://pastebin.ubuntu.com/492997/