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

[neyromant]

Hi
I'm using PostgreSQL 9.3.5 on x86_64-unknown-linux-gnu, compiled by gcc (GCC) 4.4.7 20120313 (Red Hat 4.4.7-4), 64-bit "

After switching to the version of Npgsql.EntityFrameworkCore.PostgreSQL 2.1.0, I got errors on some queries. "Npgsql.PostgresException" "42703: column " mins \ "does not exist".

I did a little investigation:
For the following code

public class Foo
{
        [Key]
        public int Id { get; set; }
        public DateTime Date { get; set; }
}

public class ApplicationDbContext : DbContext
{
     public DbSet<Foo> Foos { get; set; }
     public ApplicationDbContext(DbContextOptions<ApplicationDbContext> options) : base(options) { 
     }
}

...

var res = db.Foos.Select(x => new
{
    Date = x.Date.AddMinutes(60)
}).ToList();
...

Npgsql.EntityFrameworkCore.PostgreSQL generates the following Sql:

SELECT (x."Date" + MAKE_INTERVAL(mins => 60)) AS "Date" FROM "Foos" AS x

But version 2.0.0 of Npgsql.EntityFrameworkCore.PostgreSQL generated the following sql:

SELECT "x"."Date" FROM "Foos" AS "x"

The problem is that MAKE_INTERVAL function is not supported by PostgreSQL 9.3.

Question. Is it a bug, or is Npgsql.EntityFrameworkCore.PostgreSQL 2.1.0 just not going to support the old PostgreSql server?

[roji]

Oh, that's unfortunate... This is actually a feature added to 2.1.0, where we translate certain DateTime arithmetic (#173) to SQL. That's a positive thing - it allows the operation to be evaluated in PostgreSQL, potentially saving you from downloading a large dataset just to perform the operation client-side.

One easy workaround is to force evaluating the DateTime arithmetic client side by calling ToEnumerable() before your projection:

var res = db.Foos.ToEnumerable().Select(x => new
{
    Date = x.Date.AddMinutes(60)
}).ToList();

This should be equivalent to how your code worked on 2.0.0.

It's also worth mentioning that PostgreSQL 9.3 will reach its end-of-life in two months, so I'm reluctant to add a backwards compatibility flag or similar.

I'll close this for now as I don't think there's anything to be done in Npgsql, but will be happy to continue the discussion if you think otherwise.

[austindrenski]

@neyromant This is an interesting issue, thanks for sharing it.

@roji I'm thinking about how this relates to the various operator translations being added.

For example, I'm imagining a situation where I'm unable to take advantage of new translation support for pre-existing Postgres features, because a release also introduces support for new Postgres features that are not supported by my version of the database.

A couple of questions:

1. Do we definitively document the PostgreSQL versions that the provider supports? If not, should we?

   • e.g. "2.1.0 supports PostgreSQL versions 9.4.0 through 10.0.0"

2. Do you know if other providers (or the EF Core team) are addressing compatibility issues surrounding version specific SQL generation?

3. Would it make sense to adopt something like ASP.NET Core's MVC compatibility versioning?

The SetCompatibilityVersion method allows an app to opt-in or opt-out of potentially breaking behavior changes introduced in ASP.NET MVC Core 2.1+. These potentially breaking behavior changes are generally in how the MVC subsystem behaves and how your code is called by the runtime. By opting in, you get the latest behavior, and the long-term behavior of ASP.NET Core.

The following code sets the compatibility mode to ASP.NET Core 2.1:

public void ConfigureServices(IServiceCollection services)
{
   services.AddMvc()
           .SetCompatibilityVersion(CompatibilityVersion.Version_2_1);
}

Perhaps something similar to these:

// Option 1:
public IServiceProvider ConfigureServices(IServiceCollection services)
    => services.AddEntityFrameworkNpgsql()
               .SetNpgsqlCompatibilityVersion(NpgsqlCompatibility.Version_2_1)
               .BuildServiceProvider();

// Option 2:
public IServiceProvider ConfigureServices(IServiceCollection services)
    => services.AddEntityFrameworkNpgsql(NpgsqlCompatibility.Version_2_1)
               .BuildServiceProvider();

[roji]

Do we definitively document the PostgreSQL versions that the provider supports? If not, should we?
e.g. "2.1.0 supports PostgreSQL versions 9.4.0 through 10.0.0"

We don't and can definitely improve on this. There's a note in the compatibility section of the docs saying we aim to be compatible with supported PostgreSQL versions (5 major versions back), but that's not enough.

In practice, though, I've actually received relatively little complaints about usage of new PostgreSQL feature which aren't available in older ones.

Do you know if other providers (or the EF Core team) are addressing compatibility issues surrounding version specific SQL generation?

EF Core made a conscious choice to not be aware of the specific database it's connected to when generating queries or migrations, unlike EF6 where it was possible to vary SQL based on backend version. While this may sound weird, it does have its justifications. You can look at dotnet/efcore#9000 for some discussion (there's an earlier discussion that's more complete but I can't find it at the moment).

As that thread says, the recommended way to support different versions is by having the user specify features explicitly, as DbContext configuration options. This shifts the responsibility on the user to tell the provider "generate SQL with this or that characteristic".

Would it make sense to adopt something like ASP.NET Core's MVC compatibility versioning?

I think that's more or less the same as the recommended approach mentioned above - explicit user opt-in or out of features which may not be supported in some backends. It's possible, but I'd wait a bit to see how many people are actually affected by this issue, especially since a workaround is very easy to implement (i.e. forcing client-side evaluation).

Let me know what your thoughts are about all this.

[austindrenski]

EF Core made a conscious choice to not be aware of the specific database it's connected to when generating queries or migrations, unlike EF6 where it was possible to vary SQL based on backend version. While this may sound weird, it does have its justifications.

Reading through the issues, I can see why this makes sense. As an EF Core user, I certainly appreciate the current approach.

It's possible, but I'd wait a bit to see how many people are actually affected by this issue, especially since a workaround is very easy to implement (i.e. forcing client-side evaluation).

I can understand the "wait and see" perspective. That said, this sounds like it could be a good investment if implemented in an unobtrusive way.

Ideally, we could quietly introduce this and only use it to fence off issues as they arise, while avoiding any big overhauls to compatibility-proof the existing code base.

From my (narrow) perspective, this seems like a positive in terms of overall robustness. But....I've only been thinking about this for a few hours, so my outlook on it may be over optimistic.

Examples

This is just for brainstorming purposes (i.e. treat this as pseudocode).

It seems like a field on NpgsqlOptions would be enough to get started. Then a compatibility flag could be passed into various translators that could then either use or ignore the flag.

Before:

// NpgsqlCompositeMemberTranslator.cs

/// <summary>
/// The default member translators registered by the Npgsql provider.
/// </summary>
static readonly IMemberTranslator[] MemberTranslators =
{
    new NpgsqlStringLengthTranslator(),
    new NpgsqlDateTimeMemberTranslator()
};

/// <inheritdoc />
public NpgsqlCompositeMemberTranslator(
    [NotNull] RelationalCompositeMemberTranslatorDependencies dependencies,
    [NotNull] INpgsqlOptions npgsqlOptions)
    : base(dependencies)
{
    // ReSharper disable once VirtualMemberCallInConstructor
    AddTranslators(MemberTranslators);

// NpgsqlDateTimeMemberTranslator.cs

public virtual Expression Translate(MemberExpression e)
{
    if (e.Expression == null)
        return TranslateStatic(e);

After:

// NpgsqlCompositeMemberTranslator.cs

/// <inheritdoc />
public NpgsqlCompositeMemberTranslator(
    [NotNull] RelationalCompositeMemberTranslatorDependencies dependencies,
    [NotNull] INpgsqlOptions npgsqlOptions)
    : base(dependencies)
{
    IMemberTranslator[] MemberTranslators =
    {
        new NpgsqlStringLengthTranslator(),
        new NpgsqlDateTimeMemberTranslator(npgsqlOptions.NpgsqlCompatibility)
    };

    // ReSharper disable once VirtualMemberCallInConstructor
    AddTranslators(MemberTranslators);

// NpgsqlDateTimeMemberTranslator.cs
    
readonly NpgsqlCompatibility _compatibility;

public NpgsqlDateTimeMemberTranslator(NpgsqlCompatibility _compatibility)
    => _compatibility = compatibility;

public virtual Expression Translate(MemberExpression e)
{
    if (_compatibility < NpgsqlCompatibility.PostgreSQL_9_4)
        return null;

    if (e.Expression == null)
        return TranslateStatic(e);

[roji]

It's certainly possible to introduce something like this, and shouldn't be very difficult. Instead of a custom NpgsqlCompatibility we could simply use .NET's Version, and if it's null we can take that to mean "latest version".

Note also that Npgsql is sometimes used with "pseudo-PostgreSQL" databases, which use the PostgreSQL wire protocol but have a non-PostgreSQL backend. This is the case for Amazon Redshift, CockroachDB, CrateDB and several others. Some of these also have their compatibility quirks and issues so it's worth considering them as well. I'd handle that separately via another NpgsqlOption.

I suggest opening a separate issue for tracking this. And if you'd like to submit a PR as well I'd be happy to review.

[austindrenski]

@neyromant We've merged #483 which addresses your original issue. Keep an eye out for it in v2.2.0.

[neyromant]

@austindrenski Thank you very match!