[freetds] MS query notifications patch

Frediano Ziglio freddy77 at gmail.com
Sat May 10 04:55:12 EDT 2014


2014-04-23 18:50 GMT+01:00 Richard Hughes <cyreve at gmail.com>:
> On 2014-04-18 at 09:39, Frediano Ziglio wrote:
>> 2014-04-16 19:12 GMT+01:00 Richard Hughes <cyreve at gmail.com>:
>>> I needed support for SQL Server 2005's Query Notifications feature, so
>>> here's a patch against tip.
>>
>> The big concern is that I think that this feature works also for every
>> queries not for just sql ones. I'll add some comments on your code. I
>> think we should test what happens for prepared statements or RPCs.
>
> You're right. I was too stuck in my particular use-case to consider
> that RPC might be possible. I actually can't think of realistic design
> for an client application that would set up parameterized query
> notifications, however the server does support it so I've gone ahead
> and implemented it. Unfortunately this makes the patch a whole lot
> bigger, and a large part of my delay in getting back to you was spent
> changing my mind several times about the neatest way to squeeze it in.
> You may wish to read the diff with ignore-whitespace, because I ended
> up pulling a load of if statements out into their own functions.
>

Sorry for late reply. I was on holiday and actually I'm without my own computer.

I like the idea of splitting in query.c the code that handle tds7 from
other protocol however I would not expose all these definitions
outside it and instead all function calls should add the head
parameter.

This code

               if (!tds_dstr_isempty(&stmt->
attr.qn_msgtext) &&
!tds_dstr_isempty(&stmt->attr.qn_options)) {
                       TDSHEADERS head;
                       query_notification_init_headers(stmt, &head);
                       ret = tds7_submit_rpc(tds, name, stmt->params, &head);
               } else {
                       ret = tds_submit_rpc(tds, name, stmt->params);
               }

could then become something like

               TDSHEADERS head;
               ret = tds_submit_rpc(tds, name, stmt->params,
odbc_init_headers(stmt, &head));

where odbc_init_headers returns either &head or NULL based on values
stored on stmt.

Sometimes you test result from tds_start_query sometimes no, I think
we should always test it. The function mainly fails on memory problems
or string conversion. These condition should be quite critical so
perhaps would be better to set state to DEAD (which close the
connection) instead, unless we want to handle strange conversion
settings from the client. The rule you applied is: "if it has
parameters is it ok to not test". It make sense but perhaps here would
be more sensible to add a tds_start_query_head (or similar) function.
In this way we could add a compiler attribute (where supported) to say
we want the result tested while the normal tds_start_query could just
return void.

>>> @@ -228,6 +229,11 @@ prepare_call(struct _hstmt * stmt)
>>>      else
>>>          return SQL_ERROR;
>>>
>>> +    if (tds_dstr_isempty(&stmt->attr.qn_msgtext) != tds_dstr_isempty(&stmt->attr.qn_options))
>>> +        return SQL_SUCCESS_WITH_INFO;
>>> +    if (!tds_dstr_isempty(&stmt->attr.qn_msgtext) && !IS_TDS72_PLUS(stmt->dbc->tds_socket->conn))
>>> +        return SQL_SUCCESS_WITH_INFO;
>>> +
>>
>> I'm not sure this is the right place. Also I think MS add some error
>> to the statement. I didn't find from documentation which error should
>> be reported.
>
> Yes - the _WITH_INFO part should have been a clue that I needed to
> return some info. MS Native Client returns HY000 for the first case
> and still executes the query, but I don't have a pre-2005 server to
> find out what should happen in the second case so I just left it as a
> generic error.
>

I'll try with a former server.

> All your other comments have been swept up in to the big restructuring
> I needed to do to get RPC working.
>
> Richard.

Thanks for your patience.

Frediano


More information about the FreeTDS mailing list