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

Revert statement that is breaking sqlite on OSX. #51682

Merged
merged 3 commits into from
Mar 8, 2021

Conversation

CyrusNajmabadi
Copy link
Member

No description provided.

@CyrusNajmabadi CyrusNajmabadi requested a review from a team as a code owner March 4, 2021 23:55
@CyrusNajmabadi
Copy link
Member Author

CyrusNajmabadi commented Mar 4, 2021

Tagging @ericsink @Therzok @sharwell @AArnott

@CyrusNajmabadi
Copy link
Member Author

Not sure what the OSX issue is here. On @Therzok to continue the investigation with @ericsink and Sqlitepcl.

@Therzok
Copy link
Contributor

Therzok commented Mar 5, 2021

I think I figured it out, but not sure how to validate it works properly: ericsink/SQLitePCL.raw#407

@davidwengier
Copy link
Contributor

Thank you @CyrusNajmabadi and @Therzok for tracking this down ❤️

Copy link
Contributor

@AArnott AArnott left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sad, since this reverts a good change. I hope whatever the underlying bug is can be fixed and this workaround reverted soon.

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Auto-approval

Copy link
Member

@sharwell sharwell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you make the change conditional on the current operating system (i.e. avoid changing the behavior for Windows)?

@ericsink
Copy link

ericsink commented Mar 5, 2021

Just in case others have not seen it, ericsink/SQLitePCL.raw#407 contains a writeup of what's going on here.

@CyrusNajmabadi
Copy link
Member Author

@sharwell done.

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Auto-approval

@ericsink
Copy link

ericsink commented Mar 5, 2021

Discussion of this issue is happening on the sqlite forum at:

https://sqlite.org/forum/forumpost/de445cf241

Notable quote from DRH: "Trying to open an on-disk database using "mode=memory" doesn't make sense. Where were you trying to accomplish?"

He also reports that he was unable to repro the differing behavior, but I'm not yet sure why.

@ericsink
Copy link

ericsink commented Mar 5, 2021

Continuing to parrot things from the sqlite forum thread:

If "mode=memory" is removed, which DRH said does not make sense, then the behavior across Windows and other platforms no longer differs. In other words, it gives the error in all cases.

@CyrusNajmabadi
Copy link
Member Author

@AArnott for info here. This was added in response to what the platform cache is doing.

@AArnott
Copy link
Contributor

AArnott commented Mar 5, 2021

@ericsink Thanks for referencing the sqlite thread. I responded with a defense of what we're doing, pointing to the docs that explain why this is legit.

@CyrusNajmabadi
Copy link
Member Author

Merging this in now. If we get resolution in https://sqlite.org/forum/forumpost/9babfc9cba?t=h as to what to do here, we can update accordingly.

@CyrusNajmabadi CyrusNajmabadi merged commit 807aded into dotnet:main Mar 8, 2021
@CyrusNajmabadi CyrusNajmabadi deleted the revertSql branch March 8, 2021 22:33
@allisonchou allisonchou added this to the Next milestone Mar 12, 2021
@allisonchou allisonchou modified the milestones: Next, 16.10.P2 Mar 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants