Add support for index sort options

[khellang]

Closes #326

[austindrenski]

Looks good, just a few questions.

[austindrenski]

@khellang Would you mind rebasing when you have a chance?

[roji]

Aside from the API shape (one method vs. several), I have a more general comment.

While it's totally fine to have this control over the index, it's important to think about the interaction with ORDER BY, which also has NULLS FIRST/LAST (see #627). The ORDER BY null ordering is managed globally as a context option, and cannot be controlled query-by-query. At the moment that feature is internal-only, for testing purposes (compatibility with MSSQL), since if users start using NULLS FIRST while their indices are NULLS LAST there may be a significant perf degradation (haven't actually checked this). If there really is a need to expose ORDER BY null ordering to users, we may want to have a single context option which also affects the index setting (so that all indices would be created NULLS FIRST as well).

Just something to think about, this doesn't necessarily impact this PR.

[githubfanster]

Hi! I was just wondering if this change has been merged and available for us to use? if so, what version? thanks for new info!

[khellang]

Hi! I was just wondering if this change has been merged and available for us to use? if so, what version? thanks for new info!

@githubfanster Nope, it's still sitting here, unmerged. I'll see if I can pick this up again.

[khellang]

Alright, updated. Hopefully there should be few issues left now 🤞

[roji]

Looks great! See my small comments, other than that from my side we're good to go.

One last thing is that there's an issue tracking ASC/DESC support for indices at the EF Core level - dotnet/efcore#4150. Ideally this would be implemented there and we'd reuse the same SortOrder enum, or whatever mechanism is accepted there. If not, I think it's OK for us to release our own thing too - we shouldn't get blocked.

However, @khellang or someone else, if you feel like submitting this functionality upstream that would be ideal - we could adapt our own implementation to that. I wouldn't block this PR for that, though.

[roji]

Thanks @khellang, this looks great!

[khellang]

I've started on the scaffolding bits for this, but need a bit more time to get the tests written properly. I don't think this PR needs to wait for it though. Just letting you know 😊

[roji]

Looks very good @khellang, thanks.

First, see the comment about the possible method-specificity of the sorting bits in scaffolding.

Also, it seems like collation isn't being scaffolded - this seems like it might be a simple matter of joining on indcollation etc. For completeness it would be nice to have this in this PR as well - did you run into any issue with this?

[khellang]

Also, it seems like collation isn't being scaffolded - this seems like it might be a simple matter of joining on indcollation etc. For completeness it would be nice to have this in this PR as well - did you run into any issue with this?

Ah, no, for some reason I just forgot 😂

[khellang]

Hmm. Looks like a legit CI failure. Passed on my Mac. I wonder if there's a difference between included collation on different OS'es? 🤔

[austindrenski]

Possibly... Or something else related to the updated base image (#822, appveyor/ci#2872).

[khellang]

Looks like it (under 23.2.2.1. Standard Collations):

On all platforms, the collations named default, C, and POSIX are available. Additional collations may be available depending on operating system support.

I guess I'll have to use POSIX for the unit test then 😅

[austindrenski]

Looks like it:

On all platforms, the collations named default, C, and POSIX are available. Additional collations may be available depending on operating system support.

I guess I'll have to use POSIX for the unit test then 😅

Nice find. Can you edit to include a citation?

[khellang]

Nice find. Can you edit to include a citation?

Updated. (https://www.postgresql.org/docs/current/collation.html, under 23.2.2.1. Standard Collations)

[roji]

Thanks, it's great to see the collation scaffolding support.

Two things remain from my side - yet another annoying comment from me about the default sort order and implications, and a confirmation on the indoption bit parsing across index methods.

[khellang]

Hopefully most issues should be solved now 🤞

[khellang]

Hmm. That CI failure (Test_change_tracking_key_sizes) seems to be unrelated?

[roji]

@khellang that has nothing to do with you, just pushed a commit to skip that test for now, can you please rebase on top of that?

[roji]

I think we can merge this - see two unimportant comments, you can decide if you want to deal with them or not. Otherwise either squash your commits and force-push or let me know and I'll take care of everything.

Thanks for this valuable contribution and for the considerable patience while working on this :)

[roji]

@khellang I think we're still waiting on some very minor things here, can you confirm? It would be good to finally merge :)

[khellang]

Yup, I just haven't had time to fix them yet. I'll get to it this week 👍

[khellang]

@roji Sorry about the delay. I've added the tweaks from the last review and squashed the commits 👍

[roji]

Merged, thanks for the patient work @khellang!