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

Add include for index in postgres #1362

Merged
merged 4 commits into from
Oct 27, 2020

Conversation

lillo42
Copy link
Contributor

@lillo42 lillo42 commented Oct 19, 2020

Add include for index in postgres
Add support for postgres 11

@jzabroski
Copy link
Collaborator

Thanks for your submission, @lillo42 .

For review, things I will look at:

  • How closely does this code resemble similar feature implemented for SQL Server generator. Ideally, code should be similar.
  • Postgres is a bit different from SQL Server in that it also supports a USING method where method is one of [btree, hash, gist, spgist, gin, and brin] (although, custom methods are allowed, nobody really does this except PhD lab workers). Should we support that additional feature as well? If not specified, assume Postgres default value btree. Ideally, the method would be a C# enum PostgresIndexMethod.
  • Should we handle the negative test case where someone is using a 9_2 or 10_0 generator with this code? Right now it will silently fail.

@lillo42
Copy link
Contributor Author

lillo42 commented Oct 19, 2020

@jzabroski

* How closely does this code resemble similar feature implemented for SQL Server generator.  Ideally, code should be similar.

I implement based on SQL Server 😄

* Postgres is a bit different from SQL Server in that it also supports a `USING method` where `method` is one of `[btree, hash, gist, spgist, gin, and brin]` (although, custom methods are allowed, nobody really does this except PhD lab workers). Should we support that additional feature as well?  If not specified, assume Postgres default value `btree`.  Ideally, the `method` would be a C# enum `PostgresIndexMethod`.

That is something I want to implement, also NULL {First | Last}, CONCURRENTLY and maybe ONLY/WITH/TABLESPACE/WHERE. 😄

* Should we handle the negative test case where someone is using a 9_2 or 10_0 generator with this code?  Right now it will silently fail.

What I should to do when user try to use include a column in Postgres 10 or lower? Throw a exception

@jzabroski
Copy link
Collaborator

Yes, I would throw an exception for older generators. Or at least logsay a warning (in this case, that may be more conservative, as it guarantees non semantic changes that are physical layout issues don't block migration unexpectedly. While ideally people know their target deployment, some people work in the wild west and need a reminder to get things under control).

As for SQL Server vs. Postgres, do you see potential for code reuse. I plan to look myself but have not had time yet.

Otherwise this looks good and I'm optimistic to include this soon as part of 3.2.10

@jzabroski
Copy link
Collaborator

Table space is a huge feature that should work across all processors as most databases except Sqlite support partitioning. There is a ticket in the backlog for this.

@jzabroski
Copy link
Collaborator

jzabroski commented Oct 21, 2020

Chances are remaining tasks here are to update FluentMigrator Documentation repo to document the new processor id, and a bonus would be to add doc explaining advanced index features.

@lillo42
Copy link
Contributor Author

lillo42 commented Oct 22, 2020

foreach feature (NULL Last/FIRST, using index algorithm and etc) should I open a new PR or do every thing in this one?

As for SQL Server vs. Postgres, do you see potential for code reuse. I plan to look myself but have not had time yet.

Yes, the code are very similiar.

@jzabroski
Copy link
Collaborator

foreach feature (NULL Last/FIRST, using index algorithm and etc) should I open a new PR or do every thing in this one?

Separate PRs are better. Easier to review and more self-contained. If, prior to opening the PR, you can create a small issue with related Postgres 11 documentation (or earlier if feature existed before then), that would be great as well. I get some people just want to go straight to the PR but it helps with google search indexing when we create issues.

@jzabroski jzabroski merged commit 10e3125 into fluentmigrator:master Oct 27, 2020
@jzabroski
Copy link
Collaborator

@lillo42 Thanks for the PR and the last addition!

@jzabroski jzabroski added this to the 3.2.10 milestone Oct 27, 2020
@jzabroski
Copy link
Collaborator

@lillo42 I should release 3.2.10 by this weekend, and that will likely be the last .NET Core 3.0 release leading up to .NET 5 RTM. I am trying to rush to get the 4.0 branch merged with 3.2.9+changes so that we can easily/smoothly take in .NET 5.

@jzabroski
Copy link
Collaborator

@lillo42 If interested I created the associated doc task in the documentation repo for FluentMigrator. Ideally it would combine SQL Server and Postgres into the same page. Something like an example with approximate generated SQL would be useful - anything to help land on a Google search for something like "fluentmigrator include column index"

@lillo42 lillo42 deleted the add-include-postgres branch October 28, 2020 13:46
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

2 participants