Feature - Use enum to pick index method

[avin-kavish]

It would be nice to have an overload of

.ForNpgsqlHasMethod("gin");

that accepts an enum value. Primarily for intellisensability. (I would do this, if approved)

[avin-kavish]

Something like this

    public static class EFCoreExtensions
    {
        public static IndexBuilder ForNpgSqlHasMethod(this IndexBuilder indexBuilder, NpgIndexMethod method)
        {
            return indexBuilder.ForNpgsqlHasMethod(method.Value);
        }
    }

    public struct NpgIndexMethod
    {
        private NpgIndexMethod(string value)
        {
            Value = value;
        }

        public string Value { get; set; }

        public static NpgIndexMethod Btree => new NpgIndexMethod("btree");
        public static NpgIndexMethod Gist => new NpgIndexMethod("gist");
        public static NpgIndexMethod Hash => new NpgIndexMethod("hash");
    }

** Names subject to change.

[roji]

There are already various other places where PostgreSQL lists of values are specified as strings - notably types (e.g. when users specify HasStoreType()). We can start wrapping all of these with enums and similar, but the value seems rather limited and we'd have to update these as PostgreSQL adds new ones...

In general it seems preferable for users to consult the PostgreSQL documentation to know these values - at the very least we should point them towards the right page from the xmldocs.

[roji]

See https://stackoverflow.com/questions/56207300/does-npgsql-expose-a-strongly-typed-list-of-postgresql-column-types/56207864#56207864 for a similar request for PostgreSQL types.

[avin-kavish]

First of all, I have to start by saying I appreciate the work you've done so far. This provider is better than most out there.

I did consider all that, this is not meant to be a replacement but an overload. Similar to how model builders accept both a property expression and string property name as a selector.

I also didn't intend for it as a discovery mechanism for index types. I meant it for type safety. The cost of a misspelled enum is a squiggly line and build failure, the cost of a misspelled string literal is a run-time error when the model lazy loads on the first GET request after the build, startup and service activation time. Requiring a minimum of another refactor-build-execute cycle to correct a silly spelling error.

I do understand the cost vs benefit is low. It's a cherry on top, not the cake. There's already so many features missing in EF Core and it's providers, I'm sure nobody would notice a delayed enum update if there has to be any. In the answer to the SO question, you've said that they are well established names that never change, so there will hardly be any updates leading to refactor. Maybe it's not that much effort after all? It certainly takes less time to code a solution for this than to have this debate.

There's no rule saying that you have to turn each and every string accepting method into an enumeration. A list of 6 index types that are unlikely to change sounds like the ideal candidate. In addition to providing a link to the documentation, the xmldocs can say that "this list is not exhaustive, use the string overload for unlisted types." as a way for you to limit your liability. With the postgres extension system it is technically impossible to build an exhaustive list of types and indexes anyway.

There is no need for 100% parity, it's not like 100% of pg features are exposed in this provider already. Even with the gazillion column types, I don't see the harm in having an enumeration, although, it would be hell of a lot easier if C# supported string enums. If you have a table in the docs indicating the type mapping, why can't you have an enum and have the enum generate the table in the docs? Saying that it's okay to use strings is not an argument against static types, it simply means that it's okay to use strings, which is fine and is not being brought into question. Static types are consistent, compatible with intellisense and provide build time errors as opposed to run-time errors. None of these are done by strings.

TLDR: Wrap index types as they are only few and require no maintenance

I have far more important questions for you,

1. Is the check constraint implemented?
2. Are partial indexes implemented?
3. Is OnUpdate on foreign keys implemented?
4. If I use .ForNpgsqlUseSequenceHiLo(), How would I specify a sequence start value?
5. If I use .UseNpgsqlIdentityColumn() How would I specify identity start, current, increment and min max values?
6. For both of the above and sequences, if I use HasData() to insert seed data with specific ids. Does it lead to a key collision on the next db/client generated insert because nextval('sequence') returns the same id as the inserted ones?
7. How do you ensure that the raw SQL you have to write to make the above work gets applied to a test database created using EnsureCreated() ? Do you Migrate() your test databases?
8. .ForNpgsqlUseXminAsConcurrencyToken() can this be specified globally on all entities?
9. If I use the above, who pulls the Xmin value, Npgsql or the provider ? If the provider pulls it, is it present in the shadow state?
10. If any of the above works, can we specify them in the docs
11. What is holding this project back from being feature complete? Is it the upstream project? is it lack of time/contributors?

I'm reading through your source now, I don't see why you can't add a PropertyAnnotation called "Check" that accepts a predicate as a lambda expression. And in the sql generator create an ALTER TABLE ADD CONSTRAINT .... CHECK .....

You could also add an IndexAnnotation called Partial, PartialCondition, Condition, or IndexCondition and in your SQLGenerator just above line 602 append a WHERE clause.

You could be thinking that migrations allow for all this with arbitrary sql execution but migrations are a state transition. Migrations are tied to a specific db. They update the db from one state to the next. They are not a definition. There should be a stateless single source of truth to the database model in order for the code to be readable, portable and maintainable and this has to come in the form of feature completeness from the provider.

[austindrenski]

Saying that it's okay to use strings is not an argument against static types, it simply means that it's okay to use strings, which is fine and is not being brought into question. Static types are consistent, compatible with intellisense and provide build time errors as opposed to run-time errors. None of these are done by strings.

Those are fair points, but I think this conversation turns on the cost/benefit you mentioned above. There's a lot of work to be done, and every feature costs time to develop and adds to the long term maintenance burden.

PRs from new contributors help with the first part, but there's still the question of whether it adds enough value to make sense as part of this project (as compared to being an extension method in your specific project).

I have far more important questions for you,

I can answer a few of these off-hand, but to keep this thread on topic, please open new issues for further discussion on them

1. Is the check constraint implemented?

Blocked upstream, tracking via #837.

2. Are partial indexes implemented?

#116

3. Is OnUpdate on foreign keys implemented?

This should already be implemented by the upstream project, no?

6. For both of the above and sequences, if I use HasData() to insert seed data with specific ids. Does it lead to a key collision on the next db/client generated insert because nextval('sequence') returns the same id as the inserted ones?

We recommend seeding data with negative key values. Otherwise you should add the requisite raw SQL to bring your sequence up to date. See #536, #573, #759.

7. How do you ensure that the raw SQL you have to write to make the above work gets applied to a test database created using EnsureCreated() ? Do you Migrate() your test databases?

EnsureCreated() doesn't play well with the migration flow. But if you just need to create/drop simple test stores, then Ensure{Created,Deleted} tend to work well.

10. If any of the above works, can we specify them in the docs

Our docs are a always a work in progress that could certainly use more attention. PRs are always welcome :)

11. What is holding this project back from being feature complete? Is it the upstream project? is it lack of time/contributors?

Feature completeness is always going to be a high bar when considering a backend as feature-rich as PostgreSQL. More contributors would certainly help, and new contributors are always welcome!

[roji]

@austindrenski thanks for the good responses above, here's some more from me.

First of all, I have to start by saying I appreciate the work you've done so far. This provider is better than most out there.

Thanks for saying that! That's also good to hear.

On the issue of index method enums, it's worth saying I'm not totally against the idea. However:

• I do think a good API has to be consistent, and it's not ideal to have an enum for index methods but not for other settings - which is why I approach this question from a more global standpoint. Adding just an index method enum would leave users somewhat confused IMO.
• The actually value seems rather low. Yes, it's always better to catch typos in compile-time rather than run-time, but since index methods are part of migrations a typo would be discovered very quickly (and in a risk-free way, as migrations run in transactions). It isn't some obscure runtime branch that would fail at some random late point.
• Index methods are a very rarely-used and advanced feature, so once again people can be expected to discover the methods themselves and ensure they have no typo.

On the specific questions, to supplement @austindrenski's responses:

Is the check constraint implemented?

Blocked upstream, tracking via #837.

This has actually been unblocked at the EF Core side, I'm not actually sure we need to do anything to make it work in Npgsql (but obviously testing is needed).

If I use .ForNpgsqlUseSequenceHiLo(), How would I specify a sequence start value?

The HiLo feature seems to be very rarely used (not just with PostgreSQL, but in general in EF Core), and this is the first time anyone has raised the lack of support for specifying the starting value. See my comment at the bottom about eagerly implementing features without a clear need, but if you'd like you can open an issue for this.

If I use .UseNpgsqlIdentityColumn() How would I specify identity start, current, increment and min max values?

This is tracked by #819 and will hopefully make it into 3.0.0.

How do you ensure that the raw SQL you have to write to make the above work gets applied to a test database created using EnsureCreated() ? Do you Migrate() your test databases?

I'm not sure I understand the question - is this a general EF Core question on how EnsureCreated() interacts with migrations? It doesn't seem to be specific to the Npgsql provider.

If any of the above works, can we specify them in the docs

Our docs are a always a work in progress that could certainly use more attention. PRs are always welcome :)

Very much agree - our docs could definitely use some work. It's not always easy to keep them in sync with actual features, and it's possibly a good place for contributors to start as they're exploring/discovering the provider.

What is holding this project back from being feature complete? Is it the upstream project? is it lack of time/contributors?

Feature completeness is always going to be a high bar when considering a backend as feature-rich as PostgreSQL. More contributors would certainly help, and new contributors are always welcome!

Agree as well - the range of features is potentially extremely large, and you can see the same issue in Microsoft-developed providers as well (e.g. SQL Server / Sqlite). In some cases we are also limited by upstream EF Core, but in general this is an open source project maintained by the community.

As a general rule, I personally try to avoid rushing into feature implementation without users asking for them, or without a plausible reason to believe it would be useful. In this project it's very easy to fall into the trap of eagerly implementing tons of features and translations with the goal of being feature-complete (and because it's fun/interesting), but which almost nobody will use, and which we then have to maintain. So while it may sound odd, I'd say feature-completeness isn't exactly an explicit goal.

If you see anything you're interested in it would definitely be great to have you (and anyone else) as a contributor!