← Back to team overview

launchpad-reviewers team mailing list archive

Re: [Merge] ~pappacena/launchpad:oci-bug-add-oci-columns into launchpad:db-devel

 


Diff comments:

> diff --git a/database/schema/patch-2210-22-0.sql b/database/schema/patch-2210-22-0.sql
> new file mode 100644
> index 0000000..e1dd712
> --- /dev/null
> +++ b/database/schema/patch-2210-22-0.sql
> @@ -0,0 +1,488 @@
> +-- 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;
> +
> +-- This patch can be applied "cold", in a "fast-downtime" way. It basically
> +-- adds the new OCI database columns, and rename current indexes so they can be
> +-- removed and replaced with new ones that includes the new columns.
> +
> +ALTER TABLE BugTask
> +    ADD COLUMN ociproject integer REFERENCES ociproject,
> +    ADD COLUMN ociprojectseries integer REFERENCES ociprojectseries,
> +    DROP CONSTRAINT bugtask_assignment_checks,
> +    ADD CONSTRAINT bugtask_assignment_checks CHECK (
> +        CASE
> +            WHEN product IS NOT NULL THEN productseries IS NULL AND distribution IS NULL AND distroseries IS NULL AND sourcepackagename IS NULL
> +            WHEN productseries IS NOT NULL THEN distribution IS NULL AND distroseries IS NULL AND sourcepackagename IS NULL AND ociproject IS NULL AND ociprojectseries IS NULL
> +            WHEN distribution IS NOT NULL THEN distroseries IS NULL
> +            WHEN distroseries IS NOT NULL THEN ociproject IS NULL AND ociprojectseries IS NULL
> +            WHEN ociproject IS NOT NULL THEN ociprojectseries IS NULL AND (distribution IS NOT NULL OR product IS NOT NULL)
> +            WHEN ociprojectseries IS NOT NULL THEN ociproject IS NULL AND (distribution IS NOT NULL OR product IS NOT NULL)
> +            ELSE false
> +        END) NOT VALID;
> +
> +ALTER INDEX bugtask_distinct_sourcepackage_assignment
> +    RENAME TO old__bugtask_distinct_sourcepackage_assignment;
> +
> +ALTER INDEX bugtask__product__bug__key
> +    RENAME TO old__bugtask__product__bug__key;
> +
> +ALTER TABLE BugTaskFlat
> +    ADD COLUMN ociproject integer,
> +    ADD COLUMN ociprojectseries integer;
> +
> +ALTER TABLE BugSummary
> +    ADD COLUMN ociproject integer REFERENCES ociproject,
> +    ADD COLUMN ociprojectseries integer REFERENCES ociprojectseries,
> +    DROP CONSTRAINT bugtask_assignment_checks,
> +    ADD CONSTRAINT bugtask_assignment_checks CHECK (
> +        CASE
> +            WHEN product IS NOT NULL THEN productseries IS NULL AND distribution IS NULL AND distroseries IS NULL AND sourcepackagename IS NULL
> +            WHEN productseries IS NOT NULL THEN distribution IS NULL AND distroseries IS NULL AND sourcepackagename IS NULL AND ociproject IS NULL AND ociprojectseries IS NULL
> +            WHEN distribution IS NOT NULL THEN distroseries IS NULL
> +            WHEN distroseries IS NOT NULL THEN ociproject IS NULL AND ociprojectseries IS NULL
> +            WHEN ociproject IS NOT NULL THEN ociprojectseries IS NULL AND (distribution IS NOT NULL OR product IS NOT NULL)
> +            WHEN ociprojectseries IS NOT NULL THEN ociproject IS NULL AND (distribution IS NOT NULL OR product IS NOT NULL)
> +            ELSE false
> +        END) NOT VALID;
> +
> +ALTER INDEX bugsummary__unique
> +    RENAME TO old__bugsummary__unique;
> +
> +ALTER TABLE BugSummaryJournal
> +    ADD COLUMN ociproject integer,
> +    ADD COLUMN ociprojectseries integer;
> +
> +-- Functions
> +
> +CREATE OR REPLACE FUNCTION bugtask_flatten(task_id integer, check_only boolean)
> +    RETURNS boolean
> +    LANGUAGE plpgsql SECURITY DEFINER SET search_path = public
> +    AS $$
> +DECLARE
> +    bug_row Bug%ROWTYPE;
> +    task_row BugTask%ROWTYPE;
> +    old_flat_row BugTaskFlat%ROWTYPE;
> +    new_flat_row BugTaskFlat%ROWTYPE;
> +    _product_active boolean;
> +    _access_policies integer[];
> +    _access_grants integer[];
> +BEGIN
> +    -- This is the master function to update BugTaskFlat, but there are
> +    -- maintenance triggers and jobs on the involved tables that update
> +    -- it directly. Any changes here probably require a corresponding
> +    -- change in other trigger functions.
> +
> +    SELECT * INTO task_row FROM BugTask WHERE id = task_id;
> +    SELECT * INTO old_flat_row FROM BugTaskFlat WHERE bugtask = task_id;
> +
> +    -- If the task doesn't exist, ensure that there's no flat row.
> +    IF task_row.id IS NULL THEN
> +        IF old_flat_row.bugtask IS NOT NULL THEN
> +            IF NOT check_only THEN
> +                DELETE FROM BugTaskFlat WHERE bugtask = task_id;
> +            END IF;
> +            RETURN FALSE;
> +        ELSE
> +            RETURN TRUE;
> +        END IF;
> +    END IF;
> +
> +    SELECT * FROM bug INTO bug_row WHERE id = task_row.bug;
> +
> +    -- If it's a product(series) task, we must consult the active flag.
> +    IF task_row.product IS NOT NULL THEN
> +        SELECT product.active INTO _product_active
> +            FROM product WHERE product.id = task_row.product LIMIT 1;
> +    ELSIF task_row.productseries IS NOT NULL THEN
> +        SELECT product.active INTO _product_active
> +            FROM
> +                product
> +                JOIN productseries ON productseries.product = product.id
> +            WHERE productseries.id = task_row.productseries LIMIT 1;
> +    END IF;
> +
> +    SELECT policies, grants
> +        INTO _access_policies, _access_grants
> +        FROM bug_build_access_cache(bug_row.id, bug_row.information_type)
> +            AS (policies integer[], grants integer[]);
> +
> +    -- Compile the new flat row.
> +    SELECT task_row.id, bug_row.id, task_row.datecreated,
> +           bug_row.duplicateof, bug_row.owner, bug_row.fti,
> +           bug_row.information_type, bug_row.date_last_updated,
> +           bug_row.heat, task_row.product, task_row.productseries,
> +           task_row.distribution, task_row.distroseries,
> +           task_row.sourcepackagename, task_row.status,
> +           task_row.importance, task_row.assignee,
> +           task_row.milestone, task_row.owner,
> +           COALESCE(_product_active, TRUE),
> +           _access_policies,
> +           _access_grants,
> +           bug_row.latest_patch_uploaded, task_row.date_closed,
> +           task_row.ociproject, task_row.ociprojectseries
> +           INTO new_flat_row;
> +
> +    -- Calculate the necessary updates.
> +    IF old_flat_row.bugtask IS NULL THEN
> +        IF NOT check_only THEN
> +            INSERT INTO BugTaskFlat VALUES (new_flat_row.*);
> +        END IF;
> +        RETURN FALSE;
> +    ELSIF new_flat_row != old_flat_row THEN
> +        IF NOT check_only THEN
> +            UPDATE BugTaskFlat SET
> +                bug = new_flat_row.bug,
> +                datecreated = new_flat_row.datecreated,
> +                duplicateof = new_flat_row.duplicateof,
> +                bug_owner = new_flat_row.bug_owner,
> +                fti = new_flat_row.fti,
> +                information_type = new_flat_row.information_type,
> +                date_last_updated = new_flat_row.date_last_updated,
> +                heat = new_flat_row.heat,
> +                product = new_flat_row.product,
> +                productseries = new_flat_row.productseries,
> +                distribution = new_flat_row.distribution,
> +                distroseries = new_flat_row.distroseries,
> +                sourcepackagename = new_flat_row.sourcepackagename,
> +                status = new_flat_row.status,
> +                importance = new_flat_row.importance,
> +                assignee = new_flat_row.assignee,
> +                milestone = new_flat_row.milestone,
> +                owner = new_flat_row.owner,
> +                active = new_flat_row.active,
> +                access_policies = new_flat_row.access_policies,
> +                access_grants = new_flat_row.access_grants,
> +                date_closed = new_flat_row.date_closed,
> +                latest_patch_uploaded = new_flat_row.latest_patch_uploaded,
> +                ociproject = new_flat_row.ociproject,
> +                ociprojectseries = new_flat_row.ociprojectseries
> +                WHERE bugtask = new_flat_row.bugtask;
> +        END IF;
> +        RETURN FALSE;
> +    ELSE
> +        RETURN TRUE;
> +    END IF;
> +END;
> +$$;
> +
> +
> +CREATE OR REPLACE FUNCTION bug_summary_inc(d bugsummary) RETURNS VOID
> +LANGUAGE plpgsql AS
> +$$
> +BEGIN
> +    -- Shameless adaption from postgresql manual
> +    LOOP
> +        -- first try to update the row
> +        UPDATE BugSummary SET count = count + d.count
> +        WHERE
> +            ((product IS NULL AND $1.product IS NULL)
> +                OR product = $1.product)
> +            AND ((productseries IS NULL AND $1.productseries IS NULL)
> +                OR productseries = $1.productseries)
> +            AND ((distribution IS NULL AND $1.distribution IS NULL)
> +                OR distribution = $1.distribution)
> +            AND ((distroseries IS NULL AND $1.distroseries IS NULL)
> +                OR distroseries = $1.distroseries)
> +            AND ((sourcepackagename IS NULL AND $1.sourcepackagename IS NULL)
> +                OR sourcepackagename = $1.sourcepackagename)
> +            AND ((ociproject IS NULL AND $1.ociproject IS NULL)
> +                OR ociproject = $1.ociproject)
> +            AND ((ociprojectseries IS NULL AND $1.ociprojectseries IS NULL)
> +                OR ociprojectseries = $1.ociprojectseries)
> +            AND ((viewed_by IS NULL AND $1.viewed_by IS NULL)
> +                OR viewed_by = $1.viewed_by)
> +            AND ((tag IS NULL AND $1.tag IS NULL)
> +                OR tag = $1.tag)
> +            AND status = $1.status
> +            AND ((milestone IS NULL AND $1.milestone IS NULL)
> +                OR milestone = $1.milestone)
> +            AND importance = $1.importance
> +            AND has_patch = $1.has_patch
> +            AND access_policy IS NOT DISTINCT FROM $1.access_policy;
> +        IF found THEN
> +            RETURN;
> +        END IF;
> +        -- not there, so try to insert the key
> +        -- if someone else inserts the same key concurrently,
> +        -- we could get a unique-key failure
> +        BEGIN
> +            INSERT INTO BugSummary(
> +                count, product, productseries, distribution,
> +                distroseries, sourcepackagename,
> +                ociproject, ociprojectseries,
> +                viewed_by, tag,
> +                status, milestone, importance, has_patch, access_policy)
> +            VALUES (
> +                d.count, d.product, d.productseries, d.distribution,
> +                d.distroseries, d.sourcepackagename,
> +                d.ociproject, d.ociprojectseries,
> +                d.viewed_by, d.tag,
> +                d.status, d.milestone, d.importance, d.has_patch,
> +                d.access_policy);
> +            RETURN;
> +        EXCEPTION WHEN unique_violation THEN
> +            -- do nothing, and loop to try the UPDATE again
> +        END;
> +    END LOOP;
> +END;
> +$$;
> +
> +
> +CREATE OR REPLACE FUNCTION bugsummary_rollup_journal(batchsize integer=NULL)
> +RETURNS VOID
> +LANGUAGE plpgsql VOLATILE
> +CALLED ON NULL INPUT
> +SECURITY DEFINER SET search_path TO public AS
> +$$
> +DECLARE
> +    d bugsummary%ROWTYPE;
> +    max_id integer;
> +BEGIN
> +    -- Lock so we don't content with other invokations of this
> +    -- function. We can happily lock the BugSummary table for writes
> +    -- as this function is the only thing that updates that table.
> +    -- BugSummaryJournal remains unlocked so nothing should be blocked.
> +    LOCK TABLE BugSummary IN ROW EXCLUSIVE MODE;
> +
> +    IF batchsize IS NULL THEN
> +        SELECT MAX(id) INTO max_id FROM BugSummaryJournal;
> +    ELSE
> +        SELECT MAX(id) INTO max_id FROM (
> +            SELECT id FROM BugSummaryJournal ORDER BY id LIMIT batchsize
> +            ) AS Whatever;
> +    END IF;
> +
> +    FOR d IN
> +        SELECT
> +            NULL as id,
> +            SUM(count),
> +            product,
> +            productseries,
> +            distribution,
> +            distroseries,
> +            sourcepackagename,
> +            viewed_by,
> +            tag,
> +            status,
> +            milestone,
> +            importance,
> +            has_patch,
> +            access_policy,
> +            ociproject,
> +            ociprojectseries
> +        FROM BugSummaryJournal
> +        WHERE id <= max_id
> +        GROUP BY
> +            product, productseries, distribution, distroseries,
> +            sourcepackagename, ociproject, ociprojectseries,
> +            viewed_by, tag, status, milestone,
> +            importance, has_patch, access_policy
> +        HAVING sum(count) <> 0
> +    LOOP
> +        IF d.count < 0 THEN
> +            PERFORM bug_summary_dec(d);
> +        ELSIF d.count > 0 THEN
> +            PERFORM bug_summary_inc(d);
> +        END IF;
> +    END LOOP;
> +
> +    -- Clean out any counts we reduced to 0.
> +    DELETE FROM BugSummary WHERE count=0;
> +    -- Clean out the journal entries we have handled.
> +    DELETE FROM BugSummaryJournal WHERE id <= max_id;
> +END;
> +$$;
> +
> +
> +CREATE OR REPLACE FUNCTION bugtask_maintain_bugtaskflat_trig()
> +    RETURNS trigger
> +    LANGUAGE plpgsql SECURITY DEFINER
> +    SET search_path TO 'public'
> +    AS $$
> +BEGIN
> +    IF TG_OP = 'INSERT' THEN
> +        PERFORM bugtask_flatten(NEW.id, FALSE);
> +    ELSIF TG_OP = 'UPDATE' THEN
> +        IF NEW.bug != OLD.bug THEN
> +            RAISE EXCEPTION 'cannot move bugtask to a different bug';
> +        ELSIF (NEW.product IS DISTINCT FROM OLD.product
> +            OR NEW.productseries IS DISTINCT FROM OLD.productseries) THEN
> +            -- product.active may differ. Do a full update.
> +            PERFORM bugtask_flatten(NEW.id, FALSE);
> +        ELSIF (
> +            NEW.datecreated IS DISTINCT FROM OLD.datecreated
> +            OR NEW.product IS DISTINCT FROM OLD.product
> +            OR NEW.productseries IS DISTINCT FROM OLD.productseries
> +            OR NEW.distribution IS DISTINCT FROM OLD.distribution
> +            OR NEW.distroseries IS DISTINCT FROM OLD.distroseries
> +            OR NEW.sourcepackagename IS DISTINCT FROM OLD.sourcepackagename
> +            OR NEW.ociproject IS DISTINCT FROM OLD.ociproject
> +            OR NEW.ociprojectseries IS DISTINCT FROM OLD.ociprojectseries
> +            OR NEW.status IS DISTINCT FROM OLD.status
> +            OR NEW.importance IS DISTINCT FROM OLD.importance
> +            OR NEW.assignee IS DISTINCT FROM OLD.assignee
> +            OR NEW.milestone IS DISTINCT FROM OLD.milestone
> +            OR NEW.owner IS DISTINCT FROM OLD.owner
> +            OR NEW.date_closed IS DISTINCT FROM OLD.date_closed) THEN
> +            -- Otherwise just update the columns from bugtask.
> +            -- Access policies and grants may have changed due to target
> +            -- transitions, but an earlier trigger will already have
> +            -- mirrored them to all relevant flat tasks.
> +            UPDATE BugTaskFlat SET
> +                datecreated = NEW.datecreated,
> +                product = NEW.product,
> +                productseries = NEW.productseries,
> +                distribution = NEW.distribution,
> +                distroseries = NEW.distroseries,
> +                sourcepackagename = NEW.sourcepackagename,
> +                ociproject = NEW.ociproject,
> +                ociprojectseries = NEW.ociprojectseries,
> +                status = NEW.status,
> +                importance = NEW.importance,
> +                assignee = NEW.assignee,
> +                milestone = NEW.milestone,
> +                owner = NEW.owner,
> +                date_closed = NEW.date_closed
> +                WHERE bugtask = NEW.id;
> +        END IF;
> +    ELSIF TG_OP = 'DELETE' THEN
> +        PERFORM bugtask_flatten(OLD.id, FALSE);
> +    END IF;
> +    RETURN NULL;
> +END;
> +$$;
> +
> +DROP FUNCTION bugsummary_targets;
> +CREATE OR REPLACE FUNCTION bugsummary_targets(btf_row public.bugtaskflat)
> +    RETURNS TABLE(
> +        product integer,
> +        productseries integer,
> +        distribution integer,
> +        distroseries integer,
> +        sourcepackagename integer,
> +        ociproject integer,
> +        ociprojectseries integer
> +    )
> +    LANGUAGE sql IMMUTABLE
> +    AS $_$
> +    -- Include a sourcepackagename-free task if this one has a
> +    -- sourcepackagename, so package tasks are also counted in their
> +    -- distro/series.
> +    -- XXX what about pillar for ociprojects?

We need to resolve this XXX comment before landing this.  The plan was to denormalise the pillar onto BugTaskFlat, so this should have non-NULL product or distribution if ociproject is non-NULL.  In order to have good statistics for the containing pillar, I think we need to similarly include an ociproject-free summary row.

But possibly I'm misunderstanding something here, because in https://code.launchpad.net/~pappacena/launchpad/+git/launchpad/+merge/393570 your bugsummary.txt changes do seem to suggest that you create an ociproject-free summary row.  Can you explain what's going on there?  At any rate, the XXX should be resolved one way or another.

> +    SELECT
> +        $1.product, $1.productseries, $1.distribution,
> +        $1.distroseries, $1.sourcepackagename,
> +        $1.ociproject, $1.ociprojectseries
> +    UNION -- Implicit DISTINCT
> +    SELECT
> +        $1.product, $1.productseries, $1.distribution,
> +        $1.distroseries, NULL,
> +        $1.ociproject, $1.ociprojectseries;
> +$_$;
> +
> +CREATE OR REPLACE FUNCTION bugsummary_locations(
> +        btf_row bugtaskflat, tags text[])
> +    RETURNS SETOF bugsummaryjournal
> +    LANGUAGE plpgsql
> +    AS $$
> +BEGIN
> +    IF btf_row.duplicateof IS NOT NULL THEN
> +        RETURN;
> +    END IF;
> +    RETURN QUERY
> +        SELECT
> +            CAST(NULL AS integer) AS id,
> +            CAST(1 AS integer) AS count,
> +            bug_targets.product, bug_targets.productseries,
> +            bug_targets.distribution, bug_targets.distroseries,
> +            bug_targets.sourcepackagename,
> +            bug_viewers.viewed_by, bug_tags.tag, btf_row.status,
> +            btf_row.milestone, btf_row.importance,
> +            btf_row.latest_patch_uploaded IS NOT NULL AS has_patch,
> +            bug_viewers.access_policy,
> +            bug_targets.ociproject, bug_targets.ociprojectseries
> +        FROM
> +            bugsummary_targets(btf_row) AS bug_targets,
> +            unnest(tags) AS bug_tags (tag),
> +            bugsummary_viewers(btf_row) AS bug_viewers;
> +END;
> +$$;
> +
> +CREATE OR REPLACE FUNCTION bugsummary_insert_journals(
> +        journals bugsummaryjournal[])
> +    RETURNS void
> +    LANGUAGE sql
> +    AS $$
> +    -- We sum the rows here to minimise the number of inserts into the
> +    -- journal, as in the case of UPDATE statement we may have -1s and +1s
> +    -- cancelling each other out.
> +    INSERT INTO BugSummaryJournal(
> +            count, product, productseries, distribution,
> +            distroseries, sourcepackagename, ociproject, ociprojectseries,
> +            viewed_by, tag, status, milestone, importance, has_patch,
> +            access_policy)
> +        SELECT
> +            SUM(count), product, productseries, distribution,
> +            distroseries, sourcepackagename, ociproject, ociprojectseries,
> +            viewed_by, tag, status, milestone, importance, has_patch,
> +            access_policy
> +        FROM unnest(journals)
> +        GROUP BY
> +            product, productseries, distribution,
> +            distroseries, sourcepackagename, ociproject, ociprojectseries,
> +            viewed_by, tag, status, milestone, importance, has_patch,
> +            access_policy
> +        HAVING SUM(count) != 0;
> +$$;
> +
> +
> +-- Views
> +
> +-- Combined view so we don't have to manually collate rows from both tables.
> +-- Note that we flip the sign of the id column of BugSummaryJournal to avoid
> +-- clashes. This is enough to keep Storm happy as it never needs to update
> +-- this table, and there are no other suitable primary keys.
> +-- We don't SUM() rows here to ensure PostgreSQL has the most hope of
> +-- generating good query plans when we query this view.
> +CREATE OR REPLACE VIEW CombinedBugSummary AS (
> +    SELECT
> +        id,
> +        count,
> +        product,
> +        productseries,
> +        distribution,
> +        distroseries,
> +        sourcepackagename,
> +        viewed_by,
> +        tag,
> +        status,
> +        milestone,
> +        importance,
> +        has_patch,
> +        access_policy,
> +        ociproject,
> +        ociprojectseries
> +    FROM bugsummary
> +    UNION ALL
> +    SELECT
> +        -id AS id, count,
> +        product,
> +        productseries,
> +        distribution,
> +        distroseries,
> +        sourcepackagename,
> +        viewed_by,
> +        tag,
> +        status,
> +        milestone,
> +        importance,
> +        has_patch,
> +        access_policy,
> +        ociproject,
> +        ociprojectseries
> +   FROM bugsummaryjournal
> +);
> +
> +INSERT INTO LaunchpadDatabaseRevision VALUES (2210, 22, 0);


-- 
https://code.launchpad.net/~pappacena/launchpad/+git/launchpad/+merge/393460
Your team Launchpad code reviewers is subscribed to branch ~pappacena/launchpad:oci-bug-add-oci-columns.


References