Fix migration bug with PostgreSQL 9.5 #607

slavanap · 2018-08-18T13:09:49Z

Override MigrationsSqlGenerator.Generate(CreateSequenceOperation,...) function because of its incompatibility with PostgreSQL servers version below 10.

austindrenski · 2018-08-18T18:50:08Z

@slavanap Thanks for opening this PR.

We've been working on various version-compatibility features for the next release, so it's nice to see that users are thinking about version-compatibility too.

Could you give us some additional details describing what you're seeing now (including version info for PostgreSQL/Npgsql/EF Core), and what you expect to see after this PR?

The tests failed because they expect "AS type" part in CREATE SEQUENCE statement. I'm not sure there is some variable that specifies PostgreSQL backend version, so I could split each of related tests into 2 for my change. Moreover, it seems tests are run only for PostgreSQL version 10 at Appveyor & Travis.

roji · 2018-08-19T10:12:46Z

This was introduced in #301.

The problem here is that this "fix" also reverts the support for the new, non-bigint sequences, and I don't think we want that.

In my opinion, the current behavior should remain the default - the provider assumes the latest version of PostgreSQL is used, and supports different sequence data types with AS. However, we should give the user a compat option, in which case the provider reverts to the previous behavior (always bigint for all sequences, no AS when creating).

The problem is that the backwards compat feature, #483, wasn't backported to 2.1. @austindrenski, what do you think? Is it feasible (and risk-free) to backport this to 2.1 and add this conditional behavior?

slavanap · 2018-08-19T10:17:32Z

@roji, I agree on some points. I suggest to treat at least sequences of int and long as sequences with no AS casting, not just only long sequences, because many tutorials (and even ASP user Identity classes) use int Id { get; set; } in their entities.

roji · 2018-08-19T10:21:19Z

I suggest to treat at least sequences of int and long as sequences with no AS casting, not just only long sequences, because many tutorials (and even ASP user Identity classes) uses int Id { get; set; } in their entities.

I understand, but if you handle int sequences as bigint, you're effectively denying PostgreSQL 10 users the option of defining integer sequences in the database... I admit I'm not sure about this - it's true that lots of people out there are using int sequences in C#, and their code doesn't work if they try to run on PostgreSQL 9. But I'm thinking that a compatibility option as suggested above, plus a clear note in the 2.1 "breaking changes" docs should be enough...

slavanap · 2018-08-19T10:29:21Z

the provider assumes the latest version of PostgreSQL is used

May it request server version instead of the assumption? If similar changes would be made in PostgreSQL 11, similar issues will appear. AFAIK, it's still PostgreSQL 9 installed by default on Ubuntu Xenial and Debian Stretch. Not sure about Ubuntu Bionic.

roji · 2018-08-19T10:43:05Z

May it request server version instead of the assumption?

Unfortunately not, the service responsible for generating migration SQL (MigrationsSqlGenerator) is a singleton, which means it's used by all contexts, and does not have access to any specific database. But it is possible to use user-configured options to modify its behavior. This is just how EF Core is architectured. If this seems odd, consider that there are situations where we need to generate SQL but are not connected to any specific database (e.g. dotnet ef migrations script).

I do understand the pain of having the latest EF Core throw on the default PostgreSQL with standard code. Let me think about this for a bit (and let's see what @austindrenski thinks as well).

austindrenski · 2018-08-19T15:17:38Z

@roji wrote:

The problem here is that this "fix" also reverts the support for the new, non-bigint sequences, and I don't think we want that.

Agreed.

@roji wrote:

The problem is that the backwards compat feature, #483, wasn't backported to 2.1. @austindrenski, what do you think? Is it feasible (and risk-free) to backport this to 2.1 and add this conditional behavior?

I think #483 is sufficiently safe to backport (specifically, 04ce9cd). I'll also put together a PR to inject it into MigrationsSqlGenerator.

@roji wrote:

But I'm thinking that a compatibility option as suggested above, plus a clear note in the 2.1 "breaking changes" docs should be enough...

Agreed.

@slavanap

May it request server version instead of the assumption? If similar changes would be made in PostgreSQL 11, similar issues will appear. AFAIK, it's still PostgreSQL 9 installed by default on Ubuntu Xenial and Debian Stretch. Not sure about Ubuntu Bionic.

We're certainly sympathetic to defaults in the wild... but EF Core was designed to be opinionated and forward-leaning.

This is a great discussion on compatibility, and I'm happy that we're having it. But I do think that we can address your concerns without impacting support for features from the latest release of PostgreSQL.

austindrenski · 2018-08-19T16:35:58Z

I've opened #611 to backport the compatibility feature, and #612 to add it to MigrationsSqlGenerator.

roji · 2018-08-19T18:17:39Z

@slavanap are you interested in continuing work on this PR, once we merge the backwards compatibility support?

slavanap · 2018-08-19T19:44:05Z

I'm glad you guys paid attention on the issue already.

I've read comments in #612 and rewrote mine PR on top of it. Please, review if there's any additional changes necessary. Unfortunately, I couldn't figure out fast how to pass NpgsqlOptionsExtension.PostgresVersion in tests, just tested locally that it fixed my original issue with the option set

services.AddEntityFrameworkNpgsql().AddDbContext<ApplicationDbContext>(
    opt => opt.UseNpgsql(Configuration.GetConnectionString("Main"), o => {
        o.UseNetTopologySuite();
        o.SetPostgresVersion(new System.Version(9, 6));
    }));

// ...

using (var scope = app.ApplicationServices.GetService<IServiceScopeFactory>().CreateScope()) {
    scope.ServiceProvider.GetRequiredService<ApplicationDbContext>().Database.Migrate();
}

@roji, Do you mean merge these changes on top of hotfix/2.1.2 branch?

austindrenski · 2018-08-19T20:41:53Z

src/EFCore.PG/Migrations/NpgsqlMigrationsSqlGenerator.cs

+            // "CREATE SEQUENCE name AS type" expression is supported only in PostgreSQL 10 or above
+            // There is slightly modified version of MigrationsSqlGenerator.Generate below. The reason is here:
+            // https://github.com/aspnet/EntityFrameworkCore/blob/master/src/EFCore.Relational/Migrations/MigrationsSqlGenerator.cs#L533-L535
+            Check.NotNull(operation, nameof(operation));


These guards should be moved to the top of the method.

austindrenski · 2018-08-19T20:44:21Z

src/EFCore.PG/Migrations/NpgsqlMigrationsSqlGenerator.cs

+
+            var typeMapping = Dependencies.TypeMappingSource.GetMapping(operation.ClrType);
+            if (operation.ClrType != typeof(long))
+            {


nit: omit the braces on a one-liner if

austindrenski · 2018-08-19T20:47:52Z

src/EFCore.PG/Migrations/NpgsqlMigrationsSqlGenerator.cs

+            if (operation.ClrType != typeof(long))
+            {
+                // set the typeMapping for use with operation.StartValue (i.e. a long) below
+                typeMapping = Dependencies.TypeMappingSource.GetMapping(typeof(long));


_typeMappingSource is available as an instance field. Use that instead.

austindrenski · 2018-08-19T21:53:08Z

I've merged #612. Please rebase when you address the review.

austindrenski · 2018-08-19T21:54:52Z

src/EFCore.PG/Migrations/NpgsqlMigrationsSqlGenerator.cs

+
+            builder
+                .Append("CREATE SEQUENCE ")
+                .Append(Dependencies.SqlGenerationHelper.DelimitIdentifier(operation.Name, operation.Schema));


This should also use the instance field for _sqlGenerationHelper.

austindrenski · 2018-08-19T21:55:53Z

src/EFCore.PG/Migrations/NpgsqlMigrationsSqlGenerator.cs

+                .Append("CREATE SEQUENCE ")
+                .Append(Dependencies.SqlGenerationHelper.DelimitIdentifier(operation.Name, operation.Schema));
+
+            var typeMapping = Dependencies.TypeMappingSource.GetMapping(operation.ClrType);


This should also use the instance field for _typeMappingSource.

austindrenski · 2018-08-19T21:56:16Z

src/EFCore.PG/Migrations/NpgsqlMigrationsSqlGenerator.cs

+
+            builder
+                .Append(" START WITH ")
+                .Append(typeMapping.GenerateSqlLiteral(operation.StartValue));


austindrenski · 2018-08-19T21:56:30Z

src/EFCore.PG/Migrations/NpgsqlMigrationsSqlGenerator.cs

+                .Append(typeMapping.GenerateSqlLiteral(operation.StartValue));
+
+            SequenceOptions(operation, model, builder);
+            builder.AppendLine(Dependencies.SqlGenerationHelper.StatementTerminator);


slavanap · 2018-08-20T00:33:51Z

Applied requested changes and simplified the code. Now I wonder why have tests started to flicker.

austindrenski

This is looking good. One last style comment, then we need some tests added to the functional test suite.

austindrenski · 2018-08-20T00:47:27Z

src/EFCore.PG/Migrations/NpgsqlMigrationsSqlGenerator.cs

            builder.EndCommand();
        }

+        protected override void Generate(CreateSequenceOperation operation, IModel model, MigrationCommandListBuilder builder) {


Please move this brace to the following line

austindrenski · 2018-08-20T00:53:51Z

@slavanap Just an intermittently flaky test. I re-built the PR and it passed.

Please take a look at my review, and let us know if you need some advice on how to go about adding functional tests.

slavanap · 2018-08-20T01:02:58Z

One difficulty in writing tests for me now is how to set NpgsqlOptionsExtension.PostgresVersion value in a test. If you could help me find a test that already changes any field of NpgsqlOptionsExtension, that would simplify the task.

I'll continue tomorrow.

austindrenski · 2018-08-20T01:37:46Z

For some examples, take a look at the tests I'm working on in #541:

https://github.com/npgsql/Npgsql.EntityFrameworkCore.PostgreSQL/blob/c186ebba1b3019e7432face12f4989de4c30a4f1/test/EFCore.PG.FunctionalTests/Query/ArrayQueryTest.cs#L418-L427

You'll likely need to refactor some of the code in the support regions like here:

https://github.com/npgsql/Npgsql.EntityFrameworkCore.PostgreSQL/blob/c186ebba1b3019e7432face12f4989de4c30a4f1/test/EFCore.PG.FunctionalTests/Query/ArrayQueryTest.cs#L1216-L1229

The tests in #541 are somewhat incomplete, as they only test that we perform the translation given a supported version number.

For this PR, you will need to test the translations on both sides of version 10.0.

slavanap · 2018-08-20T09:13:31Z

@austindrenski I've found tests that are related to the subject and modified them. Now I'm thinking how to apply "Fixture" pattern to them as well. Well, if that's not possible, should I remove those tests completely and replace them with tests in a new file? What do you think?

roji · 2018-08-20T10:43:26Z

In my opinion, these tests should ideally live in NpgsqlMigrationSqlGeneratorTest, although it might be tricky to change the context for the compatibility options... Let's give it a try though.

slavanap · 2018-08-20T10:53:16Z

@roji have you seen my modifications of NpgsqlMigrationSqlGeneratorTest in this PR?

austindrenski

Let's see what @roji thinks, but I think this is looking good.

In particular, I like your VersionScope class. It's a clever way of solving the "overriding tests is complicated"-problem by explicitly scoping your configuration changes.

I've left a few other nits addressing what look like some C++/C# style differences.

Overall, I think this is very close to mergeable.

austindrenski · 2018-08-20T23:53:32Z

test/EFCore.PG.FunctionalTests/TestUtilities/NpgsqlTestHelpers.cs

 namespace Npgsql.EntityFrameworkCore.PostgreSQL.TestUtilities
 {
-    public class NpgsqlTestHelpers : TestHelpers
+    public class NpgsqlTestHelpers: TestHelpers


nit: revert

austindrenski · 2018-08-20T23:53:34Z

test/EFCore.PG.FunctionalTests/TestUtilities/NpgsqlTestHelpers.cs

-    public class NpgsqlTestHelpers : TestHelpers
+    public class NpgsqlTestHelpers: TestHelpers
    {
+        private System.Version PostgresVersion;


nit: Import the System namespace at the top.
nit: Rename to _postgresVersion.
nit: Omit the (implicit) private modifier

austindrenski · 2018-08-20T23:54:52Z

test/EFCore.PG.FunctionalTests/TestUtilities/NpgsqlTestHelpers.cs

    {
+        private System.Version PostgresVersion;
+
+        public class VersionScope : System.IDisposable {


nit: As above, drop the qualifier.
nit: Bring this brace down to the next line.

austindrenski · 2018-08-20T23:54:54Z

test/EFCore.PG.FunctionalTests/TestUtilities/NpgsqlTestHelpers.cs

+        public class VersionScope : System.IDisposable {
+            private NpgsqlTestHelpers _helpers;
+            private System.Version _oldVersion, _newVersion;
+            public VersionScope(NpgsqlTestHelpers helpers, System.Version version) {


nit: Add a line break between the fields and constructor.
nit: Bring this brace down to the next line.

austindrenski · 2018-08-20T23:56:41Z

test/EFCore.PG.FunctionalTests/TestUtilities/NpgsqlTestHelpers.cs

+                _oldVersion = helpers.PostgresVersion;
+                _newVersion = helpers.PostgresVersion = version;
+            }
+            public void Dispose() {


nit: Bring this brace down to the next line.

austindrenski · 2018-08-20T23:57:34Z

test/EFCore.PG.FunctionalTests/TestUtilities/NpgsqlTestHelpers.cs

+            }
+        }
+
        protected NpgsqlTestHelpers()


I know this was already here...but would you mind bringing these braces up like so (note the spacing)?

protected NpgsqlTestHelpers() {}

austindrenski · 2018-08-20T23:58:29Z

test/EFCore.PG.FunctionalTests/TestUtilities/NpgsqlTestHelpers.cs

+            => optionsBuilder.UseNpgsql(new NpgsqlConnection("Database=DummyDatabase"),
+                   options => options.SetPostgresVersion(PostgresVersion));
+
+        public VersionScope WithPostgresVersion(System.Version version) => new VersionScope(this, version);


Please move this above the constructor.

austindrenski · 2018-08-20T23:58:50Z

test/EFCore.PG.FunctionalTests/TestUtilities/NpgsqlTestHelpers.cs

+                   options => options.SetPostgresVersion(PostgresVersion));
+
+        public VersionScope WithPostgresVersion(System.Version version) => new VersionScope(this, version);
+


Omit trailing line breaks.

slavanap · 2018-08-21T11:40:02Z

Also extended CreateSequenceOperation_with_data_type_smallint test (dc9f75e#diff-be2409a5108c69d00b643dff9fd0df53R1054)
and marked VersionScope constructor with internal.

I wish MS will implement code style settings per project one day.
I still have to be more careful with code style when writing PRs, because I too much used to mine.
Btw, should I rebase on head of dev branch or leave this PR like that with several commits?

austindrenski · 2018-08-22T16:45:33Z

@slavanap Could you rebase on npgsql/dev?

… function because of its incompatibility with PostgreSQL servers version below 10

slavanap · 2018-08-22T22:43:57Z

@austindrenski Done.

austindrenski · 2018-08-23T18:21:39Z

@slavanap Thanks for your contribution!

slavanap · 2018-08-23T20:52:32Z

@austindrenski Thanks for your review! It was valuable experience.

roji · 2018-08-24T04:32:46Z

src/EFCore.PG/Migrations/NpgsqlMigrationsSqlGenerator.cs

            builder.EndCommand();
        }

+        protected override void Generate(CreateSequenceOperation operation, IModel model, MigrationCommandListBuilder builder)


Wouldn't it be better to simply call base.Generate() with a CreateSequenceOperation that has ClrType overridden to long? This would prevent the duplication of that code in this function...

(@austindrenski it's better to wait for me to review too, I know I'm a bit busy these days but still...)

@roji Apologies on moving forward with the merge before your review. I'll open a follow-up PR to de-duplicate where possible.

@roji How would you suggest to clone CreateSequenceOperation operation class with all it's properties? Or you suggest modify its ClrType property "in-place" and return back to original value after calling base.Generate() method?

Changing in-place is a possibility, yes. I'm not even sure it's necessary to return back to the original value as these operations are only used for SQL generation - but for correctness's sake it's a good idea.

You could also copy properties one-by-one to a new instance, including annotations, but that would be tedious and brittle in case something gets added in the future.

This was referenced Aug 19, 2018

Add support for backend compatibility (#483) #611

Merged

Inject compatibility version for migrations #612

Merged

austindrenski suggested changes Aug 19, 2018

View reviewed changes

austindrenski suggested changes Aug 20, 2018

View reviewed changes

austindrenski suggested changes Aug 21, 2018

View reviewed changes

Override MigrationsSqlGenerator.Generate(CreateSequenceOperation,...)…

2cccfb4

… function because of its incompatibility with PostgreSQL servers version below 10

austindrenski approved these changes Aug 23, 2018

View reviewed changes

austindrenski merged commit fa719a6 into npgsql:dev Aug 23, 2018

roji reviewed Aug 24, 2018

View reviewed changes

slavanap mentioned this pull request Sep 16, 2018

Simplify NpgsqlMigrationsSqlGenerator.Generate for CreateSequenceOperation #639

Merged

roji assigned slavanap Sep 23, 2018

roji added this to the 2.2.0 milestone Sep 23, 2018

roji added the bug label Sep 23, 2018

austindrenski mentioned this pull request Oct 27, 2018

Sequence migrations fails after updating from 2.1.0 to 2.1.2 #681

Closed

		options => options.SetPostgresVersion(PostgresVersion));

		public VersionScope WithPostgresVersion(System.Version version) => new VersionScope(this, version);

Fix migration bug with PostgreSQL 9.5 #607

Fix migration bug with PostgreSQL 9.5 #607

Uh oh!

Conversation

slavanap commented Aug 18, 2018

Uh oh!

austindrenski commented Aug 18, 2018

Related

Uh oh!

slavanap commented Aug 18, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

slavanap commented Aug 18, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

roji commented Aug 19, 2018

Uh oh!

slavanap commented Aug 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

roji commented Aug 19, 2018

Uh oh!

slavanap commented Aug 19, 2018

Uh oh!

roji commented Aug 19, 2018

Uh oh!

austindrenski commented Aug 19, 2018

Uh oh!

austindrenski commented Aug 19, 2018

Uh oh!

roji commented Aug 19, 2018

Uh oh!

slavanap commented Aug 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

austindrenski commented Aug 19, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

slavanap commented Aug 20, 2018

Uh oh!

austindrenski left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

austindrenski commented Aug 20, 2018

Uh oh!

slavanap commented Aug 20, 2018

Uh oh!

austindrenski commented Aug 20, 2018

Uh oh!

slavanap commented Aug 20, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

roji commented Aug 20, 2018

Uh oh!

slavanap commented Aug 20, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

austindrenski left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

austindrenski Aug 20, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

slavanap commented Aug 18, 2018 •

edited

Loading

slavanap commented Aug 18, 2018 •

edited

Loading

slavanap commented Aug 19, 2018 •

edited

Loading

slavanap commented Aug 19, 2018 •

edited

Loading

slavanap commented Aug 20, 2018 •

edited

Loading

slavanap commented Aug 20, 2018 •

edited

Loading

austindrenski Aug 20, 2018 •

edited

Loading

slavanap commented Aug 21, 2018 •

edited

Loading