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

[zvision]

On March 17, 2014, 9:28 a.m., zvision wrote:
> > 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.
> 
> zvision wrote:
>     I have uploaded a patch for trunk version. Please see my comments attached to the patch.
>     
>     Another option for non-trunk versions would be to introduce some config variable to avoid the check
>     on every connnect. But I guess it is not an elegant solution anyway.
> 
> zvision wrote:
>     Actually, having a config option in res_odbc (something like backslash_is_escape) may be not so bad:
>     - by default it would be disabled, not changing the current behavior,
>     - enabling it would give a chance to PostgreSQL (and maybe other RDBMSes) users to run realtime SIP peers with no problems
>       (I know they could have defined columns as text, but sample configurations provided use integers),
>     - a SELECT CAST check is not neccessary - it will generate errors in database logs which may be confusing
>       and may have other side effect we are not aware of.
> 
> wdoekes wrote:
>     I'm down with a patch that adds an option for that.
> 
> zvision wrote:
>     Great, I will prepare a patch and submit it with another review request.
>     Any votes for the option name? Maybe:
>     nullable_columns,
>     null_support,
>     enable_null,
>     enable_null_values?
> 
> wdoekes wrote:
>     I do have an opinion about that.
>     
>     Since MySQL is usually the odd one here, the option should reflect that.
>     
>     Something like this:
>     
>       allow_empty_string_in_nontext = yes  ; mysql compatability default, set to no for more strict databases
>     
>     That should hopefully not give someone ideas that full NULL-support is now an option.
> 
> wdoekes wrote:
>     *compatibility

https://reviewboard.asterisk.org/r/3375/ has patches for non-trunk versions


- zvision


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


On March 18, 2014, 10:42 p.m., zvision wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/3335/
> -----------------------------------------------------------
> 
> (Updated March 18, 2014, 10:42 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/trunk/res/res_config_odbc.c 410663 
> 
> 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/20140318/f1dde89e/attachment.html>