perf: drop unused ORDER BY in old-user-record pruning queries

[taddes]

Description

The two _GET_OLD_USER_RECORDS_FOR_SERVICE queries in database.py are used by
purge_old_records.py sort their entire matching set by
(replaced_at DESC, uid DESC) before applying LIMIT/OFFSET. In production
the matching set is a lot of rows, so the planner must sort them
all to hand back 10, repeated on every iteration of the purge loop, which is very much suboptimal.

This removes that ORDER BY from both queries.

Also had to update relevant tests in test_purge_old_recordssince the test enumerated service_requests by index and compared each fxa_kid positionally, relying on the dropped ORDER BY replaced_at DESC to make the order deterministic. The test's actual intent is "both replaced records were purged with valid HAWK-signed DELETE
requests against the storage node" order isn't part of it. Comparing as a set makes that intent explicit and keeps the test stable regardless of scan order.

Testing

How should reviewers test?

Issue(s)

Closes STOR-117.

[chenba]

The fetched records are checked in

syncstorage-rs/tools/tokenserver/purge_old_records.py

Lines 81 to 82 in 004d027

	if rows == previous_list:
	raise Exception("Loop detected")

. Without sorting, that check works differently; the same set but in different order is not considered a "loop". Might be okay in practice though.

[taddes]

The fetched records are checked in

syncstorage-rs/tools/tokenserver/purge_old_records.py

Lines 81 to 82 in 004d027

	if rows == previous_list:
	raise Exception("Loop detected")

. Without sorting, that check works differently; the same set but in different order is not considered a "loop". Might be okay in practice though.

The fetched records are checked in

syncstorage-rs/tools/tokenserver/purge_old_records.py

Lines 81 to 82 in 004d027

	if rows == previous_list:
	raise Exception("Loop detected")

. Without sorting, that check works differently; the same set but in different order is not considered a "loop". Might be okay in practice though.

Thanks for pointing this out. The test change plus this made me think then that you could have cases where the loop would not be detected if the lists are technically the same, but the items inside them are in a different order. Moved it to check on UID comparison and to use a set for comparison, like in the test change.

[pjenvey]

Looking at it more closely, similarly I'm also a little hesitant about this as pagination (limit+offset) is generally discouraged without an order by or something similar, but I'm not sure that matters here.

[pjenvey]

Looking at it more closely, similarly I'm also a little hesitant about this as pagination (limit+offset) is generally discouraged without an order by or something similar, but I'm not sure that matters here.

Per Taddes: "the way this script is doing this is iterative deletion, not read-only pagination. Get the rows, delete these rows, fetch next batch and repeat until empty. The processed rows are gone before the next query runs. So scan order doesn’t really matter"

On top of that we default to a hardcoded offset of 0, and when one is set it's a random offset anyway. So I don't see how potentially inconsistent ordering would ever come in to play here 👍