Merge into db-devel : stored-proc-for-bug-heat-bug-582195 : Code : Launchpad itself

Reviewer	Review Type	Date Requested	Status
Curtis Hovey (community)	rc	2010-05-27	Approve on 2010-05-27
Björn Tillenius (community)	db	2010-05-26	Approve on 2010-05-27
Stuart Bishop (community)	db	2010-05-26	Approve on 2010-05-27
Canonical Launchpad Engineering	code	2010-05-26	Pending
Review via email: mp+26064@code.staging.launchpad.net

Revision history for this message

Björn Tillenius (bjornt) wrote on 2010-05-26: Posted in a previous version of this proposal

#

On Wed, May 26, 2010 at 10:50:49AM -0000, Graham Binns wrote:
> The stored procedure is currently not hooked up to anything, but once
> the patch is applied it's easy to check the results of the stored
> procedure against the existing code using psql and an iharness session,
> thus:

Why not hook it up directly? It would be useful to have this tested,
since it's hard to check that it's doing what it should be doing.

--
Björn Tillenius | https://launchpad.net/~bjornt

Revision history for this message

Graham Binns (gmb) wrote on 2010-05-26: Posted in a previous version of this proposal

#

2010/5/26 Björn Tillenius <email address hidden>:
> On Wed, May 26, 2010 at 10:50:49AM -0000, Graham Binns wrote:
>> The stored procedure is currently not hooked up to anything, but once
>> the patch is applied it's easy to check the results of the stored
>> procedure against the existing code using psql and an iharness session,
>> thus:
>
> Why not hook it up directly? It would be useful to have this tested,
> since it's hard to check that it's doing what it should be doing.
>

I was going to do it in a subsequent branch, but since it's now late
in stub's day anyway I'll do it in this one and resubmit.

--
Graham Binns | PGP Key: EC66FA7D

Revision history for this message

Graham Binns (gmb) wrote on 2010-05-26:

#

Resubmitted. I've created an updateHeat() method on IBug which calls the stored procedure to update the bug's heat. I've hooked that up to all the sites which currently updated heat immediately (i.e. Bug.setPrivate() and setSecurity()). I'll start another branch to completely replace all the uses of CalculateBugHeatJob since that's likely to be several hundred lines long and would almost definitely exceed the limit were I to do it here.

Revision history for this message

Stuart Bishop (stub) wrote on 2010-05-27:

#

Download full text (7.2 KiB)

On Wed, May 26, 2010 at 10:26 PM, Graham Binns <email address hidden> wrote:

> === added file 'database/schema/patch-2207-99-0.sql'

There is no need for a database patch - the stored procedure should be
added to trusted.sql (which is becoming unwieldy - I think I need to
split it up).

> +CREATE OR REPLACE FUNCTION calculate_bug_heat(bug_id integer) RETURNS integer
> +LANGUAGE plpythonu AS $$
> + from datetime import datetime
> +
> + class BugHeatConstants:
> + PRIVACY = 150
> + SECURITY = 250
> + DUPLICATE = 6
> + AFFECTED_USER = 4
> + SUBSCRIBER = 2
> +
> +
> + def get_max_heat_for_bug(bug_id):
> + bug_tasks = plpy.execute(
> + "SELECT * FROM BugTask WHERE bug = %s" % bug_id)
> +
> + max_heats = []
> + for bug_task in bug_tasks:
> + if bug_task['product'] is not None:
> + product = plpy.execute(
> + "SELECT max_bug_heat FROM Product WHERE id = %s" %
> + bug_task['product'])[0]
> + max_heats.append(product['max_bug_heat'])
> + elif bug_task['distribution']:
> + distribution = plpy.execute(
> + "SELECT max_bug_heat FROM Distribution WHERE id = %s" %
> + bug_task['distribution'])[0]
> + max_heats.append(distribution['max_bug_heat'])
> + elif bug_task['productseries'] is not None:
> + product_series = plpy.execute("""
> + SELECT Product.max_bug_heat
> + FROM ProductSeries, Product
> + WHERE ProductSeries.Product = Product.id
> + AND ProductSeries.id = %s"""%
> + bug_task['productseries'])[0]
> + max_heats.append(product_series['max_bug_heat'])
> + elif bug_task['distroseries']:
> + distro_series = plpy.execute("""
> + SELECT Distribution.max_bug_heat
> + FROM DistroSeries, Distribution
> + WHERE DistroSeries.Distribution = Distribution.id
> + AND DistroSeries.id = %s"""%
> + bug_task['distroseries'])[0]
> + max_heats.append(distro_series['max_bug_heat'])
> + else:
> + pass
> +
> + return max(max_heats)

You can do this in a single query rather than one per task:

SELECT MAX(GREATEST(Product.max_bug_heat, Distribution.max_bug_heat))
FROM BugTask
LEFT OUTER JOIN ProductSeries ON BugTask.productseries = ProductSeries.id
LEFT OUTER JOIN Product ON (
    BugTask.product = Product.id
    OR ProductSeries.product = Product.id)
LEFT OUTER JOIN DistroSeries ON BugTask.distroseries = DistroSeries.id
LEFT OUTER JOIN Distribution ON (
    BugTask.distribution = Distribution.id
    OR DistroSeries.distribution = Distribution.id)
WHERE
    BugTask.bug = 1

> + # It would be nice to be able to just SELECT * here, but we need to
> + # format the timestamps so that datetime.strptime() won't choke on
> + # them.

Using epoch time would avoid the need for parsing strings (use
'EXTRACT(epoch FROM datecreated)' i...

On Wed, May 26, 2010 at 10:26 PM, Graham Binns <graham@canonical.com> wrote:

> === added file 'database/schema/patch-2207-99-0.sql'

There is no need for a database patch - the stored procedure should be
added to trusted.sql (which is becoming unwieldy - I think I need to
split it up).

> +CREATE OR REPLACE FUNCTION calculate_bug_heat(bug_id integer) RETURNS integer
> +LANGUAGE plpythonu AS $$
> +    from datetime import datetime
> +
> +    class BugHeatConstants:
> +        PRIVACY = 150
> +        SECURITY = 250
> +        DUPLICATE = 6
> +        AFFECTED_USER = 4
> +        SUBSCRIBER = 2
> +
> +
> +    def get_max_heat_for_bug(bug_id):
> +        bug_tasks = plpy.execute(
> +            "SELECT * FROM BugTask WHERE bug = %s" % bug_id)
> +
> +        max_heats = []
> +        for bug_task in bug_tasks:
> +            if bug_task['product'] is not None:
> +                product = plpy.execute(
> +                    "SELECT max_bug_heat FROM Product WHERE id = %s" %
> +                    bug_task['product'])[0]
> +                max_heats.append(product['max_bug_heat'])
> +            elif bug_task['distribution']:
> +                distribution = plpy.execute(
> +                    "SELECT max_bug_heat FROM Distribution WHERE id = %s" %
> +                    bug_task['distribution'])[0]
> +                max_heats.append(distribution['max_bug_heat'])
> +            elif bug_task['productseries'] is not None:
> +                product_series = plpy.execute("""
> +                    SELECT Product.max_bug_heat
> +                      FROM ProductSeries, Product
> +                     WHERE ProductSeries.Product = Product.id
> +                       AND ProductSeries.id = %s"""%
> +                    bug_task['productseries'])[0]
> +                max_heats.append(product_series['max_bug_heat'])
> +            elif bug_task['distroseries']:
> +                distro_series = plpy.execute("""
> +                    SELECT Distribution.max_bug_heat
> +                      FROM DistroSeries, Distribution
> +                     WHERE DistroSeries.Distribution = Distribution.id
> +                       AND DistroSeries.id = %s"""%
> +                    bug_task['distroseries'])[0]
> +                max_heats.append(distro_series['max_bug_heat'])
> +            else:
> +                pass
> +
> +        return max(max_heats)

You can do this in a single query rather than one per task:

SELECT MAX(GREATEST(Product.max_bug_heat, Distribution.max_bug_heat))
FROM BugTask
LEFT OUTER JOIN ProductSeries ON BugTask.productseries = ProductSeries.id
LEFT OUTER JOIN Product ON (
    BugTask.product = Product.id
    OR ProductSeries.product = Product.id)
LEFT OUTER JOIN DistroSeries ON BugTask.distroseries = DistroSeries.id
LEFT OUTER JOIN Distribution ON (
    BugTask.distribution = Distribution.id
    OR DistroSeries.distribution = Distribution.id)
WHERE
    BugTask.bug = 1

> +    # It would be nice to be able to just SELECT * here, but we need to
> +    # format the timestamps so that datetime.strptime() won't choke on
> +    # them.

Using epoch time would avoid the need for parsing strings (use
'EXTRACT(epoch FROM datecreated)' instead of 'TO_CHAR(...)').
It probably doesn't save any significant time though.

> +    if bug_data.nrows() == 0:
> +        return 0

I'd return None or raise an exception here - the bug doesn't exist so
the heat is undefined.

> +    bug = bug_data[0]
> +    if bug['duplicateof'] is not None:
> +        return 0

Again, None or the heat of the master bug. We should catch cases where
we are attempting to calculate the bug heat of non-existent or
duplicate bugs.

> +    if bug['private']:
> +        heat['privacy'] = BugHeatConstants.PRIVACY
> +    if bug['security_related']:
> +        heat['security'] = BugHeatConstants.SECURITY
> +
> +    # Get the heat from subscribers.
> +    sub_count = plpy.execute("""
> +        SELECT bug, COUNT(id) AS sub_count
> +            FROM BugSubscription
> +            WHERE bug = %s
> +            GROUP BY bug
> +            """ % bug_id)

This is unnecessarily complex. The simpler query is just "SELECT
COUNT(*) FROM BugSubscription WHERE bug=%s".

> +    if sub_count.nrows() > 0:
> +        heat['subscribers'] = (
> +            BugHeatConstants.SUBSCRIBER * sub_count[0]['sub_count'])

You don't need the nrows() guard, as the query will always return one row.

> +    # Get the heat from subscribers via duplicates.
> +    subs_from_dupes = plpy.execute("""
> +        SELECT bug.duplicateof AS dupe_of,
> +                COUNT(bugsubscription.id) AS dupe_sub_count
> +        FROM bugsubscription, bug
> +        WHERE   bug.duplicateof = %s
> +            AND Bugsubscription.bug = bug.id
> +        GROUP BY Bug.duplicateof""" % bug_id)
> +    if subs_from_dupes.nrows() > 0:
> +        heat['subcribers_from_dupes'] = (
> +            BugHeatConstants.SUBSCRIBER * sub_count[0]['dupe_sub_count'])

Similar here. nrows() guard is unnecessary. The query is also overly complex:

SELECT COUNT(*) FROM BugSubscription, Bug WHERE Bug.duplicateof = %s
AND BugSubscription.bug = Bug.id

I notice that if a person is subscribed to both a duplicate and the
master job, they are counted twice. If this is not correct, we can
collapse the above two sections into one using the query:

SELECT COUNT(DISTINCT BugSubscription.person)
FROM BugSubscription, Bug
WHERE Bug.id = BugSubscription.bug
    AND (Bug.id = %s OR Bug.duplicateof = %s)

If you want to expand teams to get the genuine number of people subscribed:

SELECT COUNT(DISTINCT TeamParticipation.person)
FROM TeamParticipation, BugSubscription, Bug
WHERE
    TeamParticipation.team = BugSubscription.person
    AND Bug.id = BugSubscription.bug
    AND (Bug.id = %s OR Bug.duplicateof = %s)

Bug #1 for instance has 313 subscribers without expanding the teams,
or 678 if you do expand the teams.

> +    # Bugs decay over time. Every day the bug isn't touched its heat
> +    # decreases by 1%.
> +    pg_datetime_fmt = "%Y-%m-%d %H:%M:%S"
> +    date_last_updated = datetime.strptime(
> +        bug['formatted_date_last_updated'], pg_datetime_fmt)
> +    days_since_last_update = (datetime.utcnow() - date_last_updated).days
> +    total_heat = int(total_heat * (0.99 ** days_since_last_update))

Yer... this would be simpler and clearer using epoch times.

> +INSERT INTO LaunchpadDatabaseRevision VALUES (2207, 99, 0);

As before, this should go into trusted.sql rather than a DB patch. We
get to treat stored procedures as any other code, unlike database
patches which cannot be reverted once applied.

> -Sometimes, when a bug changes, we want to see the changes reflected in the bug's
> -heat value immidiately, without waiting for heat to be recalculated. Currently
> -we adjust heat immidiately for bug privacy and security.

immediately, not immidiately.

> +    def updateHeat(self):
> +        """See `IBug`."""
> +        # We need to flush the store first to ensure that changes are
> +        # reflected in the new bug heat total.
> +        store = Store.of(self)
> +        store.flush()

You sure about that? Storm should be applying updates in order, so
when calculate_bug_heat is actually executed (at commit time or when
heat is next accessed) changes will be implicitly flushed.

-- 
Stuart Bishop <stuart@stuartbishop.net>
http://www.stuartbishop.net/

Revision history for this message

Stuart Bishop (stub) wrote on 2010-05-27:

#

Download full text (7.2 KiB)

On Wed, May 26, 2010 at 10:26 PM, Graham Binns <email address hidden> wrote:

> === added file 'database/schema/patch-2207-99-0.sql'

There is no need for a database patch - the stored procedure should be added to trusted.sql (which is becoming unwieldy - I think I need to split it up).

> +CREATE OR REPLACE FUNCTION calculate_bug_heat(bug_id integer) RETURNS integer
> +LANGUAGE plpythonu AS $$
> + from datetime import datetime
> +
> + class BugHeatConstants:
> + PRIVACY = 150
> + SECURITY = 250
> + DUPLICATE = 6
> + AFFECTED_USER = 4
> + SUBSCRIBER = 2
> +
> +
> + def get_max_heat_for_bug(bug_id):
> + bug_tasks = plpy.execute(
> + "SELECT * FROM BugTask WHERE bug = %s" % bug_id)
> +
> + max_heats = []
> + for bug_task in bug_tasks:
> + if bug_task['product'] is not None:
> + product = plpy.execute(
> + "SELECT max_bug_heat FROM Product WHERE id = %s" %
> + bug_task['product'])[0]
> + max_heats.append(product['max_bug_heat'])
> + elif bug_task['distribution']:
> + distribution = plpy.execute(
> + "SELECT max_bug_heat FROM Distribution WHERE id = %s" %
> + bug_task['distribution'])[0]
> + max_heats.append(distribution['max_bug_heat'])
> + elif bug_task['productseries'] is not None:
> + product_series = plpy.execute("""
> + SELECT Product.max_bug_heat
> + FROM ProductSeries, Product
> + WHERE ProductSeries.Product = Product.id
> + AND ProductSeries.id = %s"""%
> + bug_task['productseries'])[0]
> + max_heats.append(product_series['max_bug_heat'])
> + elif bug_task['distroseries']:
> + distro_series = plpy.execute("""
> + SELECT Distribution.max_bug_heat
> + FROM DistroSeries, Distribution
> + WHERE DistroSeries.Distribution = Distribution.id
> + AND DistroSeries.id = %s"""%
> + bug_task['distroseries'])[0]
> + max_heats.append(distro_series['max_bug_heat'])
> + else:
> + pass
> +
> + return max(max_heats)

You can do this in a single query rather than one per task:

SELECT MAX(GREATEST(Product.max_bug_heat, Distribution.max_bug_heat))
FROM BugTask
LEFT OUTER JOIN ProductSeries ON BugTask.productseries = ProductSeries.id
LEFT OUTER JOIN Product ON (
    BugTask.product = Product.id
    OR ProductSeries.product = Product.id)
LEFT OUTER JOIN DistroSeries ON BugTask.distroseries = DistroSeries.id
LEFT OUTER JOIN Distribution ON (
    BugTask.distribution = Distribution.id
    OR DistroSeries.distribution = Distribution.id)
WHERE
    BugTask.bug = 1

> + # It would be nice to be able to just SELECT * here, but we need to
> + # format the timestamps so that datetime.strptime() won't choke on
> + # them.

Using epoch time would avoid the need for parsing strings (use 'EXTRACT(epoch FROM datecreated)' i...

On Wed, May 26, 2010 at 10:26 PM, Graham Binns <graham@canonical.com> wrote:

> === added file 'database/schema/patch-2207-99-0.sql'

There is no need for a database patch - the stored procedure should be added to trusted.sql (which is becoming unwieldy - I think I need to split it up).

> +CREATE OR REPLACE FUNCTION calculate_bug_heat(bug_id integer) RETURNS integer
> +LANGUAGE plpythonu AS $$
> +    from datetime import datetime
> +
> +    class BugHeatConstants:
> +        PRIVACY = 150
> +        SECURITY = 250
> +        DUPLICATE = 6
> +        AFFECTED_USER = 4
> +        SUBSCRIBER = 2
> +
> +
> +    def get_max_heat_for_bug(bug_id):
> +        bug_tasks = plpy.execute(
> +            "SELECT * FROM BugTask WHERE bug = %s" % bug_id)
> +
> +        max_heats = []
> +        for bug_task in bug_tasks:
> +            if bug_task['product'] is not None:
> +                product = plpy.execute(
> +                    "SELECT max_bug_heat FROM Product WHERE id = %s" %
> +                    bug_task['product'])[0]
> +                max_heats.append(product['max_bug_heat'])
> +            elif bug_task['distribution']:
> +                distribution = plpy.execute(
> +                    "SELECT max_bug_heat FROM Distribution WHERE id = %s" %
> +                    bug_task['distribution'])[0]
> +                max_heats.append(distribution['max_bug_heat'])
> +            elif bug_task['productseries'] is not None:
> +                product_series = plpy.execute("""
> +                    SELECT Product.max_bug_heat
> +                      FROM ProductSeries, Product
> +                     WHERE ProductSeries.Product = Product.id
> +                       AND ProductSeries.id = %s"""%
> +                    bug_task['productseries'])[0]
> +                max_heats.append(product_series['max_bug_heat'])
> +            elif bug_task['distroseries']:
> +                distro_series = plpy.execute("""
> +                    SELECT Distribution.max_bug_heat
> +                      FROM DistroSeries, Distribution
> +                     WHERE DistroSeries.Distribution = Distribution.id
> +                       AND DistroSeries.id = %s"""%
> +                    bug_task['distroseries'])[0]
> +                max_heats.append(distro_series['max_bug_heat'])
> +            else:
> +                pass
> +
> +        return max(max_heats)

You can do this in a single query rather than one per task:

SELECT MAX(GREATEST(Product.max_bug_heat, Distribution.max_bug_heat))
FROM BugTask
LEFT OUTER JOIN ProductSeries ON BugTask.productseries = ProductSeries.id
LEFT OUTER JOIN Product ON (
    BugTask.product = Product.id
    OR ProductSeries.product = Product.id)
LEFT OUTER JOIN DistroSeries ON BugTask.distroseries = DistroSeries.id
LEFT OUTER JOIN Distribution ON (
    BugTask.distribution = Distribution.id
    OR DistroSeries.distribution = Distribution.id)
WHERE
    BugTask.bug = 1

> +    # It would be nice to be able to just SELECT * here, but we need to
> +    # format the timestamps so that datetime.strptime() won't choke on
> +    # them.

Using epoch time would avoid the need for parsing strings (use 'EXTRACT(epoch FROM datecreated)' instead of 'TO_CHAR(...)').
It probably doesn't save any significant time though.

> +    if bug_data.nrows() == 0:
> +        return 0

I'd return None or raise an exception here - the bug doesn't exist so the heat is undefined.

> +    bug = bug_data[0]
> +    if bug['duplicateof'] is not None:
> +        return 0

Again, None or the heat of the master bug. We should catch cases where we are attempting to calculate the bug heat of non-existent or duplicate bugs.

> +    if bug['private']:
> +        heat['privacy'] = BugHeatConstants.PRIVACY
> +    if bug['security_related']:
> +        heat['security'] = BugHeatConstants.SECURITY
> +
> +    # Get the heat from subscribers.
> +    sub_count = plpy.execute("""
> +        SELECT bug, COUNT(id) AS sub_count
> +            FROM BugSubscription
> +            WHERE bug = %s
> +            GROUP BY bug
> +            """ % bug_id)

This is unnecessarily complex. The simpler query is just "SELECT COUNT(*) FROM BugSubscription WHERE bug=%s".

> +    if sub_count.nrows() > 0:
> +        heat['subscribers'] = (
> +            BugHeatConstants.SUBSCRIBER * sub_count[0]['sub_count'])

You don't need the nrows() guard, as the query will always return one row.

> +    # Get the heat from subscribers via duplicates.
> +    subs_from_dupes = plpy.execute("""
> +        SELECT bug.duplicateof AS dupe_of,
> +                COUNT(bugsubscription.id) AS dupe_sub_count
> +        FROM bugsubscription, bug
> +        WHERE   bug.duplicateof = %s
> +            AND Bugsubscription.bug = bug.id
> +        GROUP BY Bug.duplicateof""" % bug_id)
> +    if subs_from_dupes.nrows() > 0:
> +        heat['subcribers_from_dupes'] = (
> +            BugHeatConstants.SUBSCRIBER * sub_count[0]['dupe_sub_count'])

Similar here. nrows() guard is unnecessary. The query is also overly complex:

SELECT COUNT(*) FROM BugSubscription, Bug WHERE Bug.duplicateof = %s AND BugSubscription.bug = Bug.id

I notice that if a person is subscribed to both a duplicate and the master job, they are counted twice. If this is not correct, we can collapse the above two sections into one using the query:

SELECT COUNT(DISTINCT BugSubscription.person)
FROM BugSubscription, Bug
WHERE Bug.id = BugSubscription.bug
    AND (Bug.id = %s OR Bug.duplicateof = %s)

If you want to expand teams to get the genuine number of people subscribed:

SELECT COUNT(DISTINCT TeamParticipation.person)
FROM TeamParticipation, BugSubscription, Bug
WHERE
    TeamParticipation.team = BugSubscription.person
    AND Bug.id = BugSubscription.bug
    AND (Bug.id = %s OR Bug.duplicateof = %s)

Bug #1 for instance has 313 subscribers without expanding the teams, or 678 if you do expand the teams.

> +    # Bugs decay over time. Every day the bug isn't touched its heat
> +    # decreases by 1%.
> +    pg_datetime_fmt = "%Y-%m-%d %H:%M:%S"
> +    date_last_updated = datetime.strptime(
> +        bug['formatted_date_last_updated'], pg_datetime_fmt)
> +    days_since_last_update = (datetime.utcnow() - date_last_updated).days
> +    total_heat = int(total_heat * (0.99 ** days_since_last_update))

Yer... this would be simpler and clearer using epoch times.

> +INSERT INTO LaunchpadDatabaseRevision VALUES (2207, 99, 0);

As before, this should go into trusted.sql rather than a DB patch. We get to treat stored procedures as any other code, unlike database patches which cannot be reverted once applied.

> -Sometimes, when a bug changes, we want to see the changes reflected in the bug's
> -heat value immidiately, without waiting for heat to be recalculated. Currently
> -we adjust heat immidiately for bug privacy and security.

immediately, not immidiately.

> +    def updateHeat(self):
> +        """See `IBug`."""
> +        # We need to flush the store first to ensure that changes are
> +        # reflected in the new bug heat total.
> +        store = Store.of(self)
> +        store.flush()

You sure about that? Storm should be applying updates in order, so when calculate_bug_heat is actually executed (at commit time or when heat is next accessed) changes will be implicitly flushed.

review: Needs Fixing (db)

Revision history for this message

Graham Binns (gmb) wrote on 2010-05-27:

#

Download full text (6.9 KiB)

On Thu, May 27, 2010 at 05:13:27AM -0000, Stuart Bishop wrote:
> Review: Needs Fixing db
> On Wed, May 26, 2010 at 10:26 PM, Graham Binns <email address hidden> wrote:
>
> > === added file 'database/schema/patch-2207-99-0.sql'
>
> There is no need for a database patch - the stored procedure should be added to trusted.sql (which is becoming unwieldy - I think I need to split it up).

Righto, I've moved it.

>[snip]
>
> You can do this in a single query rather than one per task:
>
> SELECT MAX(GREATEST(Product.max_bug_heat, Distribution.max_bug_heat))
> FROM BugTask
> LEFT OUTER JOIN ProductSeries ON BugTask.productseries = ProductSeries.id
> LEFT OUTER JOIN Product ON (
> BugTask.product = Product.id
> OR ProductSeries.product = Product.id)
> LEFT OUTER JOIN DistroSeries ON BugTask.distroseries = DistroSeries.id
> LEFT OUTER JOIN Distribution ON (
> BugTask.distribution = Distribution.id
> OR DistroSeries.distribution = Distribution.id)
> WHERE
> BugTask.bug = 1
>

Sweet; fixed.

> > + # It would be nice to be able to just SELECT * here, but we need to
> > + # format the timestamps so that datetime.strptime() won't choke on
> > + # them.
>
> Using epoch time would avoid the need for parsing strings (use
> 'EXTRACT(epoch FROM datecreated)' instead of 'TO_CHAR(...)'). It
> probably doesn't save any significant time though.

I've used epoch time instead (see below).

>
> > + if bug_data.nrows() == 0:
> > + return 0
>
> I'd return None or raise an exception here - the bug doesn't exist so
> the heat is undefined.

I've gone for raising an exception; is it worth defining an exception
subclass to use here or is Exception() okay?

> > + bug = bug_data[0]
> > + if bug['duplicateof'] is not None:
> > + return 0
>
> Again, None or the heat of the master bug. We should catch cases where
> we are attempting to calculate the bug heat of non-existent or
> duplicate bugs.
>

Okay. I'll update Bug.updateHeat() so that it doesn't try calling this
function if the bug is a duplicate and have this return None.

> > + if bug['private']:
> > + heat['privacy'] = BugHeatConstants.PRIVACY
> > + if bug['security_related']:
> > + heat['security'] = BugHeatConstants.SECURITY
> > +
> > + # Get the heat from subscribers.
> > + sub_count = plpy.execute("""
> > + SELECT bug, COUNT(id) AS sub_count
> > + FROM BugSubscription
> > + WHERE bug = %s
> > + GROUP BY bug
> > + """ % bug_id)
>
> This is unnecessarily complex. The simpler query is just "SELECT COUNT(*) FROM BugSubscription WHERE bug=%s".
>

Right; this is because I ripped it off from the query we were using to
update the bugs' heat manually :). Fixed now.

> > + if sub_count.nrows() > 0:
> > + heat['subscribers'] = (
> > + BugHeatConstants.SUBSCRIBER * sub_count[0]['sub_count'])
>
> You don't need the nrows() guard, as the query will always return one row.

Ok.

> > + # Get the heat from subscribers via duplicates.
> > + subs_from_dupes = plpy.execute("""
> > + SELECT bug.duplicateof AS dupe_of,
> > + COUNT(bugsubscription.id) AS...

On Thu, May 27, 2010 at 05:13:27AM -0000, Stuart Bishop wrote:
> Review: Needs Fixing db
> On Wed, May 26, 2010 at 10:26 PM, Graham Binns <graham@canonical.com> wrote:
> 
> > === added file 'database/schema/patch-2207-99-0.sql'
> 
> There is no need for a database patch - the stored procedure should be added to trusted.sql (which is becoming unwieldy - I think I need to split it up).

Righto, I've moved it.

>[snip]
> 
> You can do this in a single query rather than one per task:
> 
> SELECT MAX(GREATEST(Product.max_bug_heat, Distribution.max_bug_heat))
> FROM BugTask
> LEFT OUTER JOIN ProductSeries ON BugTask.productseries = ProductSeries.id
> LEFT OUTER JOIN Product ON (
>     BugTask.product = Product.id
>     OR ProductSeries.product = Product.id)
> LEFT OUTER JOIN DistroSeries ON BugTask.distroseries = DistroSeries.id
> LEFT OUTER JOIN Distribution ON (
>     BugTask.distribution = Distribution.id
>     OR DistroSeries.distribution = Distribution.id)
> WHERE
>     BugTask.bug = 1
>

Sweet; fixed.

> > +    # It would be nice to be able to just SELECT * here, but we need to
> > +    # format the timestamps so that datetime.strptime() won't choke on
> > +    # them.
> 
> Using epoch time would avoid the need for parsing strings (use
> 'EXTRACT(epoch FROM datecreated)' instead of 'TO_CHAR(...)').  It
> probably doesn't save any significant time though.

I've used epoch time instead (see below).

> 
> > +    if bug_data.nrows() == 0:
> > +        return 0
> 
> I'd return None or raise an exception here - the bug doesn't exist so
> the heat is undefined.

I've gone for raising an exception; is it worth defining an exception
subclass to use here or is Exception() okay?

> > +    bug = bug_data[0]
> > +    if bug['duplicateof'] is not None:
> > +        return 0
> 
> Again, None or the heat of the master bug. We should catch cases where
> we are attempting to calculate the bug heat of non-existent or
> duplicate bugs.
>

Okay. I'll update Bug.updateHeat() so that it doesn't try calling this
function if the bug is a duplicate and have this return None.

> > +    if bug['private']:
> > +        heat['privacy'] = BugHeatConstants.PRIVACY
> > +    if bug['security_related']:
> > +        heat['security'] = BugHeatConstants.SECURITY
> > +
> > +    # Get the heat from subscribers.
> > +    sub_count = plpy.execute("""
> > +        SELECT bug, COUNT(id) AS sub_count
> > +            FROM BugSubscription
> > +            WHERE bug = %s
> > +            GROUP BY bug
> > +            """ % bug_id)
> 
> This is unnecessarily complex. The simpler query is just "SELECT COUNT(*) FROM BugSubscription WHERE bug=%s".
>

Right; this is because I ripped it off from the query we were using to
update the bugs' heat manually :). Fixed now.

> > +    if sub_count.nrows() > 0:
> > +        heat['subscribers'] = (
> > +            BugHeatConstants.SUBSCRIBER * sub_count[0]['sub_count'])
> 
> You don't need the nrows() guard, as the query will always return one row.

Ok.

> > +    # Get the heat from subscribers via duplicates.
> > +    subs_from_dupes = plpy.execute("""
> > +        SELECT bug.duplicateof AS dupe_of,
> > +                COUNT(bugsubscription.id) AS dupe_sub_count
> > +        FROM bugsubscription, bug
> > +        WHERE   bug.duplicateof = %s
> > +            AND Bugsubscription.bug = bug.id
> > +        GROUP BY Bug.duplicateof""" % bug_id)
> > +    if subs_from_dupes.nrows() > 0:
> > +        heat['subcribers_from_dupes'] = (
> > +            BugHeatConstants.SUBSCRIBER * sub_count[0]['dupe_sub_count'])
> 
> Similar here. nrows() guard is unnecessary. The query is also overly complex:
> 
> SELECT COUNT(*) FROM BugSubscription, Bug WHERE Bug.duplicateof = %s AND BugSubscription.bug = Bug.id
> 
> I notice that if a person is subscribed to both a duplicate and the master job, they are counted twice. If this is not correct, we can collapse the above two sections into one using the query:
> 
> SELECT COUNT(DISTINCT BugSubscription.person)
> FROM BugSubscription, Bug
> WHERE Bug.id = BugSubscription.bug
>     AND (Bug.id = %s OR Bug.duplicateof = %s)

Thanks, I'll use this.

> If you want to expand teams to get the genuine number of people subscribed:
> 
> SELECT COUNT(DISTINCT TeamParticipation.person)
> FROM TeamParticipation, BugSubscription, Bug
> WHERE
>     TeamParticipation.team = BugSubscription.person
>     AND Bug.id = BugSubscription.bug
>     AND (Bug.id = %s OR Bug.duplicateof = %s)
> 
> 
> Bug #1 for instance has 313 subscribers without expanding the teams,
> or 678 if you do expand the teams.

Having talked with Deryck, we don't want to do this; it would inflate
the heat counts by far too much.

> > +    # Bugs decay over time. Every day the bug isn't touched its heat
> > +    # decreases by 1%.
> > +    pg_datetime_fmt = "%Y-%m-%d %H:%M:%S"
> > +    date_last_updated = datetime.strptime(
> > +        bug['formatted_date_last_updated'], pg_datetime_fmt)
> > +    days_since_last_update = (datetime.utcnow() - date_last_updated).days
> > +    total_heat = int(total_heat * (0.99 ** days_since_last_update))
> 
> Yer... this would be simpler and clearer using epoch times.

Right, I've converted it to use epoch time and fromtimestamp().

> > +INSERT INTO LaunchpadDatabaseRevision VALUES (2207, 99, 0);
> 
> As before, this should go into trusted.sql rather than a DB patch. We get to treat stored procedures as any other code, unlike database patches which cannot be reverted once applied.

Right.

> > -Sometimes, when a bug changes, we want to see the changes reflected in the bug's
> > -heat value immidiately, without waiting for heat to be recalculated. Currently
> > -we adjust heat immidiately for bug privacy and security.
> 
> immediately, not immidiately.

I've already fixed that (you're reading the removal part of the diff
;)).

> > +    def updateHeat(self):
> > +        """See `IBug`."""
> > +        # We need to flush the store first to ensure that changes are
> > +        # reflected in the new bug heat total.
> > +        store = Store.of(self)
> > +        store.flush()
> 
> You sure about that? Storm should be applying updates in order, so
> when calculate_bug_heat is actually executed (at commit time or when
> heat is next accessed) changes will be implicitly flushed.

Hmm. I put the flush() in there because in testing it seemed like heat
lagged behind where it should be. If I remove this, I get this result:

>>> bug = factory.makeBug()
    >>> bug.heat
    8

>>> changed = bug.setPrivate(True, bug.owner)
    >>> bug.heat
    8

>>> changed = bug.setSecurityRelated(True, bug.owner)
    >>> bug.heat
    158

Where what I should be seeing is

>>> bug = factory.makeBug()
    >>> bug.heat
    8

>>> changed = bug.setPrivate(True, bug.owner)
    >>> bug.heat
    158

>>> changed = bug.setSecurityRelated(True, bug.owner)
    >>> bug.heat
    408

So I think this is definitely necessary (or maybe it's just necessary
for testing; I don't know. What are your recommendations?)

Revision history for this message

Stuart Bishop (stub) wrote on 2010-05-27:

#

On Thu, May 27, 2010 at 6:42 PM, Graham Binns <email address hidden> wrote:

> I've gone for raising an exception; is it worth defining an exception
> subclass to use here or is Exception() okay?

Exception() is fine. Nothing outside this function would know about
your custom exception anyway.

>> You sure about that? Storm should be applying updates in order, so
>> when calculate_bug_heat is actually executed (at commit time or when
>> heat is next accessed) changes will be implicitly flushed.
>
> Hmm. I put the flush() in there because in testing it seemed like heat
> lagged behind where it should be. If I remove this, I get this result:

> So I think this is definitely necessary (or maybe it's just necessary
> for testing; I don't know. What are your recommendations?)

Your empirical results beat my fantasy, so leave it in.

Please change 'LANGUAGE plpythonu' to 'LANGUAGE plpythonu STABLE
RETURNS NULL ON NULL INPUT'

--
Stuart Bishop <email address hidden>
http://www.stuartbishop.net/

Revision history for this message

Stuart Bishop (stub) on 2010-05-27:

#

review: Approve (db)

Revision history for this message

Graham Binns (gmb) wrote on 2010-05-27:

#

On 27 May 2010 15:33, Stuart Bishop <email address hidden> wrote:
> On Thu, May 27, 2010 at 6:42 PM, Graham Binns <email address hidden> wrote:
>
>> I've gone for raising an exception; is it worth defining an exception
>> subclass to use here or is Exception() okay?
>
> Exception() is fine. Nothing outside this function would know about
> your custom exception anyway.

Cool.

>>> You sure about that? Storm should be applying updates in order, so
>>> when calculate_bug_heat is actually executed (at commit time or when
>>> heat is next accessed) changes will be implicitly flushed.
>>
>> Hmm. I put the flush() in there because in testing it seemed like heat
>> lagged behind where it should be. If I remove this, I get this result:
>
>> So I think this is definitely necessary (or maybe it's just necessary
>> for testing; I don't know. What are your recommendations?)
>
> Your empirical results beat my fantasy, so leave it in.

Fair enough.

> Please change 'LANGUAGE plpythonu' to 'LANGUAGE plpythonu STABLE
> RETURNS NULL ON NULL INPUT'

Done.

Revision history for this message

Björn Tillenius (bjornt) on 2010-05-27:

#

review: Approve (db)

Revision history for this message

Curtis Hovey (sinzui) wrote on 2010-05-27:

#

Thanks for asking for the RC before PQM closes. I do not want to delay QA of this.

review: Approve (rc)

Launchpad itself

Merge lp://staging/~gmb/launchpad/stored-proc-for-bug-heat-bug-582195 into lp://staging/launchpad/db-devel

Commit message

Description of the change

Preview Diff

Subscribers