launchpad-reviewers team mailing list archive

["Thiago F. Pappacena" <pappacena@xxxxxxxxx>]

I think all comments were addressed here.

I'll open another MP with index creation and CHECK constraints validations on existing tables, so we run it separately (and with CONCURRENT on index creation).

Diff comments:

> diff --git a/database/schema/patch-2210-26-1.sql b/database/schema/patch-2210-26-1.sql
> new file mode 100644
> index 0000000..5511d91
> --- /dev/null
> +++ b/database/schema/patch-2210-26-1.sql
> @@ -0,0 +1,132 @@
> +-- Copyright 2021 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 Snap
> +    ADD COLUMN information_type integer,
> +    ADD COLUMN project integer REFERENCES product,

Ok! I'll add in a separated patch together with other index creations, so we can run it concurrently.

> +    ADD COLUMN access_policy integer,

It doesn't seems to have a FK on GitRepository (which is the pattern I'm following here): https://pastebin.canonical.com/p/b2bmD2889X/

> +    ADD COLUMN access_grants integer[];
> +
> +COMMENT ON COLUMN Snap.private IS
> +    '(DEPRECATED; use Snap.information_type) Whether or not this snap is private.';
> +COMMENT ON COLUMN Snap.project IS
> +    'The project which is the pillar for this Snap';
> +COMMENT ON COLUMN Snap.information_type IS
> +    'Enum describing what type of information is stored, such as type of private or security related data, and used to determine to apply an access policy.';
> +
> +CREATE TABLE SnapSubscription (
> +    id serial PRIMARY KEY,
> +    person integer NOT NULL REFERENCES Person(id),
> +    snap integer NOT NULL REFERENCES Snap(id),

Ok! Changing it.

> +    date_created timestamp without time zone DEFAULT (CURRENT_TIMESTAMP AT TIME ZONE 'UTC') NOT NULL,
> +    subscribed_by integer NOT NULL REFERENCES Person(id)

Right! Adding it.

> +);
> +
> +COMMENT ON TABLE SnapSubscription IS 'Person subscription for Snap recipes.';
> +COMMENT ON COLUMN SnapSubscription.person IS
> +    'The person who subscribed to the Snap.';
> +COMMENT ON COLUMN SnapSubscription.snap IS
> +    'The Snap recipe to which the person subscribed.';
> +COMMENT ON COLUMN SnapSubscription.date_created IS
> +    'When the subscription was created.';
> +COMMENT ON COLUMN SnapSubscription.subscribed_by IS
> +    'The person performing the action of subscribing someone to the Snap.';
> +
> +
> +CREATE UNIQUE INDEX snapsubscription__person_snap__key
> +    ON SnapSubscription(snap, person);

Right! I'll back port the `skip` from the code branch.

> +
> +CREATE INDEX snapsubscription__person__idx
> +    ON SnapSubscription(person);
> +
> +ALTER TABLE AccessArtifact
> +    ADD COLUMN snap integer REFERENCES snap;
> +
> +CREATE UNIQUE INDEX accessartifact__snap__key
> +    ON AccessArtifact(snap) WHERE snap IS NOT NULL;
> +
> +
> +ALTER TABLE AccessArtifact DROP CONSTRAINT has_artifact;
> +ALTER TABLE AccessArtifact
> +    ADD CONSTRAINT has_artifact CHECK (
> +    (null_count(ARRAY[bug, branch, gitrepository, snap, specification]) = 4));

I'll create this with "NOT VALID" here, and move the check validation to another branch, so we can run it outside the db upgrade cycle.

> +
> +
> +CREATE OR REPLACE FUNCTION snap_denorm_access(snap_id integer)
> +    RETURNS void LANGUAGE plpgsql AS
> +$$
> +DECLARE
> +    info_type integer;
> +BEGIN
> +    -- XXX pappacena 2021-002-12: Once we finish filling "information_type" and

hehehe. Oops...

> +    -- deprecate the usage of "public" column at code level, we will be able to
> +    -- drop the "private" column usage here.
> +    SELECT
> +        CASE snap.information_type
> +            WHEN NULL THEN
> +                -- information type: 1 = public; 5 = proprietary
> +                CASE WHEN snap.private THEN 5 ELSE 1 END
> +            ELSE
> +                snap.information_type
> +            END
> +    INTO info_type
> +    FROM snap WHERE id = snap_id;

I agree it would improve readability. Changing it.

> +
> +    UPDATE Snap
> +        SET access_policy = policies[1], access_grants = grants
> +        FROM
> +            build_access_cache(
> +                (SELECT id FROM accessartifact WHERE snap = snap_id),
> +                info_type)
> +            AS (policies integer[], grants integer[])
> +        WHERE id = snap_id;
> +END;
> +$$;
> +
> +CREATE OR REPLACE FUNCTION accessartifact_denorm_to_artifacts(artifact_id integer)
> +    RETURNS void
> +    LANGUAGE plpgsql
> +    AS $$
> +DECLARE
> +    artifact_row accessartifact%ROWTYPE;
> +BEGIN
> +    SELECT * INTO artifact_row FROM accessartifact WHERE id = artifact_id;
> +    IF artifact_row.bug IS NOT NULL THEN
> +        PERFORM bug_flatten_access(artifact_row.bug);
> +    END IF;
> +    IF artifact_row.branch IS NOT NULL THEN
> +        PERFORM branch_denorm_access(artifact_row.branch);
> +    END IF;
> +    IF artifact_row.gitrepository IS NOT NULL THEN
> +        PERFORM gitrepository_denorm_access(artifact_row.gitrepository);
> +    END IF;
> +    IF artifact_row.snap IS NOT NULL THEN
> +        PERFORM snap_denorm_access(artifact_row.snap);
> +    END IF;
> +    IF artifact_row.specification IS NOT NULL THEN
> +        PERFORM specification_denorm_access(artifact_row.specification);
> +    END IF;
> +    RETURN;
> +END;
> +$$;
> +
> +COMMENT ON FUNCTION accessartifact_denorm_to_artifacts(artifact_id integer) IS
> +    'Denormalize the policy access and artifact grants to bugs, branches, git repositories, snaps, and specifications.';
> +
> +-- A trigger to handle snap.{private,information_type,project} changes.
> +CREATE OR REPLACE FUNCTION snap_maintain_access_cache_trig() RETURNS trigger
> +    LANGUAGE plpgsql AS $$
> +BEGIN
> +    PERFORM snap_denorm_access(NEW.id);
> +    RETURN NULL;
> +END;
> +$$;
> +
> +CREATE TRIGGER snap_maintain_access_cache
> +    AFTER INSERT OR UPDATE OF private, information_type, project ON Snap
> +    FOR EACH ROW EXECUTE PROCEDURE snap_maintain_access_cache_trig();
> +
> +
> +INSERT INTO LaunchpadDatabaseRevision VALUES (2210, 26, 1);


-- 
https://code.launchpad.net/~pappacena/launchpad/+git/launchpad/+merge/397459
Your team Launchpad code reviewers is subscribed to branch ~pappacena/launchpad:snap-pillar-db.