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

Improve SQL Server insertion logic and other update pipeline optimizations #27573

Merged
1 commit merged into from
Mar 17, 2022

Conversation

roji
Copy link
Member

@roji roji commented Mar 4, 2022

Also make RETURNING the default INSERT strategy for retrieving db-generated values (for other providers).

Fixes #27372
Fixes #27503
Fixes #24835

@roji roji requested review from AndriySvyryd and removed request for AndriySvyryd March 4, 2022 17:47
@roji roji force-pushed the UpdatePipeline/SqlServerOutput branch from 49d7651 to 466fe62 Compare March 4, 2022 20:53
@roji roji requested a review from AndriySvyryd March 4, 2022 21:05
@roji roji marked this pull request as ready for review March 4, 2022 21:06
@roji roji force-pushed the UpdatePipeline/SqlServerOutput branch from f81b86d to 96cc0f2 Compare March 5, 2022 14:25
@AndriySvyryd
Copy link
Member

AndriySvyryd commented Mar 9, 2022

Validate that trigger names are unique across the hierarchy and across the schema.
In general, triggers are more like check constraints than sequences, consider separating Name and ModelName. The former can differ from table to table.

@roji
Copy link
Member Author

roji commented Mar 9, 2022

Validate that trigger names are unique across the hierarchy and across the schema.

Triggers generally aren't in any schema (#27372 (comment)), but are considered scoped to their table. However, SQL Server/SQLite do not allow two triggers with the same name in the same database (even when they're on different tables); PostgreSQL does allow this. So I'm adding uniquification in SharedTableConvention (like for indexes, check constraints). Makes sense?

In general, triggers are more like check constraints than sequences, consider separating Name and ModelName. The former can differ from table to table.

Just want to confirm my understanding on why check constraints have model and database names, but not sequence... Is this because (on some databases) you can have multiple check constraints (or triggers) with the same database name (i.e. on different tables), but they still require unique model names so they can be referred to and modified in model-building? Whereas you can never have more than one sequence with the same database name (since they're database-scoped), so we can collapse the database and model names into one?

@roji roji requested a review from AndriySvyryd March 9, 2022 12:20
@roji roji force-pushed the UpdatePipeline/SqlServerOutput branch 2 times, most recently from 80ba153 to fa7d959 Compare March 9, 2022 14:17
@AndriySvyryd
Copy link
Member

AndriySvyryd commented Mar 9, 2022

So I'm adding uniquification in SharedTableConvention (like for indexes, check constraints). Makes sense?

Yes, but we should also add validation like ValidateSharedCheckConstraintCompatibility.RelationalModelValidator to throw a helpful exception message.

Just want to confirm my understanding on why check constraints have model and database names, but not sequence...

It's because they are declared on entity types and therefore can span over multiple tables, but would require a different name for each (or being excluded for some of them)

@AndriySvyryd
Copy link
Member

Also add support in RelationalCSharpRuntimeAnnotationCodeGenerator and a test in CSharpRuntimeModelCodeGeneratorTest

@roji roji force-pushed the UpdatePipeline/SqlServerOutput branch from 21044fb to 2594371 Compare March 15, 2022 23:44
@roji
Copy link
Member Author

roji commented Mar 15, 2022

@AndriySvyryd I think I've addressed most of the above, though I'm sure there's stuff still remaining (it's late 😬).

@roji roji requested a review from AndriySvyryd March 15, 2022 23:44
@roji roji force-pushed the UpdatePipeline/SqlServerOutput branch 4 times, most recently from fa6a0fa to 69c5592 Compare March 16, 2022 22:43
@ghost
Copy link

ghost commented Mar 16, 2022

Hello @roji!

Because this pull request has the auto-merge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

@ghost
Copy link

ghost commented Mar 16, 2022

Apologies, while this PR appears ready to be merged, I've been configured to only merge when all checks have explicitly passed. The following integrations have not reported any progress on their checks and are blocking auto-merge:

  1. Azure Pipelines

These integrations are possibly never going to report a check, and unblocking auto-merge likely requires a human being to update my configuration to exempt these integrations from requiring a passing check.

Give feedback on this
From the bot dev team

We've tried to tune the bot such that it posts a comment like this only when auto-merge is blocked for exceptional, non-intuitive reasons. When the bot's auto-merge capability is properly configured, auto-merge should operate as you would intuitively expect and you should not see any spurious comments.

Please reach out to us at fabricbotservices@microsoft.com to provide feedback if you believe you're seeing this comment appear spuriously. Please note that we usually are unable to update your bot configuration on your team's behalf, but we're happy to help you identify your bot admin.

…tions

Also make RETURNING the default INSERT strategy for retrieving
db-generated values (for other providers).

Fixes dotnet#27372
Fixes dotnet#27503
@roji roji force-pushed the UpdatePipeline/SqlServerOutput branch from 69c5592 to 8b912e1 Compare March 17, 2022 09:59
@ghost
Copy link

ghost commented Mar 17, 2022

Apologies, while this PR appears ready to be merged, I've been configured to only merge when all checks have explicitly passed. The following integrations have not reported any progress on their checks and are blocking auto-merge:

  1. Azure Pipelines

These integrations are possibly never going to report a check, and unblocking auto-merge likely requires a human being to update my configuration to exempt these integrations from requiring a passing check.

Give feedback on this
From the bot dev team

We've tried to tune the bot such that it posts a comment like this only when auto-merge is blocked for exceptional, non-intuitive reasons. When the bot's auto-merge capability is properly configured, auto-merge should operate as you would intuitively expect and you should not see any spurious comments.

Please reach out to us at fabricbotservices@microsoft.com to provide feedback if you believe you're seeing this comment appear spuriously. Please note that we usually are unable to update your bot configuration on your team's behalf, but we're happy to help you identify your bot admin.

@ghost ghost merged commit b906ca3 into dotnet:main Mar 17, 2022
@roji roji deleted the UpdatePipeline/SqlServerOutput branch March 17, 2022 11:23
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants