[asterisk-dev] [Code Review] 3346: [res_config_pgsql] Correct handling of nullable int fields in update_realtime

[zvision]

> On March 14, 2014, 8:49 p.m., Tilghman Lesher wrote:
> > One issue is that this patch is against a released version, Asterisk 11, and not against trunk.  Since we've never previously supported NULL columns as such, I think we'd want this change in trunk only.
> > 
> > Also, while you've changed the update function, we'd also want the update2 function changed at the same time, since it has similar, though more versatile, functionality.

I see your point. The fix has been made to address the ASTERISK-23351 issue. It is based on a patch which I proposed for res_config_odbc.
While res_config_pgsql has no support for (integer) null column, the res_config_odbc has code to handle this case, but due to a long present bug,
it does not work.
Please also see my comment on the integer columns support below your comment regarding this. I hope it will clarify a bit the purpose of this patch.


> On March 14, 2014, 8:49 p.m., Tilghman Lesher wrote:
> > http://svn.asterisk.org/svn/asterisk/branches/11/res/res_config_pgsql.c, lines 761-763
> > <https://reviewboard.asterisk.org/r/3346/diff/1/?file=55850#file55850line761>
> >
> >     This code appears to only work for integer types, not character types (char, varchar, bpchar, text) or even the numeric type.  I would think if we wanted support for NULL-able columns, we'd want to be able to support ALL column types, not just integers.

In general, it might be nice to have support for null columns, but I guess it would require in depth changes in many places.
This patch (and the patch for res_config_odbc) touches only integer columns for purpose. For text columns, it is nothing wrong
with settings them to an empty string, but PostgreSQL will not allow you to set an integer column to an empty string.
The real world use case are sippeers and sipregs realtime tables. When your port column type is an integer (which seems to be reasonable),
during endpoint deregistration Asterisk tries to store an empty string to this column, which obviously fails with PostgreSQL
(and works with MySQL).
Therefore this patch addresses this case only. It should not break anything, as setting an integer column to an empty string
in PostgreSQL will fail anyway. It just makes the module to handle more cases.


- zvision


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


On March 13, 2014, 10:06 a.m., zvision wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/3346/
> -----------------------------------------------------------
> 
> (Updated March 13, 2014, 10:06 a.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Bugs: ASTERISK-23351
>     https://issues.asterisk.org/jira/browse/ASTERISK-23351
> 
> 
> Repository: Asterisk
> 
> 
> Description
> -------
> 
> This patch fixes handling of nullable int columns in update_realtime function. It checks if a value is empty and sets the column to NULL instead of '', which raises an error.
> Additionally, it checks for existence of the keyfield column instead of the first parameter column.
> 
> 
> Diffs
> -----
> 
>   http://svn.asterisk.org/svn/asterisk/branches/11/res/res_config_pgsql.c 410508 
> 
> Diff: https://reviewboard.asterisk.org/r/3346/diff/
> 
> 
> Testing
> -------
> 
> Only tested for successful compilation. Someone needs to confirm that the patch works fine.
> 
> 
> Thanks,
> 
> zvision
> 
>

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