perf: drop unused ORDER BY in old-user-record pruning queries (#2302)

* rmv order_by in queries

* convert to set so that positional order is not relevant

* update to uid comparison so order no longer relevant

Author: taddes

tools/tokenserver/database.py

@@ -86,6 +86,10 @@
 """)
 
 
+# No ORDER BY: pruning is destructive and `purge_old_records.py` uses a random
+# offset for sharding, so stable pagination is not required. Avoiding the sort
+# lets this stop after LIMIT+OFFSET qualifying rows instead of
+# sorting the entire matching set.
 _GET_OLD_USER_RECORDS_FOR_SERVICE = sqltext("""\
 select
     uid, email, generation, keys_changed_at, client_state,
@@ -96,14 +100,14 @@
     users.service = :service
 and
     replaced_at is not null and replaced_at < :timestamp
-order by
-    replaced_at desc, uid desc
 limit
     :limit
 offset
     :offset
 """)
 
+# See note on _GET_OLD_USER_RECORDS_FOR_SERVICE for why this query intentionally
+# has no ORDER BY.
 _GET_OLD_USER_RECORDS_FOR_SERVICE_RANGE = """\
 select
     uid, email, generation, keys_changed_at, client_state,
@@ -116,8 +120,6 @@
     ::RANGE::
 and
     replaced_at is not null and replaced_at < :timestamp
-order by
-    replaced_at desc, uid desc
 limit
     :limit
 offset

tools/tokenserver/purge_old_records.py

@@ -64,7 +64,7 @@ def purge_old_records(
     logger.info("Purging old user records")
     try:
         database = Database()
-        previous_list = []
+        previous_uids = set()
         # Process batches of <max_per_loop> items, until we run out.
         while True:
             offset = random.randint(0, max_offset)
@@ -78,9 +78,10 @@ def purge_old_records(
             if not rows:
                 logger.info("No more data")
                 break
-            if rows == previous_list:
+            current_uids = {row.uid for row in rows}
+            if current_uids == previous_uids:
                 raise Exception("Loop detected")
-            previous_list = rows
+            previous_uids = current_uids
             range_msg = ""
             if uid_range:
                 range_msg = (

tools/tokenserver/test_purge_old_records.py

@@ -135,8 +135,11 @@ def test_purging_of_old_user_records(purge_db, mock_service_server):
     assert len(service_requests) == 2
 
     # Check that the proper delete requests were made to the service.
-    expected_kids = ["0000000000450-uw", "0000000000123-qg"]
-    for i, environ in enumerate(service_requests):
+    # Order is not asserted: the underlying query intentionally has no ORDER BY
+    # (see _GET_OLD_USER_RECORDS_FOR_SERVICE), so kids are compared as a set.
+    expected_kids = {"0000000000450-uw", "0000000000123-qg"}
+    actual_kids = set()
+    for environ in service_requests:
         # They must be to the correct path.
         assert environ["REQUEST_METHOD"] == "DELETE"
         assert re.match("/1.5/[0-9]+", environ["PATH_INFO"])
@@ -148,7 +151,8 @@ def test_purging_of_old_user_records(purge_db, mock_service_server):
         assert "uid" in userdata
         assert "node" in userdata
         assert userdata["fxa_uid"] == "test"
-        assert userdata["fxa_kid"] == expected_kids[i]
+        actual_kids.add(userdata["fxa_kid"])
+    assert actual_kids == expected_kids
 
     # Check that the user's current state is unaffected
     user = database.get_user(email)