Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Check that SequentialGuidValueGenerator still matches the SQL Server behavior #19124

Closed
roji opened this issue Dec 2, 2019 · 8 comments
Closed
Assignees
Labels
area-perf closed-out-of-scope This is not something that will be fixed/implemented and the issue is closed.

Comments

@roji
Copy link
Member

roji commented Dec 2, 2019

A discussion on the Pomelo MySQL provider indicates that the behavior of SQL Server's NEWSEQUENTIALID() function may have changed in SQL Server 14 (2017). If this is the case, we may need to update our implementation of SequentialGuidValueGenerator to make it generate sequential GUIDs based on the new behavior, for efficiency.

If there indeed was a behavior change, we may consider choosing a SequentialGuidValueGenerator based on a user-declared version of the server version, keeping the old behavior for pre-2017.

On a somewhat related note, consider whether SequentialGuidValueGenerator really belongs outside of SQL Server. If it implements a generic RFC-specified behavior that sounds good, but if it's doing something that's very SQL Server specific we should consider moving it into the provider to prevent confusion.

@ErikEJ
Copy link
Contributor

ErikEJ commented Dec 2, 2019

I can find no evidence of any change in SQL Server 2017. Only some updates/fixes relating to SQL Server 2017 on Linux. Is it platform (little/big endian) confusion?

@mguinness
Copy link

@rgward Based on the article SQL Server on Linux: CU4 – NewSequentialId() – Uuid can you shed any light on this? EF Core uses SequentialGuidValueGenerator for comparison.

@roji
Copy link
Member Author

roji commented Dec 2, 2019

Is it platform (little/big endian) confusion?

Does SQL Server actually run on any big endian platform? That would be new to me...

In any case, @lauxjpn do you have any info on this behavior change in SQL Server? Is this something you saw in a doc, or different actual behavior across these versions?

@lauxjpn
Copy link
Contributor

lauxjpn commented Dec 2, 2019

As far as I tested it then, there was no recent change (tested it in MSSQL 14 and MSSQL 10 if I remember correctly), but the method how EF Core generates its Guids seems to diverge from how SQL Server's NEWSEQUENTIALID() does it anyway.

While SQL Server increments the first bytes, the EF Core implementation increments the last bytes. This general location should be independent of the endianness (though the order within the location could change due to endianness).

I tested the SQL Server native side (T-SQL) with NEWSEQUENTIALID() and generated 3 Guids:

5addfff2-8310-ea11-953c-000c29a016f1
5bddfff2-8310-ea11-953c-000c29a016f1
5cddfff2-8310-ea11-953c-000c29a016f1

Here, the first 8 bytes changed (or a small portion of them), while the EF Core implementation changes the last 8 bytes according to the codebase (unless there is some magic happening inside of Guid itself that I missed):

public override Guid Next(EntityEntry entry)
{
    var guidBytes = Guid.NewGuid().ToByteArray();
    var counterBytes = BitConverter.GetBytes(Interlocked.Increment(ref _counter));

    if (!BitConverter.IsLittleEndian)
    {
        Array.Reverse(counterBytes);
    }

    // --> The last 8 bytes change:
    guidBytes[08] = counterBytes[1];
    guidBytes[09] = counterBytes[0];
    guidBytes[10] = counterBytes[7];
    guidBytes[11] = counterBytes[6];
    guidBytes[12] = counterBytes[5];
    guidBytes[13] = counterBytes[4];
    guidBytes[14] = counterBytes[3];
    guidBytes[15] = counterBytes[2];

    return new Guid(guidBytes);
}

The endianness has only consequences to the order of the bytes inside their locations, but not to the order of the locations themselves.

So that's why I came to the conclusion, that the EF Core und SQL Server implementations differ (I never tested the actual EF Core provider implementation though).

Anyway, my initial statement that this might be due to a recent change in SQL Server, is probably misleading and wrong. Sorry if I guided you down the wrong rabbit holes!

@roji
Copy link
Member Author

roji commented Dec 3, 2019

As far as I tested it then, there was no recent change (tested it in MSSQL 14 and MSSQL 10 if I remember correctly), but the method how EF Core generates its Guids seems to diverge from how SQL Server's NEWSEQUENTIALID() does it anyway.

That would mean that EF Core has always generated non-sequential GUIDs for SQL Server... I'll take a dive at this as well at some point to understand the situation and we'll see what to do.

But my initial statement, that this might be due to a recent change in SQL Server, is probably misleading and wrong. Sorry if I guided you down the wrong rabbit holes!

Don't worry one bit, I didn't spend any time on this yet. Thanks for your research and help on this!

@lauxjpn
Copy link
Contributor

lauxjpn commented Dec 3, 2019

That would mean that EF Core has always generated non-sequential GUIDs for SQL Server.

I think it would mean that EF Core did generate sequential GUIDs, just not in the (index optimized) way SQL Server does, where only the first bytes (probably the first DWORD?) needs to be accessed in most cases, rather than the whole 16 bytes.

@roji
Copy link
Member Author

roji commented Dec 3, 2019

@lauxjpn right, but the whole point here would be to generate GUIDs that are sequential in the same way as if SQL Server generated them, so that the index on the column is clustered efficiently etc. I'll look into it more deeply at some point.

@roji
Copy link
Member Author

roji commented Apr 20, 2020

Following investigation in #20528, it turns out that SequentialGuidValueGenerator generates GUIDs which are optimized for SQL Server - even if they don't resemble those produced by NEWSEQUENTIALID; so we don't need to do anything.

Submitted #20696 to correct the documentation.

@roji roji closed this as completed Apr 20, 2020
@roji roji removed this from the 5.0.0 milestone Apr 20, 2020
@ajcvickers ajcvickers added the closed-out-of-scope This is not something that will be fixed/implemented and the issue is closed. label Mar 10, 2022
@ajcvickers ajcvickers reopened this Oct 16, 2022
@ajcvickers ajcvickers closed this as not planned Won't fix, can't repro, duplicate, stale Oct 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-perf closed-out-of-scope This is not something that will be fixed/implemented and the issue is closed.
Projects
None yet
Development

No branches or pull requests

5 participants