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

feat(go/adbc)!: close database explicitly #1460

Merged
merged 3 commits into from
Jan 19, 2024

Conversation

levakin
Copy link
Contributor

@levakin levakin commented Jan 13, 2024

Implicit database release behaves inconsistently on different OS, which leads to bugs.

BREAKING CHANGE: adds Close to the Database interface.
Closes #1306.

@lidavidm
Copy link
Member

CC @zeroshade this affects the core definitions so we're going to want a different way to proceed.

@levakin
Copy link
Contributor Author

levakin commented Jan 15, 2024

@lidavidm do you mean adbc.Database cannot be extended?

Copy link
Member

@zeroshade zeroshade left a comment

Choose a reason for hiding this comment

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

@lidavidm Given that we're just adding a new method to an interface that it is not expected that others will be implementing outside of us, I think it's okay to just add the Close method to the interface along with the implementations.

go/adbc/driver/driverbase/database.go Outdated Show resolved Hide resolved
@lidavidm lidavidm changed the title feat(go/adbc): Close database explicitly feat!(go/adbc): close database explicitly Jan 17, 2024
Copy link

⚠️ Please follow the Conventional Commits format in CONTRIBUTING.md for PR titles.

@lidavidm lidavidm changed the title feat!(go/adbc): close database explicitly feat(go/adbc)!: close database explicitly Jan 17, 2024
@levakin levakin force-pushed the feature/go_explicit_database_close branch from 1443e74 to 75b0372 Compare January 17, 2024 19:43
Copy link
Member

@zeroshade zeroshade left a comment

Choose a reason for hiding this comment

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

Are the FlightSQL test failures related to this change or do they exist in main without this change?

Implicit database release behaves inconsistently on different OS, which leads to bugs.

Closes apache#1306
@levakin
Copy link
Contributor Author

levakin commented Jan 18, 2024

I'll now rebase on the latest main. But I suppose new changes are causing an issue. Could you please look into it too @zeroshade ? I'll share if I find anything.

Failing job

@levakin levakin force-pushed the feature/go_explicit_database_close branch from 75b0372 to 2bdb8b4 Compare January 18, 2024 01:12
@zeroshade
Copy link
Member

@levakin one of the tests calls AdbcDatabaseNew and then immediately calls AdbcDatabaseRelease, meaning it's releasing an uninitialized database. the db member of the cDatabase object is only initialized when AdbcDatabaseInit is called, until then it is just nil. So you were calling cdb.db.Close() when cdb.db was nil.

So just needed to add a nil check there and all is well 😄

@levakin
Copy link
Contributor Author

levakin commented Jan 19, 2024

@zeroshade Nice! Thanks!
What is left to do to merge the PR?

@zeroshade zeroshade merged commit 3aa0d12 into apache:main Jan 19, 2024
33 checks passed
soumyadsanyal pushed a commit to soumyadsanyal/arrow-adbc that referenced this pull request Jan 31, 2024
Implicit database release behaves inconsistently on different OS, which
leads to bugs.

BREAKING CHANGE: adds Close to the Database interface.
Closes apache#1306.

---------

Co-authored-by: Matt Topol <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

bug(go/adbc/drivermgr) Segfault when using drivermgr with the DuckDB
3 participants