Apply fill factor when creating tables #33407
Conversation
| .Append(ColumnList(operation.Columns)) | ||
| .Append(")"); | ||
|
|
||
| KeyTraits(operation, model, builder); |
There was a problem hiding this comment.
Am not sure at all, but do we think unique constraints have common options/traits with primary keys? I thought e.g. fill factor is defined at the index level, rather than on the unique constraint (not sure).
Maybe it's better to be safe and make this a SQL Server-only thing rather than a relational thing?
There was a problem hiding this comment.
I don't know. @AndriySvyryd approved the original PR.
There was a problem hiding this comment.
SQL Server supports most of the index options at the primary key level, don't know about other databases
There was a problem hiding this comment.
Basically, we don't need KeyTraits, just call IndexOptions here. Only the relevant facets will be present on the operation.
| /// <param name="model">The target model which may be <see langword="null" /> if the operations exist without a model.</param> | ||
| /// <param name="builder">The command builder to use to add the SQL fragment.</param> | ||
| protected virtual void KeyWithOptions(MigrationOperation operation, MigrationCommandListBuilder builder) | ||
| protected virtual void KeyTraits(MigrationOperation operation, IModel? model, MigrationCommandListBuilder builder) |
There was a problem hiding this comment.
For sequence/index I thing we use the word "Options" rather than "Traits"
There was a problem hiding this comment.
This copies the pattern used for IndexTraits which is the already existing method right above this one.
There was a problem hiding this comment.
Note however, we call IndexTraits at different places. I know know if it needs to called at different places--again @AndriySvyryd approved the original PR.
There was a problem hiding this comment.
Ah, missed that we had IndexTraits, makes sense I guess (though not sure why we have options in some places and traits in others...)
|
@dotnet/efteam If there is concern that this is implemented incorrectly, then I don't think we should spend a lot of time on it, but rather just revert the community PR that was approved and then revisit it at a later date when we have time to review by more people than just @AndriySvyryd. |
| /// <summary> | ||
| /// Generates a SQL fragment for extra with options of a key from a | ||
| /// <see cref="AddPrimaryKeyOperation" /> or <see cref="AddUniqueConstraintOperation" />. | ||
| /// Generates a SQL fragment for traits of an primary key or alternate key from a <see cref="CreateTableOperation" />, |
There was a problem hiding this comment.
| /// Generates a SQL fragment for traits of an primary key or alternate key from a <see cref="CreateTableOperation" />, | |
| /// Generates a SQL fragment for traits of a primary key or alternate key from a <see cref="CreateTableOperation" />, |
|
@AndriySvyryd @roji Updated. |
Fixes #33269