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

Fixed: ODBC DSN connection always used :memory: no matter what you set in odbc.ini #46

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

kkovacs
Copy link

@kkovacs kkovacs commented Dec 11, 2024

This PR should be fixing #29 .

The issue appears to be that if one uses SQLConnect() (as opposed to SQLDriverConnect(), then dbc->dsn is never set, and thus this part of the .ini parsing function always returns before even parsing the .ini file.

Here:

SQLRETURN Connect::ReadFromIniFile() {
#if defined ODBC_LINK_ODBCINST || defined WIN32
	if (dbc->dsn.empty()) {
		return SQL_SUCCESS;
	}

at https://github.com/duckdb/duckdb-odbc/blob/154f517dadc60efc5d802650c1e572a786992a4e/src/connect/connect.cpp#L98C1-L103C1

I've tested the fix on Linux (Ubuntu 24.04).

Edit: added reference to the original issue, typo

@kkovacs
Copy link
Author

kkovacs commented Dec 12, 2024

Hi guys, can someone kick the tests to re-run? I doubt that the sh -c "cat /etc/*release | grep ^ID error would be related to my change...

@kkovacs
Copy link
Author

kkovacs commented Dec 14, 2024

Maybe I can ask you @guenp to have a look at why checks fail with shell errors...?

@malcook
Copy link

malcook commented Dec 16, 2024

hi - I reported #29 and am happy to see this progress.

If there is any way I can assist in testing these changes before release, please advise how to deploy these changes, and I will try to help.

@guenp
Copy link
Contributor

guenp commented Dec 17, 2024

Thank you @kkovacs and @malcook ! I will take a look and test it

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.

3 participants