[asterisk-dev] [Code Review]: Add SQLite 3 realtime driver

[Tilghman Lesher]

> On Sept. 2, 2011, 8:47 p.m., Tilghman Lesher wrote:
> > /trunk/res/res_config_sqlite3.c, line 371
> > <https://reviewboard.asterisk.org/r/1408/diff/2/?file=19934#file19934line371>
> >
> >     The people who deliberately order disallow/allow columns or permit/deny columns care.
> 
> Terry Wilson wrote:
>     This is callback function for a single row of results and it is just the reverse order of the columns in that row. We aren't guaranteed what order the columns in the database are presented, so altering their order in the variable list shouldn't matter, especially since it is a list of column names and values. Someone seeking a particular value will parse the list comparing on the column name.
> 
> Tilghman Lesher wrote:
>     Take a look at the SIP and IAX drivers, which parse the values that come back from realtime in the order in which they are presented.  I agree that it's a bad precedent that we have had to order columns in the table to match how they're dealt with in the driver; however, it is precedent nonetheless.  If you're going to break this assumption (that is true in every other realtime driver), then that needs to be clearly documented (on the Wiki, since there's really no inline documentation for realtime drivers) for this driver.
> 
> Terry Wilson wrote:
>     I looked at chan_sip/iax2 and I really don't see anywhere where the results from ast_load_realtime() are not parsed by doing a strcasecmp on the 'name' field. Since the select queries on every backend use "SELECT *" there is no way to tell what order these columns are going to come back. It often depends on what order they were actually created at CREATE TABLE time. If someone ever relied on that order, they deserve to fail. But it would be really hard to do so. Who is going to traverse a linked list, keeping a counter, and do something like "if (i != 3) continue;".  That just isn't how people use linked lists. If this was a multi-row query (which requires an ast_config) I would completely agree that order matters. On this, though, I just can't imagine a way where someone would be silly enough to go to the extra work it would require to make the code brittle. It isn't that hard to change, but inserting at the head is more efficient and as far as the code is concerned, column order in a SELECT * is undefined. We can not know what order the values were listed in CREATE TABLE. Warning someone about it in the wiki would be like warning someone that the random number produced by X is going to be different than the one produced by Y.

I think you're missing the point.  I'm not saying that drivers depend upon fields being positionally arranged.  I'm saying that people intentionally structure their tables with the field "disallow" previous to "allow", so they can do disallow=all; allow=ulaw,gsm.  If in your driver, suddenly the fields are returned in reverse order, the user will have all codecs disabled.  We don't currently allow prefixing codecs with a '!' to do a disallow (allow=all,!g729) in the allow column (and perhaps we should, to deal with this specific problem), but it's still precedent that means we have to honor the user's definition and present fields in the order in which they were defined.

Otherwise, what you're saying here is... all realtime drivers return columns in the order in which they are defined... except this one peculiar driver, which returns them in the reverse order.  If nothing else, it violates the principle of least surprise.


- Tilghman


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


On Sept. 2, 2011, 5:05 p.m., Terry Wilson wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/1408/
> -----------------------------------------------------------
> 
> (Updated Sept. 2, 2011, 5:05 p.m.)
> 
> 
> Review request for Asterisk Developers, Olle E Johansson and Tilghman Lesher.
> 
> 
> Summary
> -------
> 
> This patch adds an SQLite 3 realtime driver. It supports multiple databases, ast_realtime_require (warn, createclose, and createchar support including the ability to create missing tables), static realtime, and batching commits in transactions to increase write performance. It currently caches no table data, but I figure that premature optimization is bad anyway.
> 
> It, like other realtime drivers, doesn't escape data when building the SQL strings. I started out using parameter binding, but SQLite doesn't let you bind column names, so given how dynamic everything with realtime is, I just when with building the statements outright. I could manually escape the parameters, but the only thing in the SQLite 3 library I could find that does that dynamically allocates the string and that ends up being a lot of little allocs and frees. I suppose we could write our own using a single ast_str with some ast_str_reset calls between each one, but it is a bit of a pain. Until we want to open up the realtime api to things like AMI where user-generated input is going to be a problem, escaping isn't really necessary.
> 
> The impetus for writing this is that Asterisk now has a built-in SQLite 3 database which the astdb uses. The next step would be to convert the astdb to using the realtime calls, thus making it possible for it to be used with any realtime backend (but using the default SQLite 3 db unless configured otherwise). After that, we can start rewriting some things that use the astdb in very crude ways to start using realtime calls to the internal (or external, if configured that way) databases so that the data can be stored in more logical ways than a key/value store allows.
> 
> 
> Diffs
> -----
> 
>   /trunk/configs/res_config_sqlite3.conf.sample PRE-CREATION 
>   /trunk/res/res_config_sqlite3.c PRE-CREATION 
> 
> Diff: https://reviewboard.asterisk.org/r/1408/diff
> 
> 
> Testing
> -------
> 
> Verified that static realtime extensions.conf loads properly. Realtime CLI commands all behave as expected. Tested unload/load and reload scenarios making sure that they behaved as expected and that there were no ref/memory leaks. Made sure that batching worked.
> 
> Once we move the astdb to using realtime calls, the existing unit tests for that should give a workout to the realtime driver as well. I didn't write a new test pointing to the astdb because the astdb locks its database so it cannot be written to from another connection.
> 
> 
> Thanks,
> 
> Terry
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20110903/136c2884/attachment.htm>