FreeTDS Development Group

[Frediano Ziglio <freddy77 AT gmail.com>]

2015-03-17 22:40 GMT+00:00 Richard Hughes <cyreve AT gmail.com>:
> On 2015-03-15 at 11:46, Frediano Ziglio wrote:
>> 2015-03-13 21:09 GMT+00:00 Richard Hughes <cyreve AT gmail.com>:
>>> I've been looking at the conversion code today and the only conclusion
>>> I've reached is that everything I did is wrong. I now assume that I
>>> should have used odbc_sql2tds() as my template but that's full of
>>> ipd/apd handling that bcp simply doesn't have. Since the first bug I
>>> noticed involved nvarchar, I was trying to figure out how the
>>> character set conversion was supposed to work and now I'm more lost
>>> than when I started.
>>
>> Perhaps I was too direct. I don't think what you did is wrong. First
>> bcp support is quite strange in ODBC. First is a sql server only
>> extension on a quite standard API. Second it use types more similar to
>> dblib than ODBC. Looks like Microsoft copied bcp source into ODBC so
>> you did actually the same. Nothing wrong in it. I don't know actually
>> how MS handle nvarchar in bcp code. We can surely help with
>> conversions. Perhaps you should focus on a problem at a time, you are
>> starting solving too much stuff altogether.
>
> Here's a v2 patch, with nvarchar and decimal now working. You were
> right - I wasn't as wrong as I thought I was (at least from the point
> of view of just getting it working).
>
>>> Hopefully everything except the datetime2/numeric conversion can be
>>> put into libTDS, but I'm neither knowledgeable enough nor skilled
>>> enough to be able to participate in an architectural discussion. If
>>> you tell me what you're planning to work on and what you need me to
>>> change then I'll happily do as I'm told. In the mean time I've got a
>>> lot of tests to write.
>>
>> Good, tests are always good. You can have a look at dblib t0016 and
>> t0017 tests just to get some code to copy/paste. What the problem with
>> datetime2/numeric ?
>
> I ended up mostly stealing from dblib's predictably-named 'bcp.c'
> test. It does a lot of useful stuff, and the version in the new patch
> now passes for me.
>
> That tests most server-side datatypes, so next on the list is testing
> conversions from client-side datatypes. Problem is, I can't find
> anywhere which documents actually what the structures you're supposed
> to pass are. I've proven by experiment that Microsoft's driver expects
> SQL_TIMESTAMP_STRUCT/SQL_NUMERIC_STRUCT for datetime2/numeric
> respectively, but it's going to be really tedious to determine all the
> types simply by trying them out on Microsoft's driver. Oh well.
>
>>>>> The HDBC problem should be discussed. Because the driver manager has
>>>>> no idea about the bcp APIs, the first parameter that all the functions
>>>>> receive is a driver manager HDBC, not a TDS_DBC*.
>>>>> SQLGetInfo(SQL_DRIVER_HDBC) can be used to convert, but that means we
>>>>> need to find SQLGetInfo. Approaches I've thought of so far are:
>>>>>
>>>>> 1) dlopen(...,RTLD_NOLOAD) with various hard-coded filenames to try to
>>>>> find which driver manager the application is using
>>>>>
>>>>> 2) dl_iterate_phdr() to look for filenames that look like the driver
>>>>> manager [this is what's currently in the patch].
>>>>>
>>>>> 3) dlsym(RTLD_DEFAULT,...).
>>>>>
>>>>> 4) Create a gratuitous libtdsodbcbcp.so containing just thunker
>>>>> functions whose sole purpose is to get out of the way of the default
>>>>> symbol search path so that method (3) works more reliably.
>>>>>
>>>>> Microsoft's Windows driver uses (1) because they know there's only one
>>>>> driver manager. (3) is subject to ordering problems with the symbol
>>>>> search, but those problems exist anyway at link time so I'm beginning
>>>>> to think that might be best.
>>>>>
>>>
>>>> (3) should be fine as odbc drivers (like our one) should be loaded
>>>> with RTLD_LOCAL flag so the SQLGetInfo from our driver is not found
>>>> with RTLD_DEFAULT.
>>>>
>>>> Another idea would be to use a sort of "tunnel" using functions like
>>>> SQLSetConnectAttr. Practically let's say we want to implement
>>>> bcp_collen, we do something like
>>>>
>>>> typedef struct  {
>>>>   DBINT cbData;
>>>>   INT idxServerCol;
>>>> } bcp_collen_params;
>>>>
>>>> RETCODE bcp_collen (
>>>>         HDBC hdbc,
>>>>         DBINT cbData,
>>>>         INT idxServerCol)
>>>> {
>>>>    bcp_collen_params params = { cbData, idxServerCol };
>>>>     return SQLSetConnectAttr(hdbc, SQL_SS_FUNC_BCP_COLLEN, &params,
>>>> sizeof(params));
>>>> }
>>>
>>> This actually doesn't solve the problem. That call to
>>> SQLSetConnectAttr() will bind to the one in odbc.c, not the one in the
>>> driver manager - it's the same problem as trying to call SQLGetInfo()
>>> directly. I'll go with my option (3). It has the surprising property
>>> that if you link with "gcc -ltdsodbc -lodbc" then you'll bind directly
>>> to FreeTDS and be confused that attempting to use any other driver
>>> doesn't work, but we'll just have to document that you must use "gcc
>>> -lodbc -ltdsodbc".
>>
>> Well, the wrapper should be statically linked to your application so
>> it can call SQLSetConnectAttr as any other normal DM function. Just my
>> idea. We can work on this problem while working on others.
>
> Ah, kind of like my option (4), but with a static library instead of a
> dynamic one.
>
>
> I'm beginning to consider my patch nearly 'done', in that it works
> more-or-less reliably as a standalone piece of functionality (and
> includes at least the functionality that I need for my application). I
> expect one more version with more tests (as mentioned above) and that
> might cause a little surgical bug fixing but no massive overhauls.
>
> If you could do a full review of this version and tell me where I can
> tidy it up, I'd appreciate it. I'm not too optimistic about being able
> to de-duplicate this bit of functionality because, while the
> high-level look is the same as dblib, just about every detail has had
> some amount of tweaking. When file I/O comes in then that will no
> longer be true, but currently it's a tricky problem.
>
> Richard.

Looks good. It's getting more into shape. Do you remember from which
changeset you start with your change? Do you have a git branch
somewhere?

About the linking stuff I have some idea. One is define bcp functions
with a macro that translate the header, something like

#define bcp_done(dbc) tdsodbc_bcp_done(tdsodbc_bcp_driver_hdbc(dbc))
static inline HDBC
tdsodbc_bcp_driver_dbc(HDBC dbc)
{
   SQLSMALLINT handle_len;
   if (SQLGetInfo(dbc, SQL_DRIVER_HDBC, &dbc, sizeof(dbc),
&handle_len) != SQL_SUCCESS)
      return NULL;
   return dbc;
}

With the function defined in the header to so the SQLGetInfo call is
made from the application

However I think that the way you call tdsodbc_bcp_* functions is not
safe. In Unix there is a different concept. While on Windows you say
(this is done implicitly by linker) I want function X from library Y
in Unix usually you just say I want function X from global symbols.
Now the problem is that if you link the DM (like unixODBC) and
libtdsodbc and you call a function like SQLGetInfo are you calling
unixODBC or FreeTDS? Well... depends from linker order,
initializations and so on. Not safe. In Microsoft documentation (like
https://msdn.microsoft.com/en-us/library/ms130922.aspx) is explicitly
said that you cannot link different drivers, similar linker problem.
The DM solve the problem as it loads libraries with LOCAL flag so
symbols do not get into global list but you just use dlsym to manually
get them. Not a big deal as you can use dlopen(..., LOCAL|NOLOAD) to
get library header and dlsym to get symbols. However you have to know
library name which could be tricky in a portable way (path change
based on configuration flags and filename change from system to
system). This was why I had the idea to incapsulate calls using
SQLSetConnectAttr (or similar) as DM will convert the handle and call
proper driver function no matter what. You could also extend
SQLGetInfo to return information you need directly from driver (like
handle, function pointers and so on).

Frediano