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

Add etc.c.odbc #3398

Merged
merged 8 commits into from
Jun 12, 2015
Merged

Add etc.c.odbc #3398

merged 8 commits into from
Jun 12, 2015

Conversation

andralex
Copy link
Member

Just a little testing with iODBC - could folks on other OSs give it a shot? There are plenty of simple samples online, I went through this:

import etc.c.odbc.sql;
import etc.c.odbc.sqlext;
import std.stdio;

int main() {
  SQLHENV env;
  SQLCHAR driver[256];
  SQLCHAR attr[256];
  SQLSMALLINT driver_ret;
  SQLSMALLINT attr_ret;
  SQLUSMALLINT direction;
  SQLRETURN ret;

  SQLAllocHandle(SQL_HANDLE_ENV, SQL_NULL_HANDLE, &env);
  SQLSetEnvAttr(env, SQL_ATTR_ODBC_VERSION, cast(void *) SQL_OV_ODBC3, 0);

  direction = SQL_FETCH_FIRST;
  while(SQL_SUCCEEDED(ret = SQLDrivers(env, direction,
                       driver.ptr, driver.sizeof, &driver_ret,
                       attr.ptr, attr.sizeof, &attr_ret))) {
    direction = SQL_FETCH_NEXT;
    printf("%s - %s\n", driver.ptr, attr.ptr);
    if (ret == SQL_SUCCESS_WITH_INFO) printf("\tdata truncation\n");
  }

  return 0;
}

Running with rdmd -L-lodbc odbctest.d should print the installed drivers.

@yebblies
Copy link
Member

Seems like a good idea to have this, but why does it belong in phobos instead of deimos and code.dlang.org? Or is it already there?

@andralex
Copy link
Member Author

@yebblies I want to make this official, and plan to add a high-level API on top of it.

I'm seeing weird linker errors at https://auto-tester.puremagic.com/show-run.ghtml?projectid=1&runid=1589661&isPull=true, any idea? I'll preemptively make all "macros" templates.


Adapted with minimal changes from the work of David L. Davis
(refer to the $(WEB
forum.dlang.org/thread/cfk7ql$(DOLLAR)1p4n$(DOLLAR)1@digitaldaemon.com#post-cfk7ql:241p4n:241:40digitaldaemon.com,
Copy link
Member

Choose a reason for hiding this comment

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

Canonical link please

forum.dlang.org/post/cfk7ql$(DOLLAR)1p4n$(DOLLAR)1@digitaldaemon.com

Copy link
Member Author

Choose a reason for hiding this comment

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

done

@CyberShadow
Copy link
Member

@yebblies I want to make this official, and plan to add a high-level API on top of it.

I'm worried that we'll get this wrong and then be forevermore stuck with a crappy API.

There's so many ways to do a DB API, I'm not sure there exists any way that's not wrong in one way or another. How high are we talking about, ORM-level?

@andralex
Copy link
Member Author

I'm worried that we'll get this wrong and then be forevermore stuck with a crappy API.

Paralysis of analysis is what they call it. Right now we're stuck forevermore with no API whatsoever. I bump into people who go like, "D is interesting, I could use it at work. What database connectivity does it have?" The case is lost as soon as I open my mouth to say "Well..."

I bump with a lower but nonzero frequency into folks who are competent and well-intended and enthusiastic about defining the most awesome standard for database connectivity in D. I have so for years. Nothing came out of it.

We can't wait for the kindness of passers-by.

ODBC is mature, ubiquitous, and here to stay. It connects to everything. I know what steps I need to take to make things work with ODBC. And I'll do it, and that's the end of it.

@CyberShadow
Copy link
Member

That's not really what I meant, but you've answered my real question as well.

ODBC doesn't seem to have asynchronous requests (without polling). SQLCompleteAsync exists but only in the MS implementation, it seems.

@andralex
Copy link
Member Author

@CyberShadow Connecting via ODBC does not preclude work on asynchronous APIs (whether or not they're based on ODBC itself).

@andralex
Copy link
Member Author

What in the world are those link errors only on select systems?

@CyberShadow
Copy link
Member

I guess that's fine if you're OK with the general consensus on it to become "it's there but it doesn't scale, so you should use Vibe/etc instead unless you aren't planning to scale or really need ODBC".

@andralex
Copy link
Member Author

I guess that's fine if you're OK with the general consensus on it to become "it's there but it doesn't scale, so you should use Vibe/etc instead unless you aren't planning to scale or really need ODBC".

That's not how I see it.

@CyberShadow
Copy link
Member

That's not how I see it.

Well... how so? Can you propose a way to use ODBC asynchronously from a fiber without blocking the entire thread?

@jacob-carlborg
Copy link
Contributor

  • No change for the Windows makefile?
  • Shouldn't all symbols be documented, at least with any empty documentation comment so it shows up in the generated documentation
  • It's quite a weird comment style:
/+
 '
 '
 +/

/+ The following bitmasks are ODBC extensions and defined in sqlext.d
enum : ulong
{
SQL_AT_COLUMN_SINGLE = 0x00000020L,
Copy link
Member

Choose a reason for hiding this comment

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

I actually can not find SQL_AT_COLUMN_SINGLE inside sqlext.d, CTRL-F tells me it only exists right here

Copy link
Member Author

Choose a reason for hiding this comment

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

@d-random-contributor
Copy link

"D is interesting, I could use it at work. What database connectivity does it have?"

PostgreSQL, MySQL, SQLite, see Libraries_and_Frameworks. 17 packages in dub.

//---------------------------------------------
// Mapping Unicode Functions
//---------------------------------------------
/+
Copy link
Member

Choose a reason for hiding this comment

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

Why keep this as a commented-out block?

Copy link
Member Author

Choose a reason for hiding this comment

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

They're #ifdef'ed out at http://www.opensource.apple.com/source/iodbc/iodbc-36/iodbc/include/sqlucode.h?txt. I'd say keep it there as a memento and decide later whether we version it in upon request.

@dnadlinger
Copy link
Member

I don't see why this should go into Phobos. A Dub package should be more than good enough, at least until that planned proposal for a standard high-level API materializes. Even then, a well-publicized Dub might be more appropriate, given all the issues we ran into with std.net.curl. Consider this to be just about the most formal veto I can extend.

@DmitryOlshansky
Copy link
Member

@klickverbot Agreed on "the second std.net.curl" point.

@CyberShadow
Copy link
Member

given all the issues we ran into with std.net.curl

Thanks for reminding me. I agree, that's a very strong point against including more such code in Phobos. For one example, see #3009

@Kozzi11
Copy link
Contributor

Kozzi11 commented Jun 11, 2015

I don't see why this should go into Phobos. A Dub package should be more than good enough, at least until that planned proposal for a standard high-level API materializes. Even then, a well-publicized Dub might be more appropriate, given all the issues we ran into with std.net.curl. Consider this to be just about the most formal veto I can extend.

Agree, please do not add another c lib dependency to phobos. This should be a dub package.

uint SQL_LEN_BINARY_ATTR
(
uint length
)
Copy link
Contributor

Choose a reason for hiding this comment

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

Very odd syntax for a function declaration. I didn't even recognized it being a function at first.

Copy link
Member Author

Choose a reason for hiding this comment

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

works

@rikkimax
Copy link
Contributor

For reference, a couple of months ago I started an ODBC driver manager.
https://github.com/DNetDev/odbc_drivermanager/
Missing a lot of functionality.
The only real limiting factor right now is the functions are not implemented for the driver manager as per spec. But can load shared libraries thanks to Derelict-Util.

@cruisercoder
Copy link

FWIW, I tried the sample code above using a different OSX driver manager (unixodbc via homebrew). The SQLAllocHandle call was returning a intermittent error depending in the presents of writeln lines above it. After digging in the unixodbc source, I'm still not sure what the issue is, but adding an -m64 flag to a hand build version seems to have solved it (although file said the libs were x86_64 before).
Long story short, ODBC adds a layer that is very sensitive to compiled library settings.

@andralex
Copy link
Member Author

There seems to be a confusion here - this is not adding a C library dependency (the way curl does). User code would need to add linking with ODBC for the calls to succeed.

This has got to be one the most scrutinized collections of declarations (pardon! there are a few definitions as well).

Folks, it's just numbers and function declarations. We're not in the business of distributing any components of ODBC or requiring it being installed. No need to get all up in arms about it. If you like reviewing, there's plenty of real stuff to review. Please focus on what matters. Thanks.

I'll also note I got exactly 1 comment related to actual issues amid the dozen comments.

@CyberShadow
Copy link
Member

this is not adding a C library dependency (the way curl does). User code would need to add linking with ODBC for the calls to succeed.

So why is this PR getting linker errors, then?

@andralex
Copy link
Member Author

let's see how this works out

@andralex
Copy link
Member Author

OK, just figured what's happening. In posix.mak, everything under etc/c/ (i.e. EXTRA_DOCUMENTABLES) is not linked in the library, i.e. it's assumed to contain only declarations. I could add EXTRA_DOCUMENTABLES to the library, but better I make all functions macros.

@andralex
Copy link
Member Author

s/macros/templates/

@cruisercoder
Copy link

I just noticed that at least two types, SQLLEN and SQLULEN, which were added in 2003 for 64-bit appear to be missing. Article: https://msdn.microsoft.com/en-us/library/ms716287(v=vs.85).aspx

@WalterBright
Copy link
Member

Auto-merge toggled on

@CyberShadow
Copy link
Member

Auto-merge toggled on

@WalterBright Did you read the comments? There is clearly no consensus and a lot of opposition against this addition.

WalterBright added a commit that referenced this pull request Jun 12, 2015
@WalterBright WalterBright merged commit 54be279 into dlang:master Jun 12, 2015
@CyberShadow
Copy link
Member

OK, I've had a longish chat with @andralex on IRC, here's the main points:

  1. Templating the functions should avoid all linker issues when ODBC isn't actually used.
  2. ODBC has poor support for asynchronous requests, but Andrei's next goal is to create a database access API which, initially, supports just ODBC, but can be extended to e.g. a Vibe.d-powered backend.

This would allow us to provide a common interface for DB access that is both usable from simple applications (using ODBC) as well as network/server apps (using Vibe.d or custom backends).

Hopefully putting the DB API in Phobos means standardizing it, thus ensuring interoperability between third-party libraries.

@Kapps
Copy link
Contributor

Kapps commented Jun 12, 2015

At the very least, this should explicitly indicate in the module documentation that the ODBC library is required to be linked separately. The point of this seems to be to let newcomers to D get simple, if potentially inefficient, database access, but I'm not sure if those coming from higher level / non-native languages would understand that the ODBC library is not itself included; there are already plenty of those questions with curl. As well as that, the issues of coffimplib / implib on Win32 I remember being a very confusing problem for me when I was new, and I'd imagine the same issue will occur here. There are more than a few questions about it with curl, and including another library in the Standard Library for D that we don't actually distribute the library itself for will cause even more confusion. If users can find the rather hidden coffimplib / DM tools install, and install DMC for implib, they can figure out how to install dub and use this as a third party package (or even better, one of the native alternatives).

As well, the idea of including this in Phobos is just... odd. There exist etc.curl and etc.c.zlib because there exists high level wrappers around them in Phobos, so at that point including the headers itself is a fairly obvious choice. I assume that this is related to a desire to include an std.sql.odbc, but I feel that would be a mistake. D is a native language with a heavy focus on performance, but I'm not sure whether ODBC can give us this performance. What use is a fast language if some of the primary use cases (and bottle-necks) use inefficient means for actually doing what they need to do? Further, vibe.d is a very prominent tool with D and this simply does not work properly with it, again causing confusion for new users when they wonder why their programs perform so poorly despite using an asynchronous IO framework. Regardless, if such a thing is desired in the standard library, this should probably be included when it is and remain in Deimos + dub until then. Otherwise, why are we including arbitrarily supporting this library and not some random math libraries, or image libraries, or ..?

@andralex
Copy link
Member Author

@Kapps yes, I'll follow with a PR with instructions on linking with odbc.

@andralex
Copy link
Member Author

@cruisercoder: thanks for investigating. If you find anything, please follow up with PRs so we stabilize this. After that I expect the ongoing maintenance costs will be minimal if any. Thx!

@andralex
Copy link
Member Author

FYI: #3401

@MartinNowak
Copy link
Member

Most ODBC drivers are notoriously slow. I hope we don't sell this as our main DB interface.

@CyberShadow
Copy link
Member

I think the plan is to leave ODBC as the crappy-but-ubiquitous fallback backend for the TBA database API.

@MartinNowak
Copy link
Member

Ah OK, sounds good, we just need to be careful from a marketing perspective.

9rnsr added a commit to 9rnsr/phobos that referenced this pull request Jul 1, 2015
This reverts commit 54be279, reversing
changes made to 8772f50.
9rnsr added a commit to 9rnsr/phobos that referenced this pull request Jul 1, 2015
9rnsr added a commit to 9rnsr/phobos that referenced this pull request Jul 7, 2015
This reverts commit 54be279, reversing
changes made to 8772f50.
9rnsr added a commit to 9rnsr/phobos that referenced this pull request Jul 7, 2015
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.