launchpad-reviewers team mailing list archive

[Colin Watson <mp+416727@xxxxxxxxxxxxxxxxxx>]

I think I've addressed all your comments in one way or another.  Please re-review.

Diff comments:

> diff --git a/database/schema/patch-2210-44-0.sql b/database/schema/patch-2210-44-0.sql
> new file mode 100644
> index 0000000..bf5e78c
> --- /dev/null
> +++ b/database/schema/patch-2210-44-0.sql
> @@ -0,0 +1,106 @@
> +-- Copyright 2022 Canonical Ltd.  This software is licensed under the
> +-- GNU Affero General Public License version 3 (see the file LICENSE).
> +
> +SET client_min_messages=ERROR;
> +
> +-- STEP 1, COLD
> +
> +ALTER TABLE Archive
> +    -- 0 == LOCAL
> +    ADD COLUMN publishing_method integer DEFAULT 0,
> +    -- 0 == DEBIAN
> +    ADD COLUMN index_format integer DEFAULT 0;
> +
> +ALTER TABLE SourcePackageRelease
> +    ADD COLUMN ci_build integer REFERENCES CIBuild,
> +    ADD CONSTRAINT at_most_one_build CHECK (
> +        null_count(ARRAY[sourcepackage_recipe_build, ci_build]) >= 1),
> +    ALTER COLUMN component DROP NOT NULL,
> +    ALTER COLUMN section DROP NOT NULL,
> +    ALTER COLUMN urgency DROP NOT NULL,
> +    ALTER COLUMN dsc_format DROP NOT NULL,
> +    ADD CONSTRAINT debian_columns CHECK (
> +        -- 1 == DPKG
> +        format != 1
> +        OR (component IS NOT NULL
> +            AND section IS NOT NULL
> +            AND urgency IS NOT NULL
> +            AND dsc_format IS NOT NULL)),
> +    -- This is always non-NULL for Debian-format source packages, but it's
> +    -- not particularly important to constrain this at the DB level.
> +    ALTER COLUMN maintainer DROP NOT NULL;
> +
> +ALTER TABLE SourcePackagePublishingHistory
> +    ADD COLUMN format integer,
> +    ALTER COLUMN component DROP NOT NULL,
> +    ALTER COLUMN section DROP NOT NULL,
> +    ADD CONSTRAINT debian_columns CHECK (
> +        (format IS NOT NULL
> +            -- 1 == DPKG
> +            AND format != 1)
> +        OR (component IS NOT NULL
> +            AND section IS NOT NULL)),
> +    ADD COLUMN channel jsonb;
> +
> +ALTER TABLE BinaryPackageRelease
> +    ADD COLUMN ci_build integer REFERENCES CIBuild,
> +    ALTER COLUMN build DROP NOT NULL,
> +    ADD CONSTRAINT one_build CHECK (null_count(ARRAY[build, ci_build]) = 1),
> +    ALTER COLUMN component DROP NOT NULL,
> +    ALTER COLUMN section DROP NOT NULL,
> +    ALTER COLUMN priority DROP NOT NULL,
> +    ADD CONSTRAINT debian_columns CHECK (
> +        -- 1 == DEB, 2 == UDEB, 5 == DDEB
> +        binpackageformat NOT IN (1, 2, 5)
> +        OR (component IS NOT NULL
> +            AND section IS NOT NULL
> +            AND priority IS NOT NULL));
> +
> +ALTER TABLE BinaryPackagePublishingHistory
> +    ADD COLUMN binarypackageformat integer,
> +    ALTER COLUMN component DROP NOT NULL,
> +    ALTER COLUMN section DROP NOT NULL,
> +    ALTER COLUMN priority DROP NOT NULL,
> +    ADD CONSTRAINT debian_columns CHECK (
> +        (binarypackageformat IS NOT NULL
> +            -- 1 == DEB, 2 == UDEB, 5 == DDEB
> +            AND binarypackageformat NOT IN (1, 2, 5))
> +        OR (component IS NOT NULL
> +            AND section IS NOT NULL
> +            AND priority IS NOT NULL)),
> +    ADD COLUMN channel jsonb;
> +
> +
> +-- Subsequent statements, to be executed live and in subsequent patches.
> +
> +/*
> +-- STEP 2, HOT
> +
> +CREATE INDEX sourcepackagerelease__ci_build__idx
> +    ON SourcePackageRelease (ci_build);
> +
> +CREATE INDEX sourcepackagepublishinghistory__channel__idx
> +    ON SourcePackagePublishingHistory (channel);

This was mostly due to observing the existing SPPH(pocket) index, which similarly perhaps doesn't make a whole lot of sense.  I can't think of a good reason to have any variant of this at present (publication is mostly going to rely on SPPH(archive, status), and existing indexes should be good enough for domination even if they aren't perfectly selective), so I've just deleted the SPPH(channel) index and its BPPH(channel) sibling; we can always add it later if we need to.

> +
> +CREATE UNIQUE INDEX binarypackagerelease__bpn__build__version__key
> +    ON BinaryPackageRelease (
> +        binarypackagename, COALESCE(build, ci_build), version);

https://pastebin.canonical.com/p/xs8JS64mbm/ bears out the theory that it's unused, so I've dropped it.

> +CREATE UNIQUE INDEX binarypackagerelease__build__bpn__key
> +    ON BinaryPackageRelease (COALESCE(build, ci_build), binarypackagename);
> +CREATE INDEX binarypackagerelease__ci_build__idx
> +    ON BinaryPackageRelease (ci_build);
> +
> +CREATE INDEX binarypackagepublishinghistory__channel__idx
> +    ON BinaryPackagePublishingHistory (channel);
> +
> +
> +-- STEP 3, COLD
> +
> +-- Replaced by binarypackagerelease__bpn__build__version__key and
> +-- binarypackagerelease__build__bpn__key respectively.
> +ALTER TABLE BinaryPackageRelease
> +    DROP CONSTRAINT binarypackagerelease_binarypackagename_key,
> +    DROP CONSTRAINT binarypackagerelease_build_name_uniq;
> +*/
> +
> +INSERT INTO LaunchpadDatabaseRevision VALUES (2210, 44, 0);


-- 
https://code.launchpad.net/~cjwatson/launchpad/+git/launchpad/+merge/416727
Your team Launchpad code reviewers is requested to review the proposed merge of ~cjwatson/launchpad:db-artifactory-publish into launchpad:db-devel.