Merge lp:~stevenk/launchpad/performant-getSourcesForDeletion into lp:launchpad

Proposed by Steve Kowalik on 2013-03-18
Status: Merged
Approved by: Steve Kowalik on 2013-03-18
Approved revision: no longer in the source branch.
Merged at revision: 16534
Proposed branch: lp:~stevenk/launchpad/performant-getSourcesForDeletion
Merge into: lp:launchpad
Diff against target: 249 lines (+49/-85)
3 files modified
database/schema/security.cfg (+0/-1)
lib/lp/soyuz/doc/archive.txt (+17/-18)
lib/lp/soyuz/model/archive.py (+32/-66)
To merge this branch: bzr merge lp:~stevenk/launchpad/performant-getSourcesForDeletion
Reviewer Review Type Date Requested Status
William Grant code 2013-03-18 Approve on 2013-03-18
Review via email: mp+153704@code.launchpad.net

Commit message

Rewrite IArchive.getSourcesForDeletion() to be performant, and Storm-ify it.

Description of the change

Rewrite IArchive.getSourcesForDeletion() to be performant, and Storm-ify it. It now makes use of SPPH.scheduleddeletiondate, which required some test fiddling.

Perform a little bit of clean-up, like destroying the use of SQLConstant, but this branch was already net-negative just due to switching to a simpler query. Drop a unneeded database perm that was missed by a testfix of mine.

To post a comment you must log in.
William Grant (wgrant) wrote :

I'd like to see a comment describing why we use scheduleddeletiondate, otherwise fine.

review: Approve (code)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'database/schema/security.cfg'
2--- database/schema/security.cfg 2013-03-12 05:51:28 +0000
3+++ database/schema/security.cfg 2013-03-18 04:11:22 +0000
4@@ -2197,7 +2197,6 @@
5 public.translationrelicensingagreement = SELECT, UPDATE
6 public.translator = SELECT, UPDATE
7 public.usertouseremail = SELECT, UPDATE
8-public.validpersoncache = SELECT
9 public.vote = SELECT, UPDATE
10 public.votecast = SELECT, UPDATE
11 public.wikiname = SELECT, UPDATE
12
13=== modified file 'lib/lp/soyuz/doc/archive.txt'
14--- lib/lp/soyuz/doc/archive.txt 2013-02-20 05:43:59 +0000
15+++ lib/lp/soyuz/doc/archive.txt 2013-03-18 04:11:22 +0000
16@@ -573,8 +573,10 @@
17
18 >>> cprov_archive.number_of_sources
19 3
20- >>> cprov_archive.getPublishedSources(
21- ... name=u'cdrkit').first().supersede()
22+ >>> cdrkit = cprov_archive.getPublishedSources(name=u'cdrkit').first()
23+ >>> cdrkit.supersede()
24+ >>> from lp.services.database.constants import UTC_NOW
25+ >>> cdrkit.scheduleddeletiondate = UTC_NOW
26
27 >>> cprov_archive.number_of_sources
28 2
29@@ -608,7 +610,7 @@
30 This method can optionally receive a source package name filter (SQL
31 LIKE) to restrict its result.
32
33- >>> cprov_archive.getSourcesForDeletion(name='ice').count()
34+ >>> cprov_archive.getSourcesForDeletion(name=u'ice').count()
35 1
36
37 If only the source publication is DELETED, leaving its binary behind,
38@@ -622,29 +624,28 @@
39 >>> login("celso.providelo@canonical.com")
40 >>> removal_candidate.requestDeletion(cprov, 'go away !')
41
42- >>> cprov_archive.getSourcesForDeletion(name='ice').count()
43+ >>> cprov_archive.getSourcesForDeletion(name=u'ice').count()
44 1
45
46 The status filter can be used to only return sources that can be
47 deleted matching a given status.
48
49 >>> cprov_archive.getSourcesForDeletion(
50- ... name='ice', status=PackagePublishingStatus.DELETED).count()
51+ ... name=u'ice', status=PackagePublishingStatus.DELETED).count()
52 1
53
54 >>> cprov_archive.getSourcesForDeletion(
55- ... name='ice', status=PackagePublishingStatus.PUBLISHED).count()
56+ ... name=u'ice', status=PackagePublishingStatus.PUBLISHED).count()
57 0
58
59 The status filter can also be a sequence of status.
60
61 >>> irrelevant_status = (
62 ... PackagePublishingStatus.SUPERSEDED,
63- ... PackagePublishingStatus.DELETED,
64- ... )
65+ ... PackagePublishingStatus.DELETED)
66
67 >>> cprov_archive.getSourcesForDeletion(
68- ... name='ice', status=irrelevant_status).count()
69+ ... name=u'ice', status=irrelevant_status).count()
70 1
71
72 The series filter can be used to return only sources from a certain
73@@ -655,20 +656,18 @@
74 >>> cprov_archive.getSourcesForDeletion(distroseries=hoary).count()
75 0
76
77-The source publication is only excluded from 'deletion list' when its
78-binary is also DELETED.
79-
80- >>> for bin in removal_candidate.getPublishedBinaries():
81- ... bin.requestDeletion(cprov, 'go away !')
82-
83- >>> cprov_archive.getSourcesForDeletion(name='ice').count()
84+The source publication is only excluded from 'deletion list' when it's
85+scheduled deletion date is set.
86+
87+ >>> from zope.security.proxy import removeSecurityProxy
88+ >>> removeSecurityProxy(removal_candidate).scheduleddeletiondate = UTC_NOW
89+ >>> cprov_archive.getSourcesForDeletion(name=u'ice').count()
90 0
91
92 Flush the database caches to invalidate old caches from the
93 corresponding publishing Postgres views.
94
95- >>> from lp.services.database.sqlbase import flush_database_caches
96- >>> flush_database_caches()
97+ >>> transaction.commit()
98
99
100 Build Lookup
101
102=== modified file 'lib/lp/soyuz/model/archive.py'
103--- lib/lp/soyuz/model/archive.py 2013-03-07 01:09:59 +0000
104+++ lib/lp/soyuz/model/archive.py 2013-03-18 04:11:22 +0000
105@@ -23,7 +23,6 @@
106 IntCol,
107 StringCol,
108 )
109-from sqlobject.sqlbuilder import SQLConstant
110 from storm.expr import (
111 And,
112 Desc,
113@@ -75,6 +74,7 @@
114 from lp.registry.model.sourcepackagename import SourcePackageName
115 from lp.registry.model.teammembership import TeamParticipation
116 from lp.services.config import config
117+from lp.services.database.bulk import load_related
118 from lp.services.database.constants import UTC_NOW
119 from lp.services.database.datetimecol import UtcDateTimeCol
120 from lp.services.database.decoratedresultset import DecoratedResultSet
121@@ -87,7 +87,6 @@
122 from lp.services.database.lpstorm import ISlaveStore
123 from lp.services.database.sqlbase import (
124 cursor,
125- quote,
126 quote_like,
127 SQLBase,
128 sqlvalues,
129@@ -608,70 +607,40 @@
130 list(store.find(GPGKey, GPGKey.id.is_in(ids)))
131 return DecoratedResultSet(resultset, pre_iter_hook=eager_load)
132
133- def getSourcesForDeletion(self, name=None, status=None,
134- distroseries=None):
135+ def getSourcesForDeletion(self, name=None, status=None, distroseries=None):
136 """See `IArchive`."""
137- clauses = ["""
138- SourcePackagePublishingHistory.archive = %s AND
139- SourcePackagePublishingHistory.sourcepackagerelease =
140- SourcePackageRelease.id AND
141- SourcePackagePublishingHistory.sourcepackagename =
142- SourcePackageName.id
143- """ % sqlvalues(self)]
144-
145- has_published_binaries_clause = """
146- EXISTS (SELECT TRUE FROM
147- BinaryPackagePublishingHistory bpph,
148- BinaryPackageRelease bpr, BinaryPackageBuild
149- WHERE
150- bpph.archive = %s AND
151- bpph.status = %s AND
152- bpph.binarypackagerelease = bpr.id AND
153- bpr.build = BinaryPackageBuild.id AND
154- BinaryPackageBuild.source_package_release =
155- SourcePackageRelease.id)
156- """ % sqlvalues(self, PackagePublishingStatus.PUBLISHED)
157-
158- source_deletable_states = (
159- PackagePublishingStatus.PENDING,
160- PackagePublishingStatus.PUBLISHED,
161- )
162- clauses.append("""
163- (%s OR SourcePackagePublishingHistory.status IN %s)
164- """ % (has_published_binaries_clause,
165- quote(source_deletable_states)))
166-
167- if status is not None:
168+ # We will return sources that can be deleted, or deleted sources that
169+ # still have published binaries. We can use scheduleddeletiondate
170+ # rather than linking through BPB, BPR and BPPH since we don't condemn
171+ # sources until their binaries are all gone due to GPL compliance.
172+ clauses = [
173+ SourcePackagePublishingHistory.archiveID == self.id,
174+ SourcePackagePublishingHistory.sourcepackagereleaseID ==
175+ SourcePackageRelease.id,
176+ SourcePackagePublishingHistory.sourcepackagenameID ==
177+ SourcePackageName.id,
178+ SourcePackagePublishingHistory.scheduleddeletiondate == None]
179+
180+ if status:
181 try:
182 status = tuple(status)
183 except TypeError:
184 status = (status, )
185- clauses.append("""
186- SourcePackagePublishingHistory.status IN %s
187- """ % sqlvalues(status))
188-
189- if distroseries is not None:
190- clauses.append("""
191- SourcePackagePublishingHistory.distroseries = %s
192- """ % sqlvalues(distroseries))
193-
194- clauseTables = ['SourcePackageRelease', 'SourcePackageName']
195-
196- order_const = "SourcePackageRelease.version"
197- desc_version_order = SQLConstant(order_const + " DESC")
198- orderBy = ['SourcePackageName.name', desc_version_order,
199- '-SourcePackagePublishingHistory.id']
200-
201- if name is not None:
202- clauses.append("""
203- SourcePackageName.name LIKE '%%' || %s || '%%'
204- """ % quote_like(name))
205-
206- preJoins = ['sourcepackagerelease']
207- sources = SourcePackagePublishingHistory.select(
208- ' AND '.join(clauses), clauseTables=clauseTables, orderBy=orderBy,
209- prejoins=preJoins)
210-
211+ clauses.append(SourcePackagePublishingHistory.status.is_in(status))
212+
213+ if distroseries:
214+ clauses.append(
215+ SourcePackagePublishingHistory.distroseriesID ==
216+ distroseries.id)
217+
218+ if name:
219+ clauses.append(SourcePackageName.name.contains_string(name))
220+
221+ sources = Store.of(self).find(
222+ SourcePackagePublishingHistory, *clauses).order_by(
223+ SourcePackageName.name, Desc(SourcePackageRelease.version),
224+ Desc(SourcePackagePublishingHistory.id))
225+ load_related(SourcePackageRelease, sources, ['sourcepackagereleaseID'])
226 return sources
227
228 @property
229@@ -758,9 +727,7 @@
230 BinaryPackageRelease.version = %s
231 """ % sqlvalues(version))
232 elif ordered:
233- order_const = "BinaryPackageRelease.version"
234- desc_version_order = SQLConstant(order_const + " DESC")
235- orderBy.insert(1, desc_version_order)
236+ orderBy.insert(1, Desc(BinaryPackageRelease.version))
237
238 if status is not None:
239 try:
240@@ -992,8 +959,7 @@
241 Component.name.is_in(components))
242 for (archive, not_used, pocket, components) in deps])
243
244- store = ISlaveStore(BinaryPackagePublishingHistory)
245- return store.find(
246+ return ISlaveStore(BinaryPackagePublishingHistory).find(
247 BinaryPackagePublishingHistory,
248 BinaryPackageName.name == dep_name,
249 BinaryPackagePublishingHistory.binarypackagename ==