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

memory optimized option disappears after being checked #19425

Closed
alanrenmsft opened this issue May 19, 2022 · 6 comments
Closed

memory optimized option disappears after being checked #19425

alanrenmsft opened this issue May 19, 2022 · 6 comments
Assignees
Labels
Area - Designer Designer for Table, View... Bug Triage: Done

Comments

@alanrenmsft
Copy link
Contributor

  1. open a new table designer on a database that does not support memory optimized table
  2. check the memory optimized checkbox.
    the option disappears.

graph-table

@alanrenmsft alanrenmsft added Bug Triage: Done Area - Designer Designer for Table, View... labels May 19, 2022
@alanrenmsft alanrenmsft assigned alanrenmsft and caohai and unassigned alanrenmsft May 19, 2022
@alanrenmsft
Copy link
Contributor Author

Expected behavior: it should be disabled.

@caohai
Copy link
Member

caohai commented May 19, 2022

If we expect this option to be correctly disabled right after initialization for db that CAN enable memory optimized but hasn't, we will need to get this info from query BEFORE the table designer is loaded.

The current implementation sets IsMemoryOptimizedTableSupported with only version check during initialization and runs query with async and we only use await on the task before publishing.

This issue happens because IsMemoryOptimizedTableSupported is initialized as true with version check so we can see those options in UI when the table designer is loaded.
Then the query execution gets completed and it updates IsMemoryOptimizedTableSupported to false, this can happen before we check the checkbox of Memory Optimized in UI, thus the new view won't contain these options any more, causing the inconsistent state.

Making this query call sync seems a reasonable fix and ensures consistency, and the time cost should be negligible compared to model import.
The other approach will be only committing query check result to IsMemoryOptimizedTableSupported before publish, this will still cause inconsistent state if a user continues to design after clicking publish. I guess having this check being async makes it tricky in terms of handling state change of IsMemoryOptimizedTableSupported.

thoughts? @alanrenmsft

@alanrenmsft
Copy link
Contributor Author

I think it is good to keep these operations async. we can follow the pattern below for similar scenarios:

  1. always have the option enabled
  2. a. if user try to publish the change prior to we can determine whether it is supported, we will let sql server engine reject it
  3. b. otherwise, if we detect that user checked the option but it is not supported, instead of hiding it, we should add a validation error so that user knows it and can uncheck the option.

@alanrenmsft
Copy link
Contributor Author

if it is not supported and not checked then we can disable it, so that it is consistent with other options?

@caohai
Copy link
Member

caohai commented May 20, 2022

I think it is good to keep these operations async. we can follow the pattern below for similar scenarios:

  1. always have the option enabled
  2. a. if user try to publish the change prior to we can determine whether it is supported, we will let sql server engine reject it
  3. b. otherwise, if we detect that user checked the option but it is not supported, instead of hiding it, we should add a validation error so that user knows it and can uncheck the option.

For the publish part, we currently wait for all async tasks to finish before running validation and we do a manual check for this property there and throw exception if memory-optimized is enabled but not supported. So we should be good there.
Other parts of the pattern make sense to me, I tested in STS and it looks good, should have a PR out shortly,

@caohai
Copy link
Member

caohai commented May 25, 2022

Fixed by STS vbump in #18050

@caohai caohai closed this as completed May 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area - Designer Designer for Table, View... Bug Triage: Done
Projects
None yet
Development

No branches or pull requests

2 participants