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

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.


- zvision


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


On March 17, 2014, 11:12 a.m., zvision wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/3335/
> -----------------------------------------------------------
> 
> (Updated March 17, 2014, 11:12 a.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/7f1841a9/attachment.html>