launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #32687
Re: [Merge] ~enriqueesanchz/launchpad:db-bugtask-packagetype-channel-repositories into launchpad:db-devel
Took a while to review, but looking good!
Left a few questions and a couple of suggestions.
I really appreciate the organized formatting.
Diff comments:
> diff --git a/database/schema/patch-2211-43-0.sql b/database/schema/patch-2211-43-0.sql
> new file mode 100644
> index 0000000..ba94922
> --- /dev/null
> +++ b/database/schema/patch-2211-43-0.sql
> @@ -0,0 +1,579 @@
> +-- Copyright 2025 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 bugtask
> + ADD COLUMN packagetype integer,
> + ADD COLUMN channel jsonb,
> + ADD COLUMN repositories jsonb;
Rename to metadata if we follow what we mentioned in the last meeting
> +
> +ALTER INDEX bugtask_distinct_sourcepackage_assignment
> + RENAME TO old__bugtask_distinct_sourcepackage_assignment;
> +
> +-- repositories not included, not using it to search
> +ALTER TABLE bugtaskflat
> + ADD COLUMN packagetype integer,
> + ADD COLUMN channel jsonb;
> +
> +-- repositories not included, not involved to select the target
> +ALTER TABLE BugSummary
> + ADD COLUMN packagetype integer,
> + ADD COLUMN channel jsonb,
> + 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
> + AND packagetype IS NULL
> + AND channel 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
> + AND packagetype IS NULL
> + AND channel 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)
> + AND sourcepackagename IS NULL
> + AND packagetype IS NULL
> + AND channel IS NULL
> + WHEN ociprojectseries IS NOT NULL THEN
> + ociproject IS NULL
> + AND (distribution IS NOT NULL OR product IS NOT NULL)
> + AND sourcepackagename IS NULL
> + AND packagetype IS NULL
> + AND channel IS NULL
> + WHEN packagetype IS NOT NULL THEN
> + sourcepackagename IS NOT NULL
> + AND (distribution IS NOT NULL OR distroseries IS NOT NULL)
> + AND product IS NULL
> + AND productseries IS NULL
> + AND ociproject IS NULL
> + AND ociprojectseries IS NULL
> + ELSE false
Should we also have a "if there is a channel, there must be a packagetype? I think we wouldn't need to check other fields because they are checked already (if package type exists, there must be source package name etc)
> + END) NOT VALID;
> +
> +ALTER INDEX bugsummary__unique RENAME TO old__bugsummary__unique;
> +
> +ALTER TABLE BugSummaryJournal
> + ADD COLUMN packagetype integer,
> + ADD COLUMN channel jsonb;
> +
> +ALTER INDEX bugsummaryjournal__full__idx
> + RENAME TO old__bugsummaryjournal__full__idx;
> +
> +
> +-- Functions
> +
> +CREATE OR REPLACE FUNCTION bugtask_flatten(task_id integer, check_only boolean)
> + RETURNS boolean
> + SET search_path = public
> + LANGUAGE plpgsql
> + SECURITY DEFINER
> + 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 * INTO bug_row FROM bug 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,
> + task_row.packagetype, task_row.channel
> + 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,
> + packagetype = new_flat_row.packagetype,
> + channel = new_flat_row.channel
> + 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 NOT DISTINCT FROM $1.product
> + AND productseries IS NOT DISTINCT FROM $1.productseries
> + AND distribution IS NOT DISTINCT FROM $1.distribution
> + AND distroseries IS NOT DISTINCT FROM $1.distroseries
> + AND sourcepackagename IS NOT DISTINCT FROM $1.sourcepackagename
> + AND ociproject IS NOT DISTINCT FROM $1.ociproject
> + AND ociprojectseries IS NOT DISTINCT FROM $1.ociprojectseries
> + AND packagetype IS NOT DISTINCT FROM $1.packagetype
> + AND channel IS NOT DISTINCT FROM $1.channel
> + AND viewed_by IS NOT DISTINCT FROM $1.viewed_by
> + AND tag IS NOT DISTINCT FROM $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, packagetype,
> + channel, 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.packagetype,
> + d.channel, 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;
> +$$;
> +
> +
> +-- bug_summary_dec does not currently have ociproject and ociprojectseries
Good question... It seems as though if it wasn't used up until now, it doesn't seem needed... I'll help dig a little
> +-- is this an actual error?
> +CREATE OR REPLACE FUNCTION bug_summary_dec(bugsummary)
> + RETURNS VOID
> + LANGUAGE sql
> + AS $$
> + -- We own the row reference, so in the absence of bugs this cannot
> + -- fail - just decrement the row.
> + UPDATE BugSummary SET count = count + $1.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 ((packagetype IS NULL AND $1.packagetype IS NULL)
> + OR packagetype = $1.packagetype)
> + AND ((channel IS NULL AND $1.channel IS NULL)
> + OR channel = $1.channel)
> + 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;
> +$$;
> +
> +CREATE OR REPLACE FUNCTION bugsummary_rollup_journal(batchsize integer DEFAULT NULL::integer)
> + RETURNS VOID
> + SET search_path TO 'public'
Tiny nit, but in a function above you use `SET search_path = public`, chose one and update the other?
> + LANGUAGE plpgsql
> + SECURITY DEFINER
> + AS $$
> +DECLARE
> + d bugsummary%ROWTYPE;
> + max_id integer;
> +BEGIN
> + -- Lock so we don't content with other invocations 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,
> + packagetype,
> + channel
> + FROM BugSummaryJournal
> + WHERE id <= max_id
> + GROUP BY
> + product, productseries, distribution, distroseries,
> + sourcepackagename, ociproject, ociprojectseries, packagetype,
> + channel, 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
> + SET search_path TO 'public'
> + LANGUAGE plpgsql
> + SECURITY DEFINER
> + 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.packagetype IS DISTINCT FROM OLD.packagetype
> + OR NEW.channel IS DISTINCT FROM OLD.channel
> + 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,
> + packagetype = NEW.packagetype,
> + channel = NEW.channel,
> + 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;
> +$$;
> +
> +
> +-- Review returns TABLE () ordering and value types
> +-- If we want to target only packages in distroseries we need to modify this
> +DROP FUNCTION bugsummary_targets;
> +CREATE FUNCTION bugsummary_targets(btf_row bugtaskflat)
Why drop and create instead of `CREATE OR REPLACE FUNCTION`? I do see examples of files similar to this that also do this, do you know if there is a reason?
> + RETURNS TABLE(
> + product integer,
> + productseries integer,
> + distribution integer,
> + distroseries integer,
> + sourcepackagename integer,
> + ociproject integer,
> + ociprojectseries integer,
> + packagetype integer,
> + channel jsonb
> + )
> + IMMUTABLE
> + LANGUAGE sql
> + AS $$
> + -- Include a sourcepackagename-free/ociproject(series)-free task if this
> + -- one has a sourcepackagename/ociproject(series), so package tasks are
> + -- also counted in their distro/series.
> + SELECT
> + $1.product, $1.productseries, $1.distribution,
> + $1.distroseries, $1.sourcepackagename, $1.ociproject,
> + $1.ociprojectseries, $1.packagetype, $1.channel
> + UNION -- Implicit DISTINCT
> + SELECT
> + $1.product, $1.productseries, $1.distribution,
> + $1.distroseries, NULL, NULL,
> + NULL, NULL, NULL;
> +$$;
> +
> +
> +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,
> + bug_targets.packagetype, bug_targets.channel
> + 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, packagetype,
> + channel, viewed_by, tag, status, milestone, importance, has_patch,
> + access_policy)
> + SELECT
> + SUM(count), product, productseries, distribution, distroseries,
> + sourcepackagename, ociproject, ociprojectseries, packagetype,
> + channel, viewed_by, tag, status, milestone, importance, has_patch,
> + access_policy
> + FROM unnest(journals)
> + GROUP BY
> + product, productseries, distribution, distroseries,
> + sourcepackagename, ociproject, ociprojectseries, packagetype,
> + channel, 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 (
I see that we use `CREATE OR REPLACE VIEW CombinedBugSummary AS (SELECT...` in previous `CombinedBugSummary` views, instead of selecting column names. Is there a reason to have it this way?
> + id,
> + count,
> + product,
> + productseries,
> + distribution,
> + distroseries,
> + sourcepackagename,
> + viewed_by,
> + tag,
> + status,
> + milestone,
> + importance,
> + has_patch,
> + access_policy,
> + ociproject,
> + ociprojectseries,
> + packagetype,
> + channel
> +)
> +AS
> +SELECT id,
> + count,
> + product,
> + productseries,
> + distribution,
> + distroseries,
> + sourcepackagename,
> + viewed_by,
> + tag,
> + status,
> + milestone,
> + importance,
> + has_patch,
> + access_policy,
> + ociproject,
> + ociprojectseries,
> + packagetype,
> + channel
> +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,
> + packagetype,
> + channel
> +FROM bugsummaryjournal;
> +
> +INSERT INTO LaunchpadDatabaseRevision VALUES (2211, 43, 0);
--
https://code.launchpad.net/~enriqueesanchz/launchpad/+git/launchpad/+merge/488210
Your team Launchpad code reviewers is requested to review the proposed merge of ~enriqueesanchz/launchpad:db-bugtask-packagetype-channel-repositories into launchpad:db-devel.
References
