Skip to content

Commit de7ec58

Browse files
cbaker6cbaker6
authored
Postgres: prepend className to unique indexes (#6741)
* prepend className to unique index to allow multiple unique indexes for different classes * add testcase * switched test so it can be tested on older versions of parse-server and show failure * get rid of console log messages on restart by checking if the index exists before creating it * add IF NOT EXISTS and IF EXISTS to ALTER TABLE * revert some of code * ensureIndex use IF NOT EXISTS * ALTER TABLE CONSTRAINT can't use IF, ADD/DROP COLUMN can * retesting * update * switchted to CREATE UNIQUE INDEX instrad of ALTER TABLE... ALTER TABLE doesn't seem to be needed
1 parent 8c88d81 commit de7ec58

File tree

2 files changed

+96
-35
lines changed

2 files changed

+96
-35
lines changed

‎spec/PostgresStorageAdapter.spec.js

+86-26
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,7 @@ const getColumns = (client, className) => {
99
return client.map(
1010
'SELECT column_name FROM information_schema.columns WHERE table_name = $<className>',
1111
{ className },
12-
a => a.column_name
12+
(a) => a.column_name
1313
);
1414
};
1515

@@ -25,7 +25,7 @@ describe_only_db('postgres')('PostgresStorageAdapter', () => {
2525
return adapter.deleteAllClasses();
2626
});
2727

28-
it('schemaUpgrade, upgrade the database schema when schema changes', done => {
28+
it('schemaUpgrade, upgrade the database schema when schema changes', (done) => {
2929
const client = adapter._client;
3030
const className = '_PushStatus';
3131
const schema = {
@@ -39,7 +39,7 @@ describe_only_db('postgres')('PostgresStorageAdapter', () => {
3939
adapter
4040
.createTable(className, schema)
4141
.then(() => getColumns(client, className))
42-
.then(columns => {
42+
.then((columns) => {
4343
expect(columns).toContain('pushTime');
4444
expect(columns).toContain('source');
4545
expect(columns).toContain('query');
@@ -49,17 +49,17 @@ describe_only_db('postgres')('PostgresStorageAdapter', () => {
4949
return adapter.schemaUpgrade(className, schema);
5050
})
5151
.then(() => getColumns(client, className))
52-
.then(columns => {
52+
.then((columns) => {
5353
expect(columns).toContain('pushTime');
5454
expect(columns).toContain('source');
5555
expect(columns).toContain('query');
5656
expect(columns).toContain('expiration_interval');
5757
done();
5858
})
59-
.catch(error => done.fail(error));
59+
.catch((error) => done.fail(error));
6060
});
6161

62-
it('schemaUpgrade, maintain correct schema', done => {
62+
it('schemaUpgrade, maintain correct schema', (done) => {
6363
const client = adapter._client;
6464
const className = 'Table';
6565
const schema = {
@@ -73,32 +73,32 @@ describe_only_db('postgres')('PostgresStorageAdapter', () => {
7373
adapter
7474
.createTable(className, schema)
7575
.then(() => getColumns(client, className))
76-
.then(columns => {
76+
.then((columns) => {
7777
expect(columns).toContain('columnA');
7878
expect(columns).toContain('columnB');
7979
expect(columns).toContain('columnC');
8080

8181
return adapter.schemaUpgrade(className, schema);
8282
})
8383
.then(() => getColumns(client, className))
84-
.then(columns => {
84+
.then((columns) => {
8585
expect(columns.length).toEqual(3);
8686
expect(columns).toContain('columnA');
8787
expect(columns).toContain('columnB');
8888
expect(columns).toContain('columnC');
8989

9090
done();
9191
})
92-
.catch(error => done.fail(error));
92+
.catch((error) => done.fail(error));
9393
});
9494

95-
it('Create a table without columns and upgrade with columns', done => {
95+
it('Create a table without columns and upgrade with columns', (done) => {
9696
const client = adapter._client;
9797
const className = 'EmptyTable';
9898
dropTable(client, className)
9999
.then(() => adapter.createTable(className, {}))
100100
.then(() => getColumns(client, className))
101-
.then(columns => {
101+
.then((columns) => {
102102
expect(columns.length).toBe(0);
103103

104104
const newSchema = {
@@ -111,7 +111,7 @@ describe_only_db('postgres')('PostgresStorageAdapter', () => {
111111
return adapter.schemaUpgrade(className, newSchema);
112112
})
113113
.then(() => getColumns(client, className))
114-
.then(columns => {
114+
.then((columns) => {
115115
expect(columns.length).toEqual(2);
116116
expect(columns).toContain('columnA');
117117
expect(columns).toContain('columnB');
@@ -176,10 +176,10 @@ describe_only_db('postgres')('PostgresStorageAdapter', () => {
176176
);
177177
await client
178178
.one(analyzedExplainQuery, [tableName, 'objectId', caseInsensitiveData])
179-
.then(explained => {
179+
.then((explained) => {
180180
const preIndexPlan = explained;
181181

182-
preIndexPlan['QUERY PLAN'].forEach(element => {
182+
preIndexPlan['QUERY PLAN'].forEach((element) => {
183183
//Make sure search returned with only 1 result
184184
expect(element.Plan['Actual Rows']).toBe(1);
185185
expect(element.Plan['Node Type']).toBe('Seq Scan');
@@ -195,17 +195,17 @@ describe_only_db('postgres')('PostgresStorageAdapter', () => {
195195
'objectId',
196196
caseInsensitiveData,
197197
])
198-
.then(explained => {
198+
.then((explained) => {
199199
const postIndexPlan = explained;
200200

201-
postIndexPlan['QUERY PLAN'].forEach(element => {
201+
postIndexPlan['QUERY PLAN'].forEach((element) => {
202202
//Make sure search returned with only 1 result
203203
expect(element.Plan['Actual Rows']).toBe(1);
204204
//Should not be a sequential scan
205205
expect(element.Plan['Node Type']).not.toContain('Seq Scan');
206206

207207
//Should be using the index created for this
208-
element.Plan.Plans.forEach(innerElement => {
208+
element.Plan.Plans.forEach((innerElement) => {
209209
expect(innerElement['Index Name']).toBe(indexName);
210210
});
211211
});
@@ -230,8 +230,8 @@ describe_only_db('postgres')('PostgresStorageAdapter', () => {
230230
'objectId',
231231
caseInsensitiveData,
232232
])
233-
.then(explained => {
234-
explained['QUERY PLAN'].forEach(element => {
233+
.then((explained) => {
234+
explained['QUERY PLAN'].forEach((element) => {
235235
//Check that basic query plans isn't a sequential scan
236236
expect(element.Plan['Node Type']).not.toContain(
237237
'Seq Scan'
@@ -244,7 +244,7 @@ describe_only_db('postgres')('PostgresStorageAdapter', () => {
244244
});
245245
});
246246
})
247-
.catch(error => {
247+
.catch((error) => {
248248
// Query on non existing table, don't crash
249249
if (error.code !== '42P01') {
250250
throw error;
@@ -276,8 +276,8 @@ describe_only_db('postgres')('PostgresStorageAdapter', () => {
276276
{ caseInsensitive: true, explain: true }
277277
);
278278

279-
preIndexPlan.forEach(element => {
280-
element['QUERY PLAN'].forEach(innerElement => {
279+
preIndexPlan.forEach((element) => {
280+
element['QUERY PLAN'].forEach((innerElement) => {
281281
//Check that basic query plans isn't a sequential scan, be careful as find uses "any" to query
282282
expect(innerElement.Plan['Node Type']).toBe('Seq Scan');
283283
//Basic query plans shouldn't have an execution time
@@ -302,8 +302,8 @@ describe_only_db('postgres')('PostgresStorageAdapter', () => {
302302
{ caseInsensitive: true, explain: true }
303303
);
304304

305-
postIndexPlan.forEach(element => {
306-
element['QUERY PLAN'].forEach(innerElement => {
305+
postIndexPlan.forEach((element) => {
306+
element['QUERY PLAN'].forEach((innerElement) => {
307307
//Check that basic query plans isn't a sequential scan
308308
expect(innerElement.Plan['Node Type']).not.toContain('Seq Scan');
309309

@@ -339,13 +339,73 @@ describe_only_db('postgres')('PostgresStorageAdapter', () => {
339339
{ username: caseInsensitiveData },
340340
{ caseInsensitive: true, explain: true }
341341
);
342-
indexPlan.forEach(element => {
343-
element['QUERY PLAN'].forEach(innerElement => {
342+
indexPlan.forEach((element) => {
343+
element['QUERY PLAN'].forEach((innerElement) => {
344344
expect(innerElement.Plan['Node Type']).not.toContain('Seq Scan');
345345
expect(innerElement.Plan['Index Name']).toContain('parse_default');
346346
});
347347
});
348348
});
349+
350+
it('should allow multiple unique indexes for same field name and different class', async () => {
351+
const firstTableName = 'Test1';
352+
const firstTableSchema = new Parse.Schema(firstTableName);
353+
const uniqueField = 'uuid';
354+
firstTableSchema.addString(uniqueField);
355+
await firstTableSchema.save();
356+
await firstTableSchema.get();
357+
358+
const secondTableName = 'Test2';
359+
const secondTableSchema = new Parse.Schema(secondTableName);
360+
secondTableSchema.addString(uniqueField);
361+
await secondTableSchema.save();
362+
await secondTableSchema.get();
363+
364+
const database = Config.get(Parse.applicationId).database;
365+
366+
//Create index before data is inserted
367+
await adapter.ensureUniqueness(firstTableName, firstTableSchema, [
368+
uniqueField,
369+
]);
370+
await adapter.ensureUniqueness(secondTableName, secondTableSchema, [
371+
uniqueField,
372+
]);
373+
374+
//Postgres won't take advantage of the index until it has a lot of records because sequential is faster for small db's
375+
const client = adapter._client;
376+
await client.none(
377+
'INSERT INTO $1:name ($2:name, $3:name) SELECT MD5(random()::text), MD5(random()::text) FROM generate_series(1,5000)',
378+
[firstTableName, 'objectId', uniqueField]
379+
);
380+
await client.none(
381+
'INSERT INTO $1:name ($2:name, $3:name) SELECT MD5(random()::text), MD5(random()::text) FROM generate_series(1,5000)',
382+
[secondTableName, 'objectId', uniqueField]
383+
);
384+
385+
//Check using find method for Parse
386+
const indexPlan = await database.find(
387+
firstTableName,
388+
{ uuid: '1234' },
389+
{ caseInsensitive: false, explain: true }
390+
);
391+
indexPlan.forEach((element) => {
392+
element['QUERY PLAN'].forEach((innerElement) => {
393+
expect(innerElement.Plan['Node Type']).not.toContain('Seq Scan');
394+
expect(innerElement.Plan['Index Name']).toContain(uniqueField);
395+
});
396+
});
397+
const indexPlan2 = await database.find(
398+
secondTableName,
399+
{ uuid: '1234' },
400+
{ caseInsensitive: false, explain: true }
401+
);
402+
indexPlan2.forEach((element) => {
403+
element['QUERY PLAN'].forEach((innerElement) => {
404+
expect(innerElement.Plan['Node Type']).not.toContain('Seq Scan');
405+
expect(innerElement.Plan['Index Name']).toContain(uniqueField);
406+
});
407+
});
408+
});
349409
});
350410

351411
describe_only_db('postgres')('PostgresStorageAdapter shutdown', () => {

‎src/Adapters/Storage/Postgres/PostgresStorageAdapter.js

+10-9
Original file line numberDiff line numberDiff line change
@@ -1128,7 +1128,7 @@ export class PostgresStorageAdapter implements StorageAdapter {
11281128
if (type.type !== 'Relation') {
11291129
try {
11301130
await t.none(
1131-
'ALTER TABLE $<className:name> ADD COLUMN $<fieldName:name> $<postgresType:raw>',
1131+
'ALTER TABLE $<className:name> ADD COLUMN IF NOT EXISTS $<fieldName:name> $<postgresType:raw>',
11321132
{
11331133
className,
11341134
fieldName,
@@ -1271,7 +1271,10 @@ export class PostgresStorageAdapter implements StorageAdapter {
12711271
{ schema, className }
12721272
);
12731273
if (values.length > 1) {
1274-
await t.none(`ALTER TABLE $1:name DROP COLUMN ${columns}`, values);
1274+
await t.none(
1275+
`ALTER TABLE $1:name DROP COLUMN IF EXISTS ${columns}`,
1276+
values
1277+
);
12751278
}
12761279
});
12771280
}
@@ -2063,13 +2066,11 @@ export class PostgresStorageAdapter implements StorageAdapter {
20632066
schema: SchemaType,
20642067
fieldNames: string[]
20652068
) {
2066-
// Use the same name for every ensureUniqueness attempt, because postgres
2067-
// Will happily create the same index with multiple names.
2068-
const constraintName = `unique_${fieldNames.sort().join('_')}`;
2069+
const constraintName = `${className}_unique_${fieldNames.sort().join('_')}`;
20692070
const constraintPatterns = fieldNames.map(
20702071
(fieldName, index) => `$${index + 3}:name`
20712072
);
2072-
const qs = `ALTER TABLE $1:name ADD CONSTRAINT $2:name UNIQUE (${constraintPatterns.join()})`;
2073+
const qs = `CREATE UNIQUE INDEX IF NOT EXISTS $2:name ON $1:name(${constraintPatterns.join()})`;
20732074
return this._client
20742075
.none(qs, [className, constraintName, ...fieldNames])
20752076
.catch(error => {
@@ -2491,7 +2492,7 @@ export class PostgresStorageAdapter implements StorageAdapter {
24912492
return (conn || this._client).tx(t =>
24922493
t.batch(
24932494
indexes.map(i => {
2494-
return t.none('CREATE INDEX $1:name ON $2:name ($3:name)', [
2495+
return t.none('CREATE INDEX IF NOT EXISTS $1:name ON $2:name ($3:name)', [
24952496
i.name,
24962497
className,
24972498
i.key,
@@ -2509,7 +2510,7 @@ export class PostgresStorageAdapter implements StorageAdapter {
25092510
): Promise<void> {
25102511
await (
25112512
conn || this._client
2512-
).none('CREATE INDEX $1:name ON $2:name ($3:name)', [
2513+
).none('CREATE INDEX IF NOT EXISTS $1:name ON $2:name ($3:name)', [
25132514
fieldName,
25142515
className,
25152516
type,
@@ -2588,7 +2589,7 @@ export class PostgresStorageAdapter implements StorageAdapter {
25882589
(fieldName, index) => `lower($${index + 3}:name) varchar_pattern_ops`
25892590
)
25902591
: fieldNames.map((fieldName, index) => `$${index + 3}:name`);
2591-
const qs = `CREATE INDEX $1:name ON $2:name (${constraintPatterns.join()})`;
2592+
const qs = `CREATE INDEX IF NOT EXISTS $1:name ON $2:name (${constraintPatterns.join()})`;
25922593
await conn
25932594
.none(qs, [indexNameOptions.name, className, ...fieldNames])
25942595
.catch(error => {

0 commit comments

Comments
 (0)