Re: ODBC bcp

2015-03-13 21:09 GMT+00:00 Richard Hughes <[email protected]>:
> On 2015-03-13 at 15:37, Frediano Ziglio wrote:
>> 2015-03-12 19:36 GMT+00:00 Richard Hughes <[email protected]>:
>>> The patch steals a large amount of code from dblib/bcp.c but, in
>>> order to give myself an achievable goal, it implements *only* bcp in
>>> from RAM; all of the file I/O stuff has been ripped out. I believe
>>> this is a very sane and useful subset to have because my application
>>> does the bcp as one step of a single larger transaction and hence
>>> needs to use the existing ODBC connection. People wanting the file I/O
>>> bit probably don't have this constraint and hence can just use dblib
>>> or freebcp.
>>>
>>> A very useful document is [1] which lists the API differences from
>>> dblib. The most annoying is probably that all errors must go through
>>> SQLGetDiagRec rather than dblib's error handling, which is what
>>> necessitated so much copy-and-paste. A future task is to look at how
>>> this could be rationalized.
>
>> Yes, coy and paste is not that great. I'm changing actually bcp.c
>> (dblib) to use tds_convert instead of dbconvert. This could help
>> moving code to libTDS (the common part) so we can reuse it. I remember
>> was a nightmare extracting code from either dblib and ctlib in libTDS
>> I don't want to do it again for dblib/odbc :)
>
> 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.

Any temporary hacky solution is fine. Copy/paste, including bcp.c from
odbc and redefining some functions or whatever is also fine.

> 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 ?

>> Well... probably using tdserror instead of dbperror could be a way.
>> libTDS use callbacks and even for normal errors from network if calls
>> these callbacks. In odbc these callbacks store errors in
>> statement/connection to be retrieved by SQLGetDiagRec. I noted that
>> bcp.c (used objdump) calls very few dblib functions (dbconvert,
>> dbvarylen, dbperror and another I don't remember now) so I don't think
>> will be much effort to move to libTDS. In bulk.c or another file.
>
> Again, if you can divide up the work and tell me which bits you're
> working on and which bits I should do then I can get started with a
> minimum of merge conflicts. We might even get file I/O out of it this
> way (although those BCPFILECP and BCPUNICODEFILE options look scary).
> Remember to keep a browser tab with the API differences [1] open while
> looking at what code can/should be shared.
>

I'm looking at bcp code from different point, not to move it to
libTDS. I'm closing some issue I want for a new release (see
https://trello.com/b/bk0UZNRJ/freetds-todo-list) but we can implement
bcp in another branch and having a new X.Y.Z version if needed.

<diversion>
Craig sent me a patch for a problem in bcp he was having. I was fixing
some issues and extending our tests while I found some issues with
bcp. One problem is that bcp code assume that it can preallocate all
buffers for data and use them. This assumption was removed many time
ago from all code but still present in bcp. The problem is that with
blobs server could say "oh, this column can be 2gb" while columns
contains only small data. For this reason libTDS does not care about
column_size for blobs and allocate data buffers as needed when it
receive data. I changed all my configuration to use 500mb as textsize
and I wrote a small shared object to limit allocations to 128mb. bcp
tests are now failing. The other issue with conversion is that
dbconvert either pad data to buffer size when you give a valid buffer
size (destlen >= 0) or you can avoid padding passing a negative buffer
size. So if you have destlen >= 0 you can have dbconvert filling large
quantities of ram while with destlen <0 you can have buffer overflows.
Using tds_convert avoid such problems and dbconvert is mainly a
wrapper around tds_convert. Also tds_convert already can allocate
buffer as needed (and actually is the default used by dbconvert).
</diversion>

What do you mean by BCPFILECP and BCPUNICODEFILE ? Supporting input
from wide characters encoded files ?

>>> 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.

> Richard.
>
>>> [1] https://msdn.microsoft.com/en-us/library/ms130924.aspx
>

Frediano