feat(pg): PG10 identity columns and preliminary PG11 support #294
Conversation
|
Tests fail on PG <10 because I added identity column syntax to the kitchen sink. Should I add a new schema ( |
|
Call the schema |
There was a problem hiding this comment.
This is awesome; thanks for taking this on!
I've included some feedback, I'd also like to see mutation tests on the new fields - to do so add a test file called something like packages/postgraphile-core/__tests__/fixtures/mutations/pg10.identity-columns.graphql
Add the PG10 schema here:
And add a fileName.startsWith("pg10.") clause here:
with console.log("Skipping test as PG version is less than 10"); return; or similar if the version number is pre 10.
Hopefully doing this in a new schema will mean the snapshot changes should be eradicated (there'll only be some new snapshots instead).
Oh, also please add a schema test so we can spot if the id column randomly comes back at some point (or if it exists in patch / similar input types).
You can copy this file for the schema test, just change the schema list to pg10 👍
| @@ -82,6 +82,7 @@ export interface PgAttribute { | |||
| typeModifier: number; | |||
| isNotNull: boolean; | |||
| hasDefault: boolean; | |||
| identity: string | void; | |||
There was a problem hiding this comment.
Be more explicit: identity: '' | 'a' | 'd' | void;
There was a problem hiding this comment.
Correction: identity: '' | 'a' | 'd'; - I suggest we remove the void option.
There was a problem hiding this comment.
Also please mirror this in the PgIntrospectionPlugin.js types 👍
| const introspectionQuery = await readFile(INTROSPECTION_PATH, "utf8"); | ||
| const { | ||
| rows: [{ server_version_num: serverVersionNum }], | ||
| } = await pgClient.query("show server_version_num;"); |
There was a problem hiding this comment.
I wondered if this might already be available on pgClient as part of the handshake, but given this is in the node-postgres tests:
I'm going to assume not. However I do note that they explicitly parseInt, so we should probably do that also.
| @@ -1,9 +1,10 @@ | |||
| -- @see https://www.postgresql.org/docs/9.5/static/catalogs.html | |||
| function makeIntrospectionQuery(serverVersionNum) { | |||
| return `-- @see https://www.postgresql.org/docs/9.5/static/catalogs.html | |||
There was a problem hiding this comment.
Please use the syntax:
return `\
-- @see ...The backslash escapes the newline so it has no effect - the resulting string will be identical but it's easier to read (and minimises differences).
| @@ -157,6 +158,7 @@ with | |||
| nullif(att.atttypmod, -1) as "typeModifier", | |||
| att.attnotnull as "isNotNull", | |||
| att.atthasdef as "hasDefault", | |||
| ${serverVersionNum >= 100000 ? `att.attidentity as "identity",` : ``} | |||
There was a problem hiding this comment.
Please use straight quotes here since there's no interpolation (stylistic reasons only): 100000 ? 'att.att....",' : ''}
There was a problem hiding this comment.
Actually I think it might be best to always define the columns rather than making void an option; so this should be something like:
${serverVersionNum >= 100000 ? 'att.attidentity' : "''"} as "identity",
This also makes the expected column names easier to scan.
| @@ -1,9 +1,10 @@ | |||
| -- @see https://www.postgresql.org/docs/9.5/static/catalogs.html | |||
| function makeIntrospectionQuery(serverVersionNum) { | |||
There was a problem hiding this comment.
Please convert this file to TypeScript, to do so I think you only need to change this declaration:
export function makeIntrospectionQuery(serverVersionNum: number): string {
There was a problem hiding this comment.
Wait I'm getting confused; graphile-build-pg is not in TypeScript yet. Instead, enable Flow with // @flow comment at the top and the same types (but no export keyword).
|
|
||
| module.exports = { | ||
| makeIntrospectionQuery, | ||
| }; |
| @@ -69,15 +70,15 @@ with | |||
| pro.proisagg = false and | |||
| pro.proiswindow = false and | |||
There was a problem hiding this comment.
According to graphile/crystal#796 these two lines causes PostGraphile to fail to start on PG11; it would be awesome if you could fix this in this PR also, but no worries if not - I can add it after 👍
According to the PG11 docs there's a new attribute prokind:
ffor a normal function,pfor a procedure,afor an aggregate function, orwfor a window function
Seems prokind did not exist before PG11, so it'd be something like:
${serverVersionNum >= 110000 ? "pro.prokind = 'f'" : 'pro.proisagg = false and pro.proiswindow = false'} and
I'm not sure if that's all that's required to support PG11.
To run PG11 without installing you can use Docker:
docker run -it --rm -p 127.0.0.1:54321:5432 postgres:11
To connect, you can do:
psql postgres://postgres@localhost:54321/postgres
For the tests
export TEST_DATABASE_URL="postgres://postgres@localhost:54321/postgres"
There was a problem hiding this comment.
Tests now pass with PG11 🎉
|
Looking good; let me know when you're ready for me to re-review 👍 |
|
All requested changes have been made, apart from adding mutation tests |
|
Awesome; I'll review now 👍 |
|
Looking really good; thanks for doing this Matt! I can look at adding tests for this towards the end of the week if you don't have any more time. I also want to figure out how to get travis to test against PG9, PG10 and PG11 before merging - but again, I'm happy for you to leave that with me - let me know 👍 🙌🙌🙌 |
|
No experience with travis, but I'll give it a shot.. |
|
AFAICT (https://github.com/travis-ci/apt-source-safelist/blob/master/ubuntu.json#L422), there's no way to run PG11 on travis without switching to |
|
Okay; drop the travis commit and we'll merge as-is and test manually for now. I'm not keen for CI to be any slower. |
This reverts commit ac7d49a.
benjie
changed the title
benjie
changed the title
benjie
changed the title
|
🎉 Merged. Looks like there's going to be an RC5 after all! 😂 |
Fixes #244
For columns
generated always as identity, the associated GraphQL input field should be omitted.For columns
generated by default as identity, the associated GraphQL input field should be nullable.