Add support for backend compatibility #483

austindrenski · 2018-06-23T19:37:13Z

As a general note, just to help me review, in the future try to do unrelated documentation/refactoring/cleanup in separate PRs... I know as you're working on your PR you see issues and it's hard to resist, but it's really much easier to review a minimal, tight PR without any unrelated changes. You can always submit another PR afterwards that only does docs/cosmetics, it's very easy to review that.

roji · 2018-07-04T21:44:53Z

src/EFCore.PG/Infrastructure/Backend.cs

+        /// <summary>
+        /// The backend process is PostgreSQL.
+        /// </summary>
+        PostgreSQL = 0,


Nit/curiosity: any reason to assign the 0 here? And not to assign other values?

No real reason right now, I can omit that in the next commit.

It was there initially for some earlier testing where the backend type was marked with [Flags]. But it made the condition testing look a little complicated, so I rolled it back to a simple enum for the first iteration.

roji · 2018-07-04T21:45:35Z

src/EFCore.PG/Infrastructure/Backend.cs

+    /// Describes the backend process that uses the PostgreSQL wire protocol.
+    /// </summary>
+    [PublicAPI]
+    public enum Backend


I think calling this BackendType is more clear.

roji · 2018-07-04T21:45:51Z

src/EFCore.PG/Infrastructure/Internal/INpgsqlOptions.cs

-        ///     Reflects the option set by <see cref="NpgsqlDbContextOptionsBuilder.ReverseNullOrdering" />.
+        /// The backend process to target.
+        /// </summary>
+        Backend Backend { get; }


As above, BackendType seems better.

roji · 2018-07-04T22:12:09Z

src/EFCore.PG/Infrastructure/Internal/NpgsqlOptionsExtension.cs

+        /// The backend version to target.
+        /// </summary>
+        [NotNull]
+        public Version Compatibility { get; private set; }


How about just calling this BackendVersion? Compatibility is pretty vague and doesn't actually describe the value...

roji · 2018-07-04T22:17:04Z

src/EFCore.PG/Query/ExpressionTranslators/Internal/NpgsqlCompositeMethodCallTranslator.cs

+        /// <summary>
+        /// The default method call translators registered by the Npgsql provider.
+        /// </summary>
+        [NotNull] [ItemNotNull] static readonly IMethodCallTranslator[] MethodCallTranslators =


One day C# 8 will come out with non-nullable references and this attribute clutter will all go away... :)

Note that R#/Rider aren't amazing at enforcing these (especially the item annotations) which is one reason I don't put them very much. There's also a R# extension that treats references as non-nullable by default (unless you specify [CanBeNull]), which at least gets rid of all the [NotNull].

Very much looking forward to that day. Clarifying nullability on what are effectively runtime constants is......tiring. I like the sounds of that R# extension, I'll give it a look.

roji · 2018-07-04T22:18:22Z

src/EFCore.PG/Query/ExpressionTranslators/Internal/NpgsqlDateAddTranslator.cs

+        /// <summary>
+        /// The minimum backend version supported by this translator.
+        /// </summary>
+        [NotNull] static readonly Version MinimumSupportedVersion = new Version("9.4");


Nit: initialize with ints instead of a string...

roji · 2018-07-04T22:20:33Z

src/EFCore.PG/Query/ExpressionTranslators/Internal/NpgsqlDateAddTranslator.cs

        {
-            if (!_methodInfoDatePartMapping.TryGetValue(methodCallExpression.Method, out var datePart))
+            // This translation is only supported for PostgreSQL 9.4 or higher.
+            if (_backend != Backend.PostgreSQL || _compatibility < MinimumSupportedVersion)


I think this requires more thought. It's true that this won't be supported on Redshift, but that's simply because it forked from PostgreSQL 8.0.x. On the other hand CrateDB and CockroachDB may actually support it.

I think that in as a general rule looking at the backend version should be enough for most cases, and looking at its type is unnecessary/confusing.

I'll remove the backend type check for now. But this raises the question of which version do we mean for BackendVersion to represent?

Should it be the actual backend's version (e.g. type: Redshift, version: Redshift-X.X) or the equivalent PostgreSQL version (e.g. type: Redshift, version: PostgreSQL-9.4)?

That's a good question, I can see arguments both ways... I'm not sure we have enough actual information on this.

Redshift forked from PostgreSQL around version 8.0.2, and has not progressed in terms of "PostgreSQL versions" - AFAIK there are no post-8.0.2 PostgreSQL language features that have been ported over to Redshift. But in any case running EF Core over Redshift may very well be much too much effort, and not necessarily a great fit. I wouldn't worry about it too much.

For the others (CockroachDB, CrateDB) I simply don't know... In the absence of information it may be safer to simply have a PostgresVersion (rather than BackendVersion, less ambiguous), and possibly have the optional BackendType determine a PostgresVersion... So that if BackendType is set to be Redshift, we'd automatically set PostgresVersion to be 8.0.2 (in addition to making some other, truly Redshift-specific behavior modifications).

But this is really largely theoretical at this point. It may make sense to remove BackendType altogether, and keep only PostgresVersion at this point, until we have an actual use case. What do you think?

roji · 2018-07-04T22:21:54Z

src/EFCore.PG/Query/ExpressionTranslators/Internal/NpgsqlDateAddTranslator.cs

            // ugly, so we just generate the expression ourselves with CustomBinaryExpression

-            var amountToAdd = methodCallExpression.Arguments.First();
+            if (!(methodCallExpression.Object is Expression instance))


Why not combine these in a single if?

roji · 2018-07-04T22:23:10Z

src/EFCore.PG/Query/ExpressionVisitors/NpgsqlSqlTranslatingExpressionVisitor.cs

+        /// <summary>
+        /// The backend process to target.
+        /// </summary>
+        readonly Backend _backend;


Where do you see this class needing to do compatibility checks, do you have a concrete compatibility issue in mind? If not I'd rather leave it out until we do...

I had something like npgsql/npgsql#2023 in mind, but looking at it again, that doesn't really apply here. I'll roll these back until there's a reason to use them.

austindrenski · 2018-07-14T21:12:55Z

@roji Thanks for the review! I've pushed a commit to address your comments.

As a general note, just to help me review, in the future try to do unrelated documentation/refactoring/cleanup in separate PRs... I know as you're working on your PR you see issues and it's hard to resist, but it's really much easier to review a minimal, tight PR without any unrelated changes.

That's my bad... It's hard to recognize mission creep in the moment. I rolled back some of the changes, and split others off into a separate PR, which brings the affected file count from 13 to 8. I'll try to keep an eye on the peripheral changes going forward.

roji · 2018-07-15T11:43:47Z

src/EFCore.PG/Query/ExpressionTranslators/Internal/NpgsqlDateAddTranslator.cs

+        /// </summary>
+        /// <param name="backendType">The backend process to target.</param>
+        /// <param name="compatibility">The backend version to target.</param>
+        public NpgsqlDateAddTranslator(BackendType backendType, [NotNull] Version compatibility)


As above, rename to backendVersion.

roji · 2018-07-15T11:44:20Z

src/EFCore.PG/Query/ExpressionTranslators/Internal/NpgsqlDateAddTranslator.cs

-        /// <returns>
-        ///     A SQL expression representing the translated MethodCallExpression.
-        /// </returns>
+        [NotNull] readonly Version _compatibility;


Still need to rename _compatibility here to _backendVersion.

roji · 2018-07-15T11:45:31Z

src/EFCore.PG/Query/ExpressionTranslators/Internal/NpgsqlDateAddTranslator.cs

-        ///     Translates the given method call expression.
+        /// The backend process to target.
+        /// </summary>
+        readonly BackendType _backendType;


As this isn't being used, it can be removed, right?

roji · 2018-07-15T11:50:28Z

src/EFCore.PG/Query/ExpressionTranslators/Internal/NpgsqlDateAddTranslator.cs

        {
-            if (!_methodInfoDatePartMapping.TryGetValue(methodCallExpression.Method, out var datePart))
+            // This translation is only supported for PostgreSQL 9.4 or higher.
+            if (_backend != Backend.PostgreSQL || _compatibility < MinimumSupportedVersion)


That's a good question, I can see arguments both ways... I'm not sure we have enough actual information on this.

Redshift forked from PostgreSQL around version 8.0.2, and has not progressed in terms of "PostgreSQL versions" - AFAIK there are no post-8.0.2 PostgreSQL language features that have been ported over to Redshift. But in any case running EF Core over Redshift may very well be much too much effort, and not necessarily a great fit. I wouldn't worry about it too much.

For the others (CockroachDB, CrateDB) I simply don't know... In the absence of information it may be safer to simply have a PostgresVersion (rather than BackendVersion, less ambiguous), and possibly have the optional BackendType determine a PostgresVersion... So that if BackendType is set to be Redshift, we'd automatically set PostgresVersion to be 8.0.2 (in addition to making some other, truly Redshift-specific behavior modifications).

But this is really largely theoretical at this point. It may make sense to remove BackendType altogether, and keep only PostgresVersion at this point, until we have an actual use case. What do you think?

roji

Take a look at my new comments, I'd consider getting rid of BackendType altogether until we know exactly what we want/need, and simply have a PostgresVersion.

austindrenski · 2018-07-15T16:27:01Z

@roji wrote:

I think this requires more thought. It's true that this won't be supported on Redshift, but that's simply because it forked from PostgreSQL 8.0.x. On the other hand CrateDB and CockroachDB may actually support it.

I think that in as a general rule looking at the backend version should be enough for most cases, and looking at its type is unnecessary/confusing.

@austindrenski wrote:

I'll remove the backend type check for now. But this raises the question of which version do we mean for BackendVersion to represent?

Should it be the actual backend's version (e.g. type: Redshift, version: Redshift-X.X) or the equivalent PostgreSQL version (e.g. type: Redshift, version: PostgreSQL-9.4)?

@roji wrote:

That's a good question, I can see arguments both ways... I'm not sure we have enough actual information on this.

Redshift forked from PostgreSQL around version 8.0.2, and has not progressed in terms of "PostgreSQL versions" - AFAIK there are no post-8.0.2 PostgreSQL language features that have been ported over to Redshift. But in any case running EF Core over Redshift may very well be much too much effort, and not necessarily a great fit. I wouldn't worry about it too much.

For the others (CockroachDB, CrateDB) I simply don't know... In the absence of information it may be safer to simply have a PostgresVersion (rather than BackendVersion, less ambiguous), and possibly have the optional BackendType determine a PostgresVersion... So that if BackendType is set to be Redshift, we'd automatically set PostgresVersion to be 8.0.2 (in addition to making some other, truly Redshift-specific behavior modifications).

But this is really largely theoretical at this point. It may make sense to remove BackendType altogether, and keep only PostgresVersion at this point, until we have an actual use case. What do you think?

I think that's the best way forward. BackendType can always be reintroduced if/when we have more clarity.

I've removed BackendType in the latest commit, and renamed BackendVersion to PostgresVersion.

roji

Almost there, I think... Check out my comments.

roji · 2018-07-16T06:33:33Z

src/EFCore.PG/Infrastructure/Internal/NpgsqlOptionsExtension.cs

+        public NpgsqlOptionsExtension()
+        {
+            AdminDatabase = "postgres";
+            PostgresVersion = new Version(10, 4);


I'm not sure it's a good idea to hardcode a default version like this:

It makes it impossible to distinguish between user-provided values and defaults - a distinction that may be important. With this change, if we ask the question "what is the current database compatibility version", the answer will be "10.4" even if the user hasn't specified anything.

This value won't evolve unless we make it evolve with each and every PostgreSQL release.

Finally, I'd rather have the default admin database (postgres) specified in NpgsqlRelationalConnection

It seems safer and cleaner to just leave the options null if the user hasn't said anything. For PostgresVersion, a null would mean "assume the latest supported PostgreSQL version", i.e. don't turn off any features for backwards compat.

On the other hand, for ReverseNullOrdering I think your change is good - a nullable bool doesn't really help anyone and just makes things more complex.

That makes sense. But, I'm going to punt on making any changes to the admin database stuff (since it was here before this PR)... I can take a look at changing it in a follow up PR.

Actually, scratch that. I thought that was a default from before I started this PR, but the diff looks like I've added it.... Dropping the hard-coded admin default.

roji · 2018-07-16T06:40:56Z

src/EFCore.PG/Query/ExpressionTranslators/Internal/NpgsqlDateAddTranslator.cs

            // ugly, so we just generate the expression ourselves with CustomBinaryExpression

-            var amountToAdd = methodCallExpression.Arguments.First();
+            if (!(methodCallExpression.Object is Expression instance) ||


I like the new pattern matching syntax, but I'm not sure I like it as much as you :)

Unless I'm mistaken, if we're here, we already know that methodCallExpression.Object is non-null (thanks to the lookup above), that its type is Expression, and that there's one argument. So the conditions here are completely redundant, and seem to be used only to define new local variables, no?

If that's the case, I'd rather just define local variables. This makes the code easier to reason about, and doesn't make us doubt that maybe there's a chance of a null being there, etc...

I'm not sure I like this form of programming: the type of methodCallExpression.Object is Expression, and unless

I am a bit enamoured with the pattern matching...but you're right that this one is a bit over the top.

However, I think we still want a (simple) null check on both the instance and the argument. While DateTime and DateTimeOffset are structs, it's always possible that an earlier translator/visitor may have have forwarded a null reference.

roji · 2018-07-22T11:57:23Z

src/EFCore.PG/Query/ExpressionTranslators/Internal/NpgsqlCompositeMethodCallTranslator.cs

+            AddTranslators(instanceTranslators);

            foreach (var plugin in npgsqlOptions.Plugins)
+            {


I actually prefer leaving out curlies for simple one-liners, but I'm not die-hard on that.

I removed this in the rebase. (I've omitted them elsewhere to match the style...not sure why I added this one.)

roji · 2018-07-22T11:57:59Z

Looks good, but I think the unrelated changes in this PR were submitted as other PRs, right? In which case they need to be removed from this one before merging...

- Defaults compatibility version to 10.4. - Cleans up related files. - Adds (basic) documentation. - Adds nullability attributing where inconsistent.

- Also adds documentation throughout.

- Fixes issue 480.

- Defaults to PostgreSQL.

- Backend -> BackendType - Compatibiltiy -> BackendVersion - Rollback some peripheral changes - Add license header when missing in modified files

It may be useful, but until we have more insight into how PostgreSQL-like backends conform/differ, its safer to just focus on the equivalent PostgreSQL version identifier. Also renamed `BackendVersion` to `PostgresVersion`.

This will help differentiate user-defined versions from defaults. Also allows other options to be null by default.

roji · 2018-07-22T17:27:59Z

Thanks for the work!

Just a small note: you squashed all the commits in this PR (which is a good thing), but left all the joined commit messages in the final squashed commit, leaving a huge message with lots of irrelevant messages. It's no big deal - I rewrote the history with git rebase -i to clean it up, but you'll likely have to rewind your branch (git reset --hard to before this PR was merged) and pull again. If you have any issues let me know.

In general I try to avoid rewriting history because of the mess it creates but sometimes it makes sense.

austindrenski added the enhancement label Jun 23, 2018

austindrenski self-assigned this Jun 23, 2018

austindrenski requested a review from roji June 23, 2018 19:37

austindrenski force-pushed the compatibility branch from acf70b8 to 590ec36 Compare June 23, 2018 19:51

austindrenski added this to the 2.2.0 milestone Jun 24, 2018

austindrenski mentioned this pull request Jun 28, 2018

NullReferenceException in NpgsqlCompositeMethodCallTranslator.cs @ line 66 #494

Closed

roji requested changes Jul 4, 2018

View reviewed changes

austindrenski force-pushed the compatibility branch 3 times, most recently from d401c30 to 0e6ac66 Compare July 14, 2018 20:27

austindrenski changed the title ~~Add support for compatibility versioning~~ Add support for backend type and version compatibility Jul 14, 2018

austindrenski changed the title ~~Add support for backend type and version compatibility~~ Add support for backend compatibility Jul 14, 2018

austindrenski force-pushed the compatibility branch from 0e6ac66 to 0a418f8 Compare July 14, 2018 20:58

austindrenski mentioned this pull request Jul 14, 2018

Add some doc comments, align styles #520

Merged

austindrenski force-pushed the compatibility branch from 0a418f8 to 663f384 Compare July 14, 2018 21:17

roji reviewed Jul 15, 2018

View reviewed changes

roji requested changes Jul 15, 2018

View reviewed changes

austindrenski force-pushed the compatibility branch from 1e88fb6 to ca4ebba Compare July 15, 2018 16:08

austindrenski force-pushed the compatibility branch 4 times, most recently from 0e02130 to 416ba8b Compare July 15, 2018 20:59

roji requested changes Jul 16, 2018

View reviewed changes

austindrenski force-pushed the compatibility branch 2 times, most recently from 0f605b3 to e297515 Compare July 21, 2018 20:54

This was referenced Jul 21, 2018

Add doc comments and refactor composite translators #529

Merged

Add doc comments and refactor options classes #530

Merged

austindrenski force-pushed the compatibility branch from e297515 to 98adeed Compare July 22, 2018 02:33

roji reviewed Jul 22, 2018

View reviewed changes

austindrenski added 7 commits July 22, 2018 08:48

Add compatibility versioning

01ad083

- Defaults compatibility version to 10.4. - Cleans up related files. - Adds (basic) documentation. - Adds nullability attributing where inconsistent.

Add compatibiltiy into generators and visitors

0200b8e

- Also adds documentation throughout.

Add compatibility check for DateTime.Add(...)

57ec0be

- Fixes issue 480.

Add compatibility tests

a82cb45

Add backend awareness for compatibility

483c751

- Defaults to PostgreSQL.

Change names for backend compatibity types/methods

2db1694

- Backend -> BackendType - Compatibiltiy -> BackendVersion - Rollback some peripheral changes - Add license header when missing in modified files

Remove BackendType until needed explicitly

2dccf66

It may be useful, but until we have more insight into how PostgreSQL-like backends conform/differ, its safer to just focus on the equivalent PostgreSQL version identifier. Also renamed `BackendVersion` to `PostgresVersion`.

austindrenski force-pushed the compatibility branch from 98adeed to b6b4638 Compare July 22, 2018 12:59

Set PostgresVersion to null by default

bf756ae

This will help differentiate user-defined versions from defaults. Also allows other options to be null by default.

austindrenski force-pushed the compatibility branch from b6b4638 to bf756ae Compare July 22, 2018 13:10

austindrenski merged commit 04ce9cd into npgsql:dev Jul 22, 2018

austindrenski deleted the compatibility branch July 22, 2018 13:27

roji pushed a commit that referenced this pull request Jul 22, 2018

Add support for backend compatibility (#483)

df8af3c

This was referenced Jul 22, 2018

Amazon Redshift support #260

Closed

Npgsql.EntityFrameworkCore.PostgreSQL generates unsupported sql for old postgresql server #478

Closed

Support for compatibility versioning #480

Open

roji mentioned this pull request Aug 19, 2018

Fix migration bug with PostgreSQL 9.5 #607

Merged

austindrenski added a commit to austindrenski/npgsql-efcore.pg that referenced this pull request Aug 19, 2018

Add support for backend compatibility (npgsql#483)

bd5d500

austindrenski added a commit that referenced this pull request Aug 19, 2018

Add support for backend compatibility (#483) (#611)

2f4c05f

Add support for backend compatibility #483

Add support for backend compatibility #483

Uh oh!

Conversation

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

Uh oh!

Related:

Uh oh!

roji left a comment

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!

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!

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!

roji Jul 15, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

austindrenski commented Jul 14, 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!

roji Jul 15, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

roji left a comment

Choose a reason for hiding this comment

Uh oh!

austindrenski commented Jul 15, 2018

Uh oh!

roji left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

roji commented Jul 22, 2018

Uh oh!

roji commented Jul 22, 2018

austindrenski commented Jun 23, 2018 •

edited

Loading

roji Jul 15, 2018 •

edited

Loading

roji Jul 15, 2018 •

edited

Loading

austindrenski Jul 17, 2018 •

edited

Loading