[asterisk-dev] [Code Review] 3335: [res_config_odbc] Fix for nullable integer columns and keyfield existence check in update_odbc

wdoekes reviewboard at asterisk.org
Mon Mar 17 04:28:21 CDT 2014


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/3335/#review11245
-----------------------------------------------------------



http://svn.asterisk.org/svn/asterisk/branches/12/res/res_config_odbc.c
<https://reviewboard.asterisk.org/r/3335/#comment20880>

    See the other review: two separate paramcount++'s smells.


So, the changes look good. But that regression thing is .. not so nice.

For trunk and up, we could change the behaviour. But we can't for 1.8..12.

The options are:

- leave the bug in place, only fix in trunk

- change the behaviour mid-release

- check which database we're using during connect-time


For that last option, we could use this:

    mysql> select cast('' as decimal) as num;
    +-----+
    | num |
    +-----+
    |   0 |
    +-----+

    postgres=> select cast('' as decimal) as num;
    ERROR:  invalid input syntax for type numeric: ""
    LINE 1: select cast('' as decimal) as num;

And hope that this extra query doesn't cause any regressions anywhere.

And then we'd have to choose between keeping this behaviour in trunk or "fixing" it to write NULLs when dealing with numeric types.


My vote:
- add the above check or any other which consistently declares how the database reacts
- apply this patch to trunk (removing the check again)

But a second opinion would be nice.

- wdoekes


On March 14, 2014, 4:32 p.m., zvision wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/3335/
> -----------------------------------------------------------
> 
> (Updated March 14, 2014, 4:32 p.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Bugs: ASTERISK-23459
>     https://issues.asterisk.org/jira/browse/ASTERISK-23459
> 
> 
> Repository: Asterisk
> 
> 
> Description
> -------
> 
> This patch fixes setting nullable integer columns to NULL instead of an empty string, which fails for PostgreSQL, for example.
> The current code is supposed to do so, but the check is broken. The patch also allows the first column in the list to be a nullable integer.
> Also, the check for existence of a mandatory column checked for the first column in the list instead of the key field lookup column.
> 
> 
> Diffs
> -----
> 
>   http://svn.asterisk.org/svn/asterisk/branches/12/res/res_config_odbc.c 410554 
> 
> Diff: https://reviewboard.asterisk.org/r/3335/diff/
> 
> 
> Testing
> -------
> 
> Tested by me. Use case scenario: Asterisk + res_odbc + PostgreSQL backend, SIP realtime peers + regs.
> When a 'port' column in SIP regs (I assume this also applies when using sippeers only) is a nullable integer,
> Asterisk tries to write an empty string here during SIP endpoint deregistration.
> 
> 
> Thanks,
> 
> zvision
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20140317/e76a76a5/attachment.html>


More information about the asterisk-dev mailing list