-
-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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.
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
PG: Index Support and Improvements #4629
Conversation
Codecov Report
@@ Coverage Diff @@
## master #4629 +/- ##
==========================================
- Coverage 92.88% 9.95% -82.94%
==========================================
Files 119 119
Lines 8836 8934 +98
==========================================
- Hits 8207 889 -7318
- Misses 629 8045 +7416
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
a few nits with the flow types otherwise LGTM.
@@ -1355,13 +1372,15 @@ export class PostgresStorageAdapter implements StorageAdapter { | |||
|
|||
const qs = `SELECT ${columns} FROM $1:name ${wherePattern} ${sortPattern} ${limitPattern} ${skipPattern}`; | |||
debug(qs, values); | |||
return this._client.any(qs, values) | |||
.catch(error => { | |||
// @flow-disable-next |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please remove :) we try to enforce flow in ther.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I keep getting Missing type annotation for U.
Don't know how to resolve it.
} | ||
return []; | ||
return Promise.reject(err); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why now throw err
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No Idea I wrote this like a month ago
@@ -1745,28 +1764,132 @@ export class PostgresStorageAdapter implements StorageAdapter { | |||
}); | |||
} | |||
|
|||
createIndex(className: string, index: any, type: string = 'btree', conn: ?any) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
make it a Promise<void>
as return type please.
@vitaly-t Can you look at this when you have a moment? |
@flovilmart I noticed something when I'm using updateSchemaWithIndexes on startup. In DatabaseController.js -> performInitialization() return Promise.all([usernameUniqueness, emailUniqueness, roleUniqueness, adapterInit, indexPromise]); Shouldn't I get the indexes for |
@dplewis which indexes are you expecting? |
I'll look into it.
|
@flovilmart I think I got it |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
@flovilmart Almost forgot about this one. |
Looks good, but I have one concern, it seems that the index names are not consistent from one DB to the other and those get exposed up the JSON responses. |
@flovilmart The indexes will be consistent excluding the Do you see any potential issues with exposing the names in the JSON Response? I use the index names in the test cases |
@flovilmart In the case of same index with different name, mongo and pg have different outputs.
Mongo: Creates name_index_1 and ignores name_index_2 (no error) PG: Creates both name_index_1 and name_index_2 (no error) To have consistency which one makes sense or should error be thrown? |
fields.forEach((field) => { | ||
if (field === 'objectId') { | ||
key = { _id: 1 }; | ||
name = '_id_'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn't this preclude compound indices that include _id
as a field?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think so, I'll write a test case for it. Regarding #4719 (comment) can you show me which line you are talking about.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In your tests the schema you are expecting uses the _id
field. While that is what backs objectId in the mongo backend, what I would expect Parse to store and report to the user is _id_: { objectId: 1 }
since "objectId" is the name of the field in the parse interface. It makes it easier to write tooling if the name in indexes
matches the name in fields
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It might introduce breaking change. I'm all for changing it tho.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is definitely a breaking change but it seems unlikely anybody is using index support right now since it basically doesn't work. Parse isn't even consistent in whether it returns the _id_
index. @TylerBrock Thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Index support currently works for Mongo, this PR is more geared towards Postgres so there will be some inconsistencies.
If we are introducing breaking changes, I'd recommend removing the ability for users to add names to their indexes, and have parse autogenerate them. This makes it easier to keep track of duplicate indexes, transforming keys, and provide support for future DB adapters. Thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It technically works but you can't create indices on most of the fields you would want to do so (pointers, updatedAt, etc).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are test for compound text indexes, I'll write a few more for compound that include (objectId, pointers, updatedAt, etc)
@flovilmart Have you seen this? #4629 (comment) |
@dplewis is there any issue with going forward with the current implementation and keep different behaviors if it’s not problematic? |
@dplewis build looks broken on the CI, can you double check? |
@dplewis any chance we can get this one in? |
@flovilmart Sorry for the late reply. Had a lot of emails from stalebot. I'll try to get this in, its gone stale |
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
Closes: #4719
Seeing how far this goes...