<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="8" style="border: 1px #c9c399 solid;">
<tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="https://reviewboard.asterisk.org/r/3335/">https://reviewboard.asterisk.org/r/3335/</a>
</td>
</tr>
</table>
<br />
<div>
<table width="100%" border="0" bgcolor="white" style="border: 1px solid #C0C0C0; border-collapse: collapse; margin: 2px padding: 2px;">
<thead>
<tr>
<th colspan="4" bgcolor="#F0F0F0" style="border-bottom: 1px solid #C0C0C0; font-size: 9pt; padding: 4px 8px; text-align: left;">
<a href="https://reviewboard.asterisk.org/r/3335/diff/4/?file=56006#file56006line493" style="color: black; font-weight: bold; text-decoration: underline;">http://svn.asterisk.org/svn/asterisk/branches/12/res/res_config_odbc.c</a>
<span style="font-weight: normal;">
(Diff revision 4)
</span>
</th>
</tr>
</thead>
<tbody style="background-color: #e4d9cb; padding: 4px 8px; text-align: center;">
<tr>
<td colspan="4"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">static int update_odbc(const char *database, const char *table, const char *keyfield, const char *lookup, const struct ast_variable *fields)</pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#e9eaa8" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">493</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="tb"> </span><span class="tb"> </span><span class="tb"> </span><span class="tb"> </span><span class="n">snprintf</span><span class="p">(</span><span class="n">sql</span> <span class="o">+</span> <span class="n">strlen</span><span class="p">(</span><span class="n">sql</span><span class="p">),</span> <span class="k">sizeof</span><span class="p">(</span><span class="n">sql</span><span class="p">)</span> <span class="o">-</span> <span class="n">strlen</span><span class="p">(</span><span class="n">sql</span><span class="p">),</span> <span class="s"><span class="hl">",</span> %s=NULL"</span><span class="p">,</span> <span class="n">field</span><span class="o">-></span><span class="n">name</span><span class="p">);</span></pre></td>
<th bgcolor="#e9eaa8" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">493</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="tb"> </span><span class="tb"> </span><span class="tb"> </span><span class="tb"> </span><span class="n">snprintf</span><span class="p">(</span><span class="n">sql</span> <span class="o">+</span> <span class="n">strlen</span><span class="p">(</span><span class="n">sql</span><span class="p">),</span> <span class="k">sizeof</span><span class="p">(</span><span class="n">sql</span><span class="p">)</span> <span class="o">-</span> <span class="n">strlen</span><span class="p">(</span><span class="n">sql</span><span class="p">),</span> <span class="n"><span class="hl">paramcount</span></span><span class="o"><span class="hl">++</span></span><span class="hl"> </span><span class="o"><span class="hl">?</span></span><span class="hl"> </span><span class="s"><span class="hl">", %s=NULL"</span></span><span class="hl"> </span><span class="o"><span class="hl">:</span></span><span class="hl"> </span><span class="s"><span class="hl">"</span> %s=NULL"</span><span class="p">,</span> <span class="n">field</span><span class="o">-></span><span class="n">name</span><span class="p">);</span></pre></td>
</tr>
</tbody>
</table>
<pre style="margin-left: 2em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">See the other review: two separate paramcount++'s smells.</pre>
</div>
<br />
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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.</pre>
<p>- wdoekes</p>
<br />
<p>On March 14th, 2014, 4:32 p.m. UTC, zvision wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-image: url('https://reviewboard.asterisk.org/static/rb/images/review_request_box_top_bg.ab6f3b1072c9.png'); background-position: left top; background-repeat: repeat-x; border: 1px black solid;">
<tr>
<td>
<div>Review request for Asterisk Developers.</div>
<div>By zvision.</div>
<p style="color: grey;"><i>Updated March 14, 2014, 4:32 p.m.</i></p>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Bugs: </b>
<a href="https://issues.asterisk.org/jira/browse/ASTERISK-23459">ASTERISK-23459</a>
</div>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
Asterisk
</div>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Description </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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.</pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Testing </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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.</pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> </h1>
<ul style="margin-left: 3em; padding-left: 0;">
<li>http://svn.asterisk.org/svn/asterisk/branches/12/res/res_config_odbc.c <span style="color: grey">(410554)</span></li>
</ul>
<p><a href="https://reviewboard.asterisk.org/r/3335/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>