launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Review: Approve



Diff comments:

> === modified file 'lib/lp/registry/model/person.py'
> --- lib/lp/registry/model/person.py	2017-04-22 13:09:01 +0000
> +++ lib/lp/registry/model/person.py	2017-06-06 08:18:21 +0000
> @@ -1434,6 +1434,32 @@
>          from lp.registry.model.distribution import Distribution
>          store = Store.of(self)
>          WorkItem = SpecificationWorkItem
> +
> +        # Since a workitem's assignee defaults to its specification's
> +        # assignee, the PostgreSQL planner isn't always able to work out
> +        # the selectivity of the filter. Put that in a CTE to force it
> +        # to calculate the workitems up front, rather than doing a hash
> +        # join over all of Specification and SpecificationWorkItem.
> +        assigned_specificationworkitem = With(
> +            'assigned_specificationworkitem',
> +            Union(
> +                Select(
> +                    SpecificationWorkItem.id,

This should use the abbreviated WorkItem alias, rather than using WorkItem / SpecificationWorkItem interchangeably for the same table in the same method.  (Alternatively, drop the alias entirely, although it's probably reasonable to keep it since the full name is rather wordy.)

> +                    where=And(
> +                        SpecificationWorkItem.assignee_id.is_in(
> +                            self.participant_ids),
> +                        Not(SpecificationWorkItem.deleted))),
> +                Select(
> +                    SpecificationWorkItem.id,
> +                    where=And(
> +                        SpecificationWorkItem.specification_id.is_in(
> +                            Select(
> +                                Specification.id,
> +                                where=Specification._assigneeID.is_in(
> +                                    self.participant_ids))),
> +                        Not(SpecificationWorkItem.deleted))),
> +                all=True))

I'm not sure what the point of using UNION ALL is here, since this CTE is just treated as a set anyway.  Is it necessary for performance?

> +
>          origin = [Specification]
>          productjoin, query = get_specification_active_product_filter(self)
>          origin.extend(productjoin)


-- 
https://code.launchpad.net/~wgrant/launchpad/upcoming-optimisation/+merge/325141
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.