Postgres IDENTITY Column support

[mitsos1os]

Description of change

Fixes: #5365
This PR adds the ability to create IDENTITY columns (for Postgres 10+!) using the PrimaryGeneratedColumn decorator, instead of the default SERIAL that was being created so far.

ex:

@Entity()
export class User {
  @PrimaryGeneratedColumn("identity")
  id!: number;
}

The user must be cautious not to use the identity option in the decorator when connecting to a Postgres <10 database because it will throw an error since it is not supported in the DB Engine.

I chose to add identity as an extra generationStrategy instead of adding an extra option flag, because it directly avoids rest of boilerplate code provided from the increment default strategy (Adding Sequences). If extra flag was chosen and increment was kept as a generationStrategy, extra logic would have to be applied to check the flag's value and not create the SERIAL column with its associated SEQUENCES and DEFAULT VALUES accordingly.

Unit tests have been added that test the produced SQL for creating the column in a table.

However no functional tests added that can load the table from the database and apply proper TableColumn options (although the realted code has been added), due to the fact that older Postgres version is used in docker-compose.yml. I could directly upgrade it in the compose file and apply my test, but I am not sure this is a wanted behavior.

Open to suggestions and comments!

Thanks!

Pull-Request Checklist

• Code is up-to-date with the master branch
• npm run lint passes with this change
• npm run test passes with this change
• This pull request links relevant issues as Fixes #0000
• There are new or updated unit tests validating the change
• Documentation has been updated to reflect this change
• The new commits follow conventions explained in CONTRIBUTING.md

[imnotjames]

Why not use update things so that increment uses the IDENTITY syntax when connected to postgres 10?

[mitsos1os]

Why not use update things so that increment uses the IDENTITY syntax when connected to postgres 10?

Hi! Can you please explain what you mean by saying update things? I didn't understand your suggestion

[imnotjames]

Why not use update things so that increment uses the IDENTITY syntax when connected to postgres 10?

Hi! Can you please explain what you mean by saying update things? I didn't understand your suggestion

Change the default behavior of increment for primary generated column so that it uses the new postgres feature when connected to postgres 10.

This is as an alternative to adding a new feature for only postgres 10 which will not be implemented by any other driver.

Is that possible?

[mitsos1os]

@imnotjames From what I have seen there is no information for the version of the Database Server to which the client is connected to. The only thing I found with a quick look is something like that brianc/node-postgres#2002 which would require extra tampering with the embedded driver functionality.

Also I stated in my PR description that I chose a new generation strategy to also circumvent the accompanying SEQUENCE creation that comes with SERIAL column definition.

[imnotjames]

@imnotjames From what I have seen there is no information for the version of the Database Server to which the client is connected to. The only thing I found with a quick look is something like that brianc/node-postgres#2002 which would require extra tampering with the embedded driver functionality.

See #7716

Also I stated in my PR description that I chose a new generation strategy to also circumvent the accompanying SEQUENCE creation that comes with SERIAL column definition.

It seems that this is the new suggested mechanism for increment instead of serial when supported, right?

I don't think we should add yet another generation strategy that's custom for a specific database. I also think we should drop rowid because cockroachdb seems to use that as the default anyway.

[mitsos1os]

Ok thanks! I will take a look.
Just to make sure, will you review this for integration?
What I mean is it would be good to avoid a different opinion from a reviewer that would want to revert this change again

If it is OK, I have no problem changing it to what you suggested

[mitsos1os]

OK, I took a look at the version checking, it is straightforward.
How should I proceed with my PR without re-writing the same code for version checking as here?

[imnotjames]

Ok thanks! I will take a look.
Just to make sure, will you review this for integration?
What I mean is it would be good to avoid a different opinion from a reviewer that would want to revert this change again

If it is OK, I have no problem changing it to what you suggested

I can review it and I have the ability to merge code.

I can't 100% say that it will be merged in.

We can always leave this PR open, create a new branch, get that code working, and then make sure the tests show things are good.

The only thing I'm unsure about is what will happen with existing tables when this change gets in. I think the answer is "nothing" - they'll stay the same, but if they are recreated they would get the new code / sequence / etc. That's probably fine?

[mitsos1os]

@imnotjames OK cool!
Regarding the #7716 PR where the getVersion is implemented, how do you want to handle this since I will have to use the same/similar util?
Should I also create it in my PR?

Also left a comment regarding my view on the implementation here

[imnotjames]

If you need it, write it. If it gets merged in first we'll update that PR to match.

[mitsos1os]

@pleerock thanks for getting back! I have rebased and addressed the comments on your conversations. Take a look and let me know what you think!
Thanks!

[pleerock]

Looks good to me, thank you for contribution!

[mitsos1os]

Great! Looking forward to it getting merged!