Merge ~pappacena/launchpad:db-patch-ociproject-project-pillar into launchpad:db-devel

[Thiago F. Pappacena]

Adding OCIProject.product database column and adjusting the way GitRepository table references OCIProject.

[Colin Watson (cjwatson)]

I would move this patch to 2210-08-8; it might as well be part of the overall OCI series, as long as we haven't yet reached 9 and had to overflow to another minor number.

There are several other things we need to add:

 * Rename OCIProject.product to OCIProject.project. While we still use the term "product" in anything old (partly because there's a lot of code to change, and partly because renaming DB columns/tables is awkward), everything new uses the term "project" instead. Compare GitRepository.project.

 * Add something like this to the `ALTER TABLE OCIProject` statement so that it's actually possible to create an OCIProject row based on a project:
   ALTER COLUMN distribution DROP NOT NULL,
   ADD CONSTRAINT one_container CHECK ((project IS NULL) != (distribution IS NULL))

 * Add a unique index on `OCIProject (project, ociprojectname) WHERE project IS NOT NULL`.

 * Change the GitRepository one_container constraint to allow `(project IS NOT NULL AND distribution IS NULL AND sourcepackagename IS NULL AND ociprojectname IS NOT NULL)`.

 * Alter most of the GitRepository project indexes involving (project) and (owner, project) to add a `WHERE ociprojectname IS NULL` condition, and add parallel indexes for the `ociprojectname IS NOT NULL` case. This is similar to 2210-08-1 and 2210-08-2, which between them do a similar sort of operation to add (distribution, ociprojectname) alongside (distribution, sourcepackagename).
  * Note that there are complexities here because creating indexes on non-trivial tables is generally too slow to be done during a fastdowntime deployment. You'll need the same kind of structure that you see in 2210-08-1 and 2210-08-2: the initial cold patch should rename the relevant existing indexes to add an `old__` prefix; the follow-up hot patch should create new indexes and drop the old ones. These must not be in the same MP; the hot patch should be proposed against master from a branch that doesn't include the cold patch. We'll need to apply the hot patch manually to production with `CONCURRENTLY` shortly after the fastdowntime, and sync up master later.

review: Needs Fixing

[Thiago F. Pappacena (pappacena)]

cjwatson, I think this MP is good for another round of review.

I also created another MP (pointing to master) here, with statements that we should run as "cold patch" with CONCURRENTLY: https://code.launchpad.net/~pappacena/launchpad/+git/launchpad/+merge/383897

[William Grant (wgrant)]

The proposed GitRepository change is not without controversy. I realise I approved something analogous in https://code.launchpad.net/~cjwatson/launchpad/+git/launchpad/+merge/374672, but I think it's worth discussing. Are we sure we want to use GitRepository(project, ociprojectname) rather than GitRepository(ociproject)?

DistributionSourcePackage started off weird because the DistributionSourcePackage table (DistributionSourcePackageInDatabase is the Storm model) was added many years after the DistributionSourcePackage virtual object, to store a couple of metadata fields. That's why they're generally referenced in the DB as (distribution, sourcepackagename) -- for most older tables there was no DistributionSourcePackage to add a foreign key to. This makes renames, moves, etc. impossible, which is somewhat OK because it's not really possible to rename source packages anyway, but that's not something we necessarily want to replicate.

Storing the tuple in GitRepository does make finding all the repositories on a pillar easier, but there are other ways to do that search efficiently. It's also much less clear how we'd implement more generalised subprojects using the tuple model.

[Thiago F. Pappacena (pappacena)]

I agree that the database would be strongly simplified if we have GitRepository(ociproject).

It will probably require some refactoring and data migration, but I guess it worth it. I'll take a look on what should be changed codewise to adapt to it.

Do you see some downside on having GitRepository(ociproject), cjwatson?

[Thiago F. Pappacena (pappacena)]

Changed GitRepository table to deprecate ociprojectname column and create a new oci_project column.

As discussed with cjwatson, this column is referenced by the code today (so it cannot be dropped until code changes reaches production) but it shouldn't be in use just yet. Therefore, we don't need to migrate any data.

I have also updated the related MP (https://code.launchpad.net/~pappacena/launchpad/+git/launchpad/+merge/383897) to create indexes for oci_project, and those we will need to run with CONCURRENTLY.

[Colin Watson (cjwatson)]

review: Approve

[Colin Watson (cjwatson)]

It might be a good idea to update the commit message to mention something about GitRepository as well, if you can do so concisely.

[Thiago F. Pappacena (pappacena)]

Pushed the requested changes. I'll land this, and we need to apply https://code.launchpad.net/~pappacena/launchpad/+git/launchpad/+merge/384670 manually.

[William Grant (wgrant)]

Vanilla ADD CONSTRAINT on a table with non-trivial data isn't generally permitted, as it's too slow. It should be preferred to create the constraint with NOT VALID and then VALIDATE it in a hot patch later.

GitRepository may be small enough that we can get away with it here, but we should confirm on staging.

[Thiago F. Pappacena (pappacena)]

wgrant, I'm creating a new MP to add the NOT VALID, and updating the hot patch to run the VALIDATE.

- MP to add NOT VALID: https://code.launchpad.net/~pappacena/launchpad/+git/launchpad/+merge/385005
- Hot patch update: https://code.launchpad.net/~pappacena/launchpad/+git/launchpad/+merge/384670

This branch got merged, but since it's just a CHECK constraint validation, there shouldn't be any problem if it was already applied somewhere (ALTER TABLE ... VALIDATE is idempotent).

diff --git a/database/schema/patch-2210-08-8.sql b/database/schema/patch-2210-08-8.sql
new file mode 100644
index 0000000..dffbaac
--- /dev/null
+++ b/database/schema/patch-2210-08-8.sql
@@ -0,0 +1,46 @@
+-- Copyright 2020 Canonical Ltd.  This software is licensed under the
+-- GNU Affero General Public License version 3 (see the file LICENSE).
+
+SET client_min_messages=ERROR;
+
+ALTER TABLE OCIProject ADD COLUMN project integer REFERENCES product;
+
+ALTER TABLE OCIProject
+    ALTER COLUMN distribution
+    DROP NOT NULL,
+    ADD CONSTRAINT one_container
+        CHECK ((project IS NULL) != (distribution IS NULL));
+
+COMMENT ON COLUMN OCIProject.project
+    IS 'The project that this OCI project is associated with.';
+
+CREATE UNIQUE INDEX ociproject__project__ociprojectname__key
+    ON OCIProject (project, ociprojectname) WHERE project IS NOT NULL;
+
+
+-- Alter GitRepository table to allow oci_project.
+COMMENT ON COLUMN GitRepository.ociprojectname
+    IS 'Deprecated column. Check one_container and default_implies_target constraints before removing.';
+
+ALTER TABLE GitRepository
+    ADD COLUMN oci_project integer REFERENCES ociproject,
+    DROP CONSTRAINT one_container,
+    ADD CONSTRAINT one_container CHECK (
+        -- Distribution + OCIProjectName, to keep compatibility temporarily
+        (project IS NULL AND distribution IS NOT NULL AND sourcepackagename IS NULL AND ociprojectname IS NOT NULL) OR
+        -- Project
+        (project IS NOT NULL AND distribution IS NULL AND sourcepackagename IS NULL AND oci_project IS NULL AND ociprojectname IS NULL) OR
+        -- Distribution source package
+        (project IS NULL AND distribution IS NOT NULL AND sourcepackagename IS NOT NULL AND oci_project IS NULL AND ociprojectname IS NULL) OR
+        -- OCI project
+        (project IS NULL AND distribution IS NULL AND sourcepackagename IS NULL AND oci_project IS NOT NULL AND ociprojectname IS NULL) OR
+        -- Personal
+        (project IS NULL AND distribution IS NULL AND sourcepackagename IS NULL AND oci_project IS NULL)),
+    DROP CONSTRAINT default_implies_target,
+    ADD CONSTRAINT default_implies_target CHECK (
+        project IS NOT NULL
+        OR distribution IS NOT NULL
+        OR oci_project IS NOT NULL
+        OR (NOT owner_default AND NOT target_default));
+
+INSERT INTO LaunchpadDatabaseRevision VALUES (2210, 8, 8);