npgsql / efcore.pg

Entity Framework Core provider for PostgreSQL
PostgreSQL License
1.59k stars 227 forks source link

Make HasIndex work on JSON properties #2568

Open roji opened 2 years ago

roji commented 2 years ago

Like https://github.com/dotnet/efcore/issues/29623.

PG documentation

The simple, general thing is to create an expression index. It's also possible to speed up containment specifically via a GIN index, but that's limited (explore further).

rwasef1830 commented 2 years ago

A long part of this can be accomplished if the EF Core API were improved such that it allowed arbitrary expression indices. Yes, I know we can write raw Sql migrations, but having it defined in the metadata allows for automatic maintenance (such as generation of drop/recreate automatically when the expression changes). You can also have the user specify which Entities the expression depends on so that the ordering of the statement generation doesn't lead to a non-executable migration.

And ditto for triggers and trigger functions, would be so nice.

roji commented 2 years ago

Arbitrary expression index support is covered by https://github.com/dotnet/efcore/issues/28360; I think that's largely orthogonal to this issue; users should be able to just do HasIndex on a property inside JSON without knowing anything about expression indexes or how to configure them.

More generally about expression indexes, triggers and other similar things: there's a huge number of possible things you can do in databases, and it's unfeasible to try to cover all of them in EF. Triggers and indexes are examples of features which don't affect EF's runtime behavior in any way; that means that the only value of adding APIs for them is for a nicer user experience when configuring them - since users can always do raw SQL in migrations. In addition, covering all the possible configuration options of e.g. triggers using .NET EF APIs is a lot of work, and doing that just so that users can use .NET instead of SQL seems... unjustified. That's why I generally think this kind of thing is pretty low priority.

rwasef1830 commented 2 years ago

Yes, I kind of agree but I just wish the EF makes an easier API to specify an arbitrary thing and table entities dependencies and an ordering value and how to create it as sql and how to drop it as sql and let me set my annotations and hook in the differ so I can have automatic maintenance of these things.

This is currently possible today but the API is buried very deeply and is awkward to use and debug. (and is marked with skeletons and warning signs of this being internal APIs subject to change and here be dragons and no documentation). It's also annoying that a lot of very useful stuff is marked internal making it hard to touch or appropriate.

Such an API will allow the community to create packages that define higher level apis such as views and triggers and any arbitrary database feature.

roji commented 2 years ago

This is currently possible today but the API is buried very deeply and is awkward to use and debug. (and is marked with skeletons and warning signs of this being internal APIs subject to change and here be dragons and no documentation). It's also annoying that a lot of very useful stuff is marked internal making it hard to touch or appropriate.

Which exact internal API are you referring to here?

Yes, I kind of agree but I just wish the EF makes an easier API to specify an arbitrary thing and table entities dependencies and an ordering value and how to create it as sql and how to drop it as sql and let me set my annotations and hook in the differ so I can have automatic maintenance of these things.

That doesn't really work... The differ needs to be able to know which parts of the thing to compare (trigger name, type, SQL definition...), produce an operation that the migrations SQL generator can recognize, and the migrations SQL generator needs to know how to render that operation as SQL. In theory we can provide extensibility points for all these, but the specifics are more complicated than they look.

You can indeed already set arbitrary annotations on things (e.g. on a table, on a model), and the differ would detect changes and create the appropriate operation. But the migrations SQL generator would still need to know to render the correct SQL for it.

Overall, I'm not seeing why this should all be extensible and implemented in external packages... There aren't 100 unsupported database types (like triggers, view), and if someone really wants to, they can contribute support into EF itself. That would save us from having to design good extension points to e.g. the migrations SQL generator, which wouldn't really be useful otherwise.

PS in EF Core 7.0 we did add metadata support for triggers, so it should be possible for providers to add their provider-specific modeilng around triggers and support them. Again, I think the value of doing so is quite low, given that people can just manage triggers via raw SQL. Not everything needs a .NET API.

rwasef1830 commented 2 years ago

Which exact internal API are you referring to here?

IRelationalAnnotationProvider, the linq expression visitor, the IMigrationsModelDiffer.

I noticed in EF successive releases that the team is doing good work to opening up these APIs with extensibility points, but anyway, I was stating some of the pain points I felt when I implemented a custom kind of index for EF Core (pgroonga) and I had to write an annotations provider, a decorator for the differ to manipulate the list of operations, and a migration sql generator, then a EF Core "plugin", and I needed to decorate a some services inside the internal container.

One of the challenges I faced was EF Core's requirement that indexes be tied to specific properties, and before EF Core 5 there was the issue of only being able to specify one index per set of properties. So I'm forced to specify "decoy" columns, then make another annotation with the real index expression, then hack the generator to generate different sql when my annotation was present.

It feels unnecessarily difficult, but I do appreciate it's at least possible. Anyway, the point of my discussion was simply to raise possibilities of things that would be nice to make possible or keep in mind as a general guidance of direction for the future.