Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Postgres: prepend className to unique indexes #6741

Merged
merged 13 commits into from
Oct 12, 2020
Merged

Postgres: prepend className to unique indexes #6741

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
fb33713
prepend className to unique index to allow multiple unique indexes fo…
cbaker6 Jun 19, 2020
839cee8
add testcase
cbaker6 Jun 22, 2020
74bc9ec
switched test so it can be tested on older versions of parse-server a…
cbaker6 Jun 22, 2020
832f645
get rid of console log messages on restart by checking if the index e…
cbaker6 Jun 24, 2020
9b34990
add IF NOT EXISTS and IF EXISTS to ALTER TABLE
cbaker6 Jun 24, 2020
7aace75
revert some of code
cbaker6 Jun 24, 2020
533ca0a
ensureIndex use IF NOT EXISTS
cbaker6 Jun 24, 2020
84d5d1f
ALTER TABLE CONSTRAINT can't use IF, ADD/DROP COLUMN can
cbaker6 Jun 24, 2020
b0f781d
retesting
cbaker6 Jun 24, 2020
e1bafd7
update
cbaker6 Jun 24, 2020
1481aba
switchted to CREATE UNIQUE INDEX instrad of ALTER TABLE... ALTER TABL…
cbaker6 Jun 24, 2020
8aeff77
Merge branch 'master' into fix_unique_index
cbaker6 Jul 29, 2020
815cc7d
Merge branch 'master' into fix_unique_index
cbaker6 Oct 11, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
112 changes: 86 additions & 26 deletions spec/PostgresStorageAdapter.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ const getColumns = (client, className) => {
return client.map(
'SELECT column_name FROM information_schema.columns WHERE table_name = $<className>',
{ className },
a => a.column_name
(a) => a.column_name
);
};

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

it('schemaUpgrade, upgrade the database schema when schema changes', done => {
it('schemaUpgrade, upgrade the database schema when schema changes', (done) => {
const client = adapter._client;
const className = '_PushStatus';
const schema = {
Expand All @@ -39,7 +39,7 @@ describe_only_db('postgres')('PostgresStorageAdapter', () => {
adapter
.createTable(className, schema)
.then(() => getColumns(client, className))
.then(columns => {
.then((columns) => {
expect(columns).toContain('pushTime');
expect(columns).toContain('source');
expect(columns).toContain('query');
Expand All @@ -49,17 +49,17 @@ describe_only_db('postgres')('PostgresStorageAdapter', () => {
return adapter.schemaUpgrade(className, schema);
})
.then(() => getColumns(client, className))
.then(columns => {
.then((columns) => {
expect(columns).toContain('pushTime');
expect(columns).toContain('source');
expect(columns).toContain('query');
expect(columns).toContain('expiration_interval');
done();
})
.catch(error => done.fail(error));
.catch((error) => done.fail(error));
});

it('schemaUpgrade, maintain correct schema', done => {
it('schemaUpgrade, maintain correct schema', (done) => {
const client = adapter._client;
const className = 'Table';
const schema = {
Expand All @@ -73,32 +73,32 @@ describe_only_db('postgres')('PostgresStorageAdapter', () => {
adapter
.createTable(className, schema)
.then(() => getColumns(client, className))
.then(columns => {
.then((columns) => {
expect(columns).toContain('columnA');
expect(columns).toContain('columnB');
expect(columns).toContain('columnC');

return adapter.schemaUpgrade(className, schema);
})
.then(() => getColumns(client, className))
.then(columns => {
.then((columns) => {
expect(columns.length).toEqual(3);
expect(columns).toContain('columnA');
expect(columns).toContain('columnB');
expect(columns).toContain('columnC');

done();
})
.catch(error => done.fail(error));
.catch((error) => done.fail(error));
});

it('Create a table without columns and upgrade with columns', done => {
it('Create a table without columns and upgrade with columns', (done) => {
const client = adapter._client;
const className = 'EmptyTable';
dropTable(client, className)
.then(() => adapter.createTable(className, {}))
.then(() => getColumns(client, className))
.then(columns => {
.then((columns) => {
expect(columns.length).toBe(0);

const newSchema = {
Expand All @@ -111,7 +111,7 @@ describe_only_db('postgres')('PostgresStorageAdapter', () => {
return adapter.schemaUpgrade(className, newSchema);
})
.then(() => getColumns(client, className))
.then(columns => {
.then((columns) => {
expect(columns.length).toEqual(2);
expect(columns).toContain('columnA');
expect(columns).toContain('columnB');
Expand Down Expand Up @@ -176,10 +176,10 @@ describe_only_db('postgres')('PostgresStorageAdapter', () => {
);
await client
.one(analyzedExplainQuery, [tableName, 'objectId', caseInsensitiveData])
.then(explained => {
.then((explained) => {
const preIndexPlan = explained;

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

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

//Should be using the index created for this
element.Plan.Plans.forEach(innerElement => {
element.Plan.Plans.forEach((innerElement) => {
expect(innerElement['Index Name']).toBe(indexName);
});
});
Expand All @@ -230,8 +230,8 @@ describe_only_db('postgres')('PostgresStorageAdapter', () => {
'objectId',
caseInsensitiveData,
])
.then(explained => {
explained['QUERY PLAN'].forEach(element => {
.then((explained) => {
explained['QUERY PLAN'].forEach((element) => {
//Check that basic query plans isn't a sequential scan
expect(element.Plan['Node Type']).not.toContain(
'Seq Scan'
Expand All @@ -244,7 +244,7 @@ describe_only_db('postgres')('PostgresStorageAdapter', () => {
});
});
})
.catch(error => {
.catch((error) => {
// Query on non existing table, don't crash
if (error.code !== '42P01') {
throw error;
Expand Down Expand Up @@ -276,8 +276,8 @@ describe_only_db('postgres')('PostgresStorageAdapter', () => {
{ caseInsensitive: true, explain: true }
);

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

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

Expand Down Expand Up @@ -339,13 +339,73 @@ describe_only_db('postgres')('PostgresStorageAdapter', () => {
{ username: caseInsensitiveData },
{ caseInsensitive: true, explain: true }
);
indexPlan.forEach(element => {
element['QUERY PLAN'].forEach(innerElement => {
indexPlan.forEach((element) => {
element['QUERY PLAN'].forEach((innerElement) => {
expect(innerElement.Plan['Node Type']).not.toContain('Seq Scan');
expect(innerElement.Plan['Index Name']).toContain('parse_default');
});
});
});

it('should allow multiple unique indexes for same field name and different class', async () => {
const firstTableName = 'Test1';
const firstTableSchema = new Parse.Schema(firstTableName);
const uniqueField = 'uuid';
firstTableSchema.addString(uniqueField);
await firstTableSchema.save();
await firstTableSchema.get();

const secondTableName = 'Test2';
const secondTableSchema = new Parse.Schema(secondTableName);
secondTableSchema.addString(uniqueField);
await secondTableSchema.save();
await secondTableSchema.get();

const database = Config.get(Parse.applicationId).database;

//Create index before data is inserted
await adapter.ensureUniqueness(firstTableName, firstTableSchema, [
uniqueField,
]);
await adapter.ensureUniqueness(secondTableName, secondTableSchema, [
uniqueField,
]);

//Postgres won't take advantage of the index until it has a lot of records because sequential is faster for small db's
const client = adapter._client;
await client.none(
'INSERT INTO $1:name ($2:name, $3:name) SELECT MD5(random()::text), MD5(random()::text) FROM generate_series(1,5000)',
[firstTableName, 'objectId', uniqueField]
);
await client.none(
'INSERT INTO $1:name ($2:name, $3:name) SELECT MD5(random()::text), MD5(random()::text) FROM generate_series(1,5000)',
[secondTableName, 'objectId', uniqueField]
);

//Check using find method for Parse
const indexPlan = await database.find(
firstTableName,
{ uuid: '1234' },
{ caseInsensitive: false, explain: true }
);
indexPlan.forEach((element) => {
element['QUERY PLAN'].forEach((innerElement) => {
expect(innerElement.Plan['Node Type']).not.toContain('Seq Scan');
expect(innerElement.Plan['Index Name']).toContain(uniqueField);
});
});
const indexPlan2 = await database.find(
secondTableName,
{ uuid: '1234' },
{ caseInsensitive: false, explain: true }
);
indexPlan2.forEach((element) => {
element['QUERY PLAN'].forEach((innerElement) => {
expect(innerElement.Plan['Node Type']).not.toContain('Seq Scan');
expect(innerElement.Plan['Index Name']).toContain(uniqueField);
});
});
});
});

describe_only_db('postgres')('PostgresStorageAdapter shutdown', () => {
Expand Down
19 changes: 10 additions & 9 deletions src/Adapters/Storage/Postgres/PostgresStorageAdapter.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1128,7 +1128,7 @@ export class PostgresStorageAdapter implements StorageAdapter {
if (type.type !== 'Relation') {
try {
await t.none(
'ALTER TABLE $<className:name> ADD COLUMN $<fieldName:name> $<postgresType:raw>',
'ALTER TABLE $<className:name> ADD COLUMN IF NOT EXISTS $<fieldName:name> $<postgresType:raw>',
{
className,
fieldName,
Expand Down Expand Up @@ -1271,7 +1271,10 @@ export class PostgresStorageAdapter implements StorageAdapter {
{ schema, className }
);
if (values.length > 1) {
await t.none(`ALTER TABLE $1:name DROP COLUMN ${columns}`, values);
await t.none(
`ALTER TABLE $1:name DROP COLUMN IF EXISTS ${columns}`,
values
);
}
});
}
Expand Down Expand Up @@ -2063,13 +2066,11 @@ export class PostgresStorageAdapter implements StorageAdapter {
schema: SchemaType,
fieldNames: string[]
) {
// Use the same name for every ensureUniqueness attempt, because postgres
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here’s the main part of the bug. The assumption that was made in the comments is incorrect

// Will happily create the same index with multiple names.
const constraintName = `unique_${fieldNames.sort().join('_')}`;
const constraintName = `${className}_unique_${fieldNames.sort().join('_')}`;
const constraintPatterns = fieldNames.map(
(fieldName, index) => `$${index + 3}:name`
);
const qs = `ALTER TABLE $1:name ADD CONSTRAINT $2:name UNIQUE (${constraintPatterns.join()})`;
const qs = `CREATE UNIQUE INDEX IF NOT EXISTS $2:name ON $1:name(${constraintPatterns.join()})`;
return this._client
.none(qs, [className, constraintName, ...fieldNames])
.catch(error => {
Expand Down Expand Up @@ -2491,7 +2492,7 @@ export class PostgresStorageAdapter implements StorageAdapter {
return (conn || this._client).tx(t =>
t.batch(
indexes.map(i => {
return t.none('CREATE INDEX $1:name ON $2:name ($3:name)', [
return t.none('CREATE INDEX IF NOT EXISTS $1:name ON $2:name ($3:name)', [
i.name,
className,
i.key,
Expand All @@ -2509,7 +2510,7 @@ export class PostgresStorageAdapter implements StorageAdapter {
): Promise<void> {
await (
conn || this._client
).none('CREATE INDEX $1:name ON $2:name ($3:name)', [
).none('CREATE INDEX IF NOT EXISTS $1:name ON $2:name ($3:name)', [
fieldName,
className,
type,
Expand Down Expand Up @@ -2588,7 +2589,7 @@ export class PostgresStorageAdapter implements StorageAdapter {
(fieldName, index) => `lower($${index + 3}:name) varchar_pattern_ops`
)
: fieldNames.map((fieldName, index) => `$${index + 3}:name`);
const qs = `CREATE INDEX $1:name ON $2:name (${constraintPatterns.join()})`;
const qs = `CREATE INDEX IF NOT EXISTS $1:name ON $2:name (${constraintPatterns.join()})`;
await conn
.none(qs, [indexNameOptions.name, className, ...fieldNames])
.catch(error => {
Expand Down