List queries can end their sort on a column two rows share, and nothing checks the last sort key #225
No Assignees
Notifications
Due Date
No due date set.
Dependencies
No dependencies set.
Reference: wiki-cafe/member-console#225
Reference in New Issue
Block a user
What a contributor runs into
A query that sorts on a column two rows can share returns those rows in whatever order the query plan produces, and that order can change from one run to the next. Seven paged operator lists (grants, products, organizations, people, billing accounts, subscriptions and domain claims) have been fixed by ending their sort on the row id, the shape
ListEntitlementSetChangesuses:ORDER BY c.effective_at DESC, c.created_at DESC, c.change_id DESC(internal/entitlements/queries/entitlement_set_changes.sql:132). Ids are UUIDv7 (docs/identifiers.md:23) and rise in the order rows are written, so the id breaks a tie by write order.These queries still end on a column that ties:
ListActiveSitesByWorkspace,ListActiveSitesByPoolandListForceReducedSitesByPool(internal/integrations/fedwiki/store/queries/sites.sql:154,:165,:177) sort bylast_modified_at DESC NULLS LAST, created_at DESC. A downgrade keeps the first sites of the pool's list and parks the rest, and an upgrade reactivates parked sites in list order (reconcilePoolQuota,internal/integrations/fedwiki/workflows/reconcile.go:154-185). A swap parks the last site of the workspace's list (swapActiveSite,internal/integrations/fedwiki/web/site_actions.go:592-603).created_atdefaults toNOW()(internal/integrations/fedwiki/store/migrations/00001_init.sql:41), the start of the writing transaction, so two sites written in one transaction with the samelast_modified_at, or none, tie. I have not found a production path that writes two sites in one transaction, so this is a gap in the query, not an observed failure.ListOrganizationsByType(internal/organization/queries/organizations.sql:90) sorts by name alone; what that does to the default change and the reorder is described on #187.ListAllProducts(internal/billing/queries/products.sql:28) andListSharedTierProducts(internal/billing/queries/plan_ladders.sql:130) sort by product name, which is not unique. The plan topology overview (internal/server/operator_topology.go:124-140) lists same-named products, shared tiers and add-ons alike, in either order.SearchOrganizationsByName(internal/organization/queries/organizations.sql:84) andLookupPersons(internal/identity/queries/persons.sql:42-44) sort by name and stop at a limit. When more rows match than the limit, which ones the operator lookup lists is not defined.ListDeliveringGrantsByOrgID(internal/entitlements/queries/grants.sql:364) sorts bycreated_atalone. Grants written in one transaction share it, so a member's Sources list can show them in either order.ListEntitlementSetChangeEffects(internal/entitlements/queries/entitlement_set_changes.sql:149) sorts one rule change's effects by organization name and key, and two organizations with one name tie. Only a test calls it today; a review of a rule change's effects (#113) would read it.ListFailedObligations(internal/entitlements/queries/entitlement_set_changes.sql:101) ends ono.pool_id, which looks unique and is not: one pool can have failed obligations under two changes of the same set, and the query stops at a limit.Three cases have their own issues: the organization page's members list (#210), the rows
core.conferwrites (#218), and sign-in taking the first organization and workspace by name (#122).Why it costs
A tie shows up as a golden test that fails now and then, a page that reorders on refresh, or a pager that lists one row on two pages and another on none. People learn to rerun, and a real change in order then gets rerun too. In the FedWiki lists the tie decides which of a member's sites goes read-only. Each fix so far was made by hand, one query at a time, and nothing stops the next query from repeating the mistake.
A rough scan, a script that reads the last key of each ORDER BY, finds 163 ORDER BY clauses in the 61 tracked
queries/*.sqlfiles. 143 of them order a query's own result, and 87 of those end on a column not namedidor*_id, most often a name (19),created_at(18) orresource_key(13). Many of the 87 are unique already (a domain, a key, a provider name), and an ending on*_idis not always unique, asListFailedObligationsshows. A check on the column's name alone is therefore not enough.Where
Every
queries/*.sqlfile beside a package'ssqlc.yaml; the project lint (internal/lint, run bygo run . lintfrommake lint);docs/go-conventions.md.Done when
make lintfails on a query whose ORDER BY does not end on a unique key, unless an allowlist names the query with a reason (<package path>.<Name> # reason), the wayscripts/sqlc-unused/allow.txtnames the unused queries that stay. An entry whose query was fixed or deleted fails too. A first version can accept a last key namedidor*_idand require an entry for every other ending. A stricter version reads the primary keys and unique constraints the migrations declare, so it also catches an ending likeListFailedObligations'pool_id.scripts/sqlc-unusedalready finds every package's query files, and the rule can share that walk.docs/go-conventions.mdstates the rule and names the check.Related: #165 reworks the FedWiki paths that change a site's status, the swap and the downgrade sweep among them, which read the three site lists.
FedWiki's three lists,
ListActiveSitesByWorkspace,ListActiveSitesByPoolandListForceReducedSitesByPool, now end onsite_id DESC, so two sites that tie on every other column come back in one order and the sweep parks the same one every pass (9027c0b1). The lint rule and the other lists remain.