Re: MS query notifications patch

2014-05-14 19:24 GMT+01:00 Richard Hughes <[email protected]>:
> On 2014-05-10 at 09:55, Frediano Ziglio wrote:
>> 2014-04-23 18:50 GMT+01:00 Richard Hughes <[email protected]>:
>>> On 2014-04-18 at 09:39, Frediano Ziglio wrote:
>>>> 2014-04-16 19:12 GMT+01:00 Richard Hughes <[email protected]>:
>>>>> I needed support for SQL Server 2005's Query Notifications feature, so
>>>>> here's a patch against tip.
>
>> Sorry for late reply. I was on holiday and actually I'm without my own computer.
>
> This entire thread seems to be plagued with late replies - work got
> busy for me :-)
>

Let see if this time I'm more quick. I have a test machine. Actually I
committed to my master branch and launched the test. If there are no
regression I'll commit. Just a request. Which mail address (your) can
I use for the commit and which description for the commit do you want.
The feature is really useful!

Actually is just:

"Add support for MS query notifications"

a bit too few considering the importance can have.

>> 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.
>
> I had ruled out that option because I didn't think I was allowed to
> change the TDS API. If that breakage is allowed then absolutely adding
> a new parameter is cleaner.
>
> This version of the patch does, however, undo the splitting-out of
> tds7. I find it difficult to manage and review patches (even my own)
> which combine cosmetic changes with functional changes. Once we're
> happy with the functional stuff then either you or I can do the split
> as a separate task.
>
> ...
>> It make sense but perhaps here would
>> be more sensible to add a tds_start_query_head (or similar) function.
>
> Done.
>
>>>>> @@ -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.
>
> If it's inconvenient for you, I can get all the installation media for
> 2000 - I just need to find the time to spin up a virtual machine and
> get it running and tested.
>
> Richard.

My testbox has an msde (free mssql 2000) I can use.

I just need to code a new test. Oh.. if you have some code to test
with is more than welcome. I try adding tests for all feature we have.

Frediano