Make clientId field in ClientIds non-nullable and unique #9209

Will-Howard · 2024-05-01T14:30:06Z

Both of these constraints are already satisfied on the EAF databases, @jimrandomh @b0b3rt could you check they are true on LW before merging this.

It should be safe to merge if the constraints aren't satisfied, the migration would fail the deployment if there are nulls in the db, and the unique constraint isn't required in the code so would just silently fail. I'm planning to add a query with an ON CONFLICT in a separate PR which will fail without the unique constraint

Creating the unique index takes at least 45 mins on dev (I gave up timing it after that point) so I would strongly recommend running this on prod before deploying

[Update after fixing the concurrently-in-migration problem: I've added a way to access the db client outside the current transaction, which allows us to run CONCURRENTLY queries in migrations, but comes at the cost of breaking atomicity for those queries]

┆Issue is synchronized with this Asana task by Unito

Will-Howard · 2024-05-02T10:56:43Z

packages/lesswrong/lib/collections/clientIds/collection.ts

@@ -20,7 +20,7 @@ addUniversalFields({
  createdAtOptions: {canRead: ['admins']},
 });

-ensureIndex(ClientIds, {clientId: 1});
+ensureIndex(ClientIds, {clientId: 1}, {unique: true, concurrently: true, name: "idx_ClientIds_clientId_unique"});


The reason for the explicit name is that the generated name doesn't include whether the index is unique or not, so it says the index already exists. Ideally we would change CreateIndexQuery so it does include this but making this backwards compatible (i.e. not duplicating all existing unique indexes) would be a pain so I haven't done it for this PR

oetherington

All looks good to me, provided LW are happy

oetherington · 2024-05-02T11:20:12Z

packages/lesswrong/unitTests/sql/CreateIndexQuery.tests.ts

@@ -33,5 +33,11 @@ describe("CreateIndexQuery", () => {
      expectedSql: 'CREATE INDEX IF NOT EXISTS "idx_TestCollection_b_ci" ON "TestCollection" USING btree ( LOWER("b") )',
      expectedArgs: [],
    },
+    {


…nique-client-ids-2024-04

oetherington · 2024-05-08T16:15:10Z

Note that the index will need to be added to the migration now that #9219 has been merged

Will-Howard · 2024-05-09T10:16:58Z

It's now throwing an error when creating the index in the migration due to not being able to do CONCURRENTLY inside a transaction, I'm planning to fix it by passing a runAfterTransaction function to the migration. [Edit: I'm actually going to split this into two PRs, using this as the WIP branch for now]

oetherington · 2024-05-09T10:57:01Z

Sounds reasonable. I'm planning on rewriting a lot of the makemigrations script later today to account for the problems Robert found and it'll also fix the issue with create concurrent indexes in migrations. Feel free to do a quick fix here if you want, but I probably wouldn't bother spending much time on it (you could even just add a quick manual migration if it's easier).

Will-Howard · 2024-05-10T08:36:03Z

packages/lesswrong/server/migrations/meta/utils.ts

@@ -174,7 +175,7 @@ export const updateIndexes = async <N extends CollectionNameString>(
  collection: CollectionBase<N>,
 ): Promise<void> => {
  for (const index of expectedIndexes[collection.collectionName] ?? []) {
-    collection._ensureIndex(index.key, index);
+    await collection._ensureIndex(index.key, index);


This was not being awaited before, I'm not sure what the behaviour would have been wrt what happens when the transaction closes

Huh - it's weird that eslint din't complain. Maybe it doesn't run on this file because migrations aren't actually ever imported - umzug does some magic to read the files. If that's the case we can probably add the directory to the eslint config directly.

It's because the type was any before, so no need to change anything with eslint

oetherington

Nice! Thanks - there's some annoying fidly stuff in there!

oetherington · 2024-05-10T09:11:53Z

schema/accepted_schema.sql

@@ -160,8 +160,8 @@ CREATE TABLE "ClientIds" (
 -- Index "idx_ClientIds_schemaVersion" ON "ClientIds", hash: ec6b84a52c243854cac9404c21e54f56
 CREATE INDEX IF NOT EXISTS "idx_ClientIds_schemaVersion" ON "ClientIds" USING btree ("schemaVersion");

-- Index "idx_ClientIds_clientId" ON "ClientIds", hash: b8febff6f23392c9ca4946ad2e3d7710
-CREATE INDEX IF NOT EXISTS "idx_ClientIds_clientId" ON "ClientIds" USING btree ("clientId");


Small note that we should probably make a follow up PR once that has been deployed to all sites that removes the old index

oetherington · 2024-05-15T13:04:52Z

packages/lesswrong/lib/sql/CreateIndexQuery.ts

@@ -19,14 +19,15 @@ import { StringType } from "./Type";
 class CreateIndexQuery<T extends DbObject> extends Query<T> {
  private isUnique: boolean;

-  constructor(table: Table<T>, index: TableIndex<T>, ifNotExists = true) {
+  constructor(table: Table<T>, index: TableIndex<T>, ifNotExists = true, allowConcurrent=true) {


Rather than having multiple optional booleans, I think it'd be cleaner to have an object with named options

oetherington · 2024-05-15T13:07:43Z

packages/lesswrong/server/migrations/meta/utils.ts

@@ -174,7 +175,7 @@ export const updateIndexes = async <N extends CollectionNameString>(
  collection: CollectionBase<N>,
 ): Promise<void> => {
  for (const index of expectedIndexes[collection.collectionName] ?? []) {
-    collection._ensureIndex(index.key, index);
+    await collection._ensureIndex(index.key, index);


Huh - it's weird that eslint din't complain. Maybe it doesn't run on this file because migrations aren't actually ever imported - umzug does some magic to read the files. If that's the case we can probably add the directory to the eslint config directly.

make clientId field in ClientIds non-nullable and unique

5c831fc

Will-Howard requested a review from a team as a code owner May 1, 2024 14:30

Will-Howard requested review from oetherington and removed request for a team May 1, 2024 14:30

jpaddison3 assigned oetherington May 1, 2024

Will-Howard marked this pull request as draft May 1, 2024 14:30

make create concurrent and add test

69800a8

Will-Howard commented May 2, 2024

View reviewed changes

fix test

5d39bec

Will-Howard marked this pull request as ready for review May 2, 2024 11:03

oetherington reviewed May 2, 2024

View reviewed changes

oetherington assigned Will-Howard and unassigned oetherington May 2, 2024

Will-Howard mentioned this pull request May 8, 2024

Support stale-while-revalidate caching #9204

Merged

Merge branch 'master' of github.com:ForumMagnum/ForumMagnum into wh-u…

ff46c8f

…nique-client-ids-2024-04

fix conflicts

898e08b

Will-Howard marked this pull request as draft May 9, 2024 10:51

standardise removal of CONCURRENTLY from accepted_schema.sql

2a28acb

Will-Howard added 2 commits May 10, 2024 09:25

run CONCURRENTLY queries outside transaction

1e0eaab

remove unneeded migration

59ae397

Will-Howard commented May 10, 2024

View reviewed changes

Will-Howard marked this pull request as ready for review May 10, 2024 08:39

Will-Howard marked this pull request as draft May 10, 2024 13:34

Will-Howard force-pushed the wh-unique-client-ids-2024-04 branch 3 times, most recently from efd9ffd to 59ae397 Compare May 10, 2024 14:05

comment out troublesome migration

2f0b1b7

Will-Howard force-pushed the wh-unique-client-ids-2024-04 branch from 57d9cb4 to 2f0b1b7 Compare May 10, 2024 15:50

merge with updated makeMigrations

78b67ee

Will-Howard force-pushed the wh-unique-client-ids-2024-04 branch from 97f6d97 to f752085 Compare May 13, 2024 14:22

void CONCURRENTLY index creation

5ca526e

Will-Howard force-pushed the wh-unique-client-ids-2024-04 branch from b4dd610 to 5ca526e Compare May 13, 2024 14:36

fix console log

0036540

Will-Howard marked this pull request as ready for review May 13, 2024 14:45

Will-Howard assigned oetherington and unassigned Will-Howard May 13, 2024

oetherington approved these changes May 15, 2024

View reviewed changes

oetherington assigned Will-Howard and unassigned oetherington May 15, 2024

destructure CreateIndexQuery params

ac1123c

Will-Howard merged commit be3b543 into master May 16, 2024
14 checks passed

Will-Howard mentioned this pull request May 26, 2024

Make clientId field non-nullable with manual migration #9333

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make clientId field in ClientIds non-nullable and unique #9209

Make clientId field in ClientIds non-nullable and unique #9209

Will-Howard commented May 1, 2024 •

edited

Will-Howard May 2, 2024

oetherington left a comment

oetherington May 2, 2024

oetherington commented May 8, 2024

Will-Howard commented May 9, 2024 •

edited

oetherington commented May 9, 2024

Will-Howard May 10, 2024

oetherington May 15, 2024

Will-Howard May 16, 2024

oetherington left a comment

oetherington May 10, 2024

oetherington May 15, 2024

oetherington May 15, 2024

Make clientId field in ClientIds non-nullable and unique #9209

Make clientId field in ClientIds non-nullable and unique #9209

Conversation

Will-Howard commented May 1, 2024 • edited

Choose a reason for hiding this comment

oetherington left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oetherington commented May 8, 2024

Will-Howard commented May 9, 2024 • edited

oetherington commented May 9, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oetherington left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Will-Howard commented May 1, 2024 •

edited

Will-Howard commented May 9, 2024 •

edited