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

Refactoring method createTable #4456

Merged
merged 5 commits into from
Dec 26, 2017
Merged

Refactoring method createTable #4456

Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
2146a57
Refactoring method createTable
vitaly-t Dec 24, 2017
e07da41
Update PostgresStorageAdapter.js
vitaly-t Dec 24, 2017
78c63c4
Update PostgresStorageAdapter.js
vitaly-t Dec 24, 2017
d2fdc64
Update PostgresStorageAdapter.js
vitaly-t Dec 24, 2017
c2f00ce
Merge branch 'master' into vitaly-t-patch-1
vitaly-t Dec 25, 2017
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
32 changes: 16 additions & 16 deletions src/Adapters/Storage/Postgres/PostgresStorageAdapter.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -684,6 +684,7 @@ export class PostgresStorageAdapter {
// Just create a table, do not insert in schema
createTable(className, schema, conn) {
conn = conn || this._client;
const self = this;
Copy link
Contributor

Choose a reason for hiding this comment

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

quick question, shouldn't we use .bind(this) instead of self capturing?

debug('createTable', className, schema);
const valuesArray = [];
const patternsArray = [];
Expand Down Expand Up @@ -721,24 +722,23 @@ export class PostgresStorageAdapter {
});
const qs = `CREATE TABLE IF NOT EXISTS $1:name (${patternsArray.join(',')})`;
const values = [className, ...valuesArray];
return conn.task(t => {
return this._ensureSchemaCollectionExists(t)
.then(() => t.none(qs, values))
.catch(error => {
if (error.code === PostgresDuplicateRelationError) {
// Table already exists, must have been created by a different request. Ignore error.
} else {

return conn.task('create-table', function * (t) {
try {
yield self._ensureSchemaCollectionExists(t);
yield t.none(qs, values);
} catch(error) {
if (error.code !== PostgresDuplicateRelationError) {
throw error;
}})
})
.then(() => {
return conn.tx('create-relation-tables', t => {
const queries = relations.map((fieldName) => {
return t.none('CREATE TABLE IF NOT EXISTS $<joinTable:name> ("relatedId" varChar(120), "owningId" varChar(120), PRIMARY KEY("relatedId", "owningId") )', {joinTable: `_Join:${fieldName}:${className}`});
});
return t.batch(queries);
});
}
// ELSE: Table already exists, must have been created by a different request. Ignore the error.
}
yield t.tx('create-table-tx', tx => {
return tx.batch(relations.map(fieldName => {
Copy link
Member

@dplewis dplewis Dec 25, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

@vitaly-t Merry Christmas. Quick question. I've notice you got rid of t.batch in favor of t.none(this._pgp.helpers(...)) in other functions. Is there a reason why you use it here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@dplewis as per that PR, it is a performance optimization, to execute all queries via a single IO operation, as opposed to executing them separately. It is much faster that way ;)

Copy link
Member

@dplewis dplewis Dec 26, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

I always thought .batch was a single I/O operation.

Copy link
Member

Choose a reason for hiding this comment

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

Looks good to me 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

batch is an operation irrelevant as to the IO, it simply resolves an array of promises, like Promise.all does, except it also settles all the promises.

return tx.none('CREATE TABLE IF NOT EXISTS $<joinTable:name> ("relatedId" varChar(120), "owningId" varChar(120), PRIMARY KEY("relatedId", "owningId") )', {joinTable: `_Join:${fieldName}:${className}`});
}));
});
});
}

addFieldIfNotExists(className, fieldName, type) {
Expand Down