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.

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

Create versioninfo table with primary key. #1743

Merged
merged 9 commits into from
May 19, 2024

Conversation

vaital3000
Copy link
Contributor

@vaital3000 vaital3000 commented Mar 13, 2024

We use MySQL with option "sql_require_primary_key", so we can't use fluentmigrator now, because we can't create table without PK.

I replaced VersionMigration.cs realization in my project and it's works fine, but it can be needed for somebody yet.

@jzabroski
Copy link
Collaborator

I would rather we not mix the unique constraint with the primary key constraint. Otherwise, I like the overall approach. Do you think you can refactor this so that the two are distinct? I do realize the drawback of my request is extra storage space for a second index, but as it is, without a primary key it is going to be stored as a heap, so it should actually be more efficient.

@jzabroski jzabroski self-requested a review March 19, 2024 14:42
@jzabroski
Copy link
Collaborator

@vaital3000 Hi, we have not heard back from you. Can you please update the PR and we can then merge it?

@vaital3000
Copy link
Contributor Author

@jzabroski Hi, sory, i did'n see your message without mention. I want to test you suggestion and will come back with fixes.

@jzabroski
Copy link
Collaborator

Great. Again, no issue with the core concept to address mysql feature. Just think it should not be a breaking change.

@vaital3000
Copy link
Contributor Author

@jzabroski
I rolled back the condition when creating a unique index, it works fine.

To ensure that the change is non-breaking, I added a property CreateWithPrimaryKey to IVersionTableMetaData with a default value of false.

Copy link
Collaborator

@jzabroski jzabroski left a comment

Choose a reason for hiding this comment

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

@schambers @fubar-coder Do you see any downside to the default setting for CreateWithPrimaryKey being true? This would go into 6.0.0 ideally. I'm trying to think if any databases disallow combining unique constraints and primary keys and I don't know of any.

@schambers
Copy link
Member

I think if anything, certain databases may throw an error if attempted to combine unique constraint and primary key on same column but perhaps we determine if thats the case during integration testing. This shouldn't cause any issues

@jzabroski
Copy link
Collaborator

I should finish reviewing and most likely emerging this this week. It will go into 6.0.0

@jzabroski jzabroski added this to the 6.0.0 milestone Apr 4, 2024
@jzabroski
Copy link
Collaborator

Should merge this tonight. 6.0.0 is getting closer. I am mainly trying to remove build warnings before calling 6.0.0 done.

@jzabroski jzabroski merged commit 0eb2b62 into fluentmigrator:main May 19, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants