[asterisk-bugs] [JIRA] Issue Comment Edited: (ASTERISK-20400) Let ODBC store multiple formats

[Matt Jordan (JIRA)]

    [ https://issues.asterisk.org/jira/browse/ASTERISK-20400?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=197055#comment-197055 ] 

Matt Jordan edited comment on ASTERISK-20400 at 9/14/12 8:28 AM:
-----------------------------------------------------------------

First, discussions about code are better had on the asterisk-dev mailing list.  Discussions on JIRA issues for improvements to Asterisk typically occur when there is a patch associated with the improvement.  You will almost certainly get more involvement in this discussion if you take it to the mailing list.

Second, we do not allow JIRA issues for improvements or new features to remain open when there is no patch associated with them.  If we did, we'd end up with thousands of requests for changes in behavior or new features with nothing for developers to act on.  If you'd like to contribute a patch that'd be fantastic, but until that time this issue will be closed.  As soon as you do have a patch, please contact a bug marshal in #asterisk-bugs or on the asterisk-bugs/asterisk-dev mailing list and we'd be happy to reopen this issue.

Finally, some answers to your questions:
# ODBC voicemail not storing multiple attachments is a long known and well understood limitation.  There are lots of ways this limitation could be addressed, all of which have limitations as well - so "best" may not be the appropriate way to describe any solution.  The real question should be: what all functionality should an ODBC backend support?  Keep in mind some of the following issues when thinking of a solution:
* Multiple storage types would require more complex queries to the database.
* Multiple storage types would require significantly higher storage requirements.
* Multiple storage types may not be necessary if Asterisk supports a media container format, e.g., Matroska.
# Drastic changes to the schema is *highly* discouraged.  Making the upgrade path from one version of Asterisk to another very difficult should always be done as a last resort, and major changes should only be done in non-LTS versions.  A small schema changes was made in Asterisk 11, and we spent a lot of time trying to not make that change.  In the end we had to, but - apart from adding the new column to the voicemail tables - an administrator does not have to manipulate the data in any way.  Asterisk actually will 'upgrade' the voicemail information when it detects that it has data missing the new column's value.  Any change to the schema should attempt to make the upgrade process as painless as possible.

So, to answer your specific questions, my personal opinion is:
1. In a non-LTS version, pick the option that has the best schema for a relational database, which seems to be option (c).  Provide a script to convert the database from one version of Asterisk to the next.
1.1. Extremely important.  I do not think a patch like this would be appropriate for an LTS release, nor do I think it would be appropriate unless the patch included utilities to aid the upgrade process and strong defensive programming in app_voicemail to prevent errors in data conversion from impacting the system.
2. The entire system of macros is itself fundamentally flawed, but that's a different patch.  If you want to 'optimize' this operation (although I wouldn't recommend optimizing something until you know the impact of your operation), you can always add parameters to the RETRIEVE macro, such as the channel that the message is being retrieved for.  That way you can inspect the supported formats and retrieve only those formats that the channel supports.

Again, if you are serious about writing a patch to support than by all means please do so, but further discussion about this should take place in the asterisk-dev mailing list or the #asterisk-dev channel.

      was (Author: mjordan):
    First, discussions about code are better had on the asterisk-dev mailing list.  Discussions on JIRA issues for improvements to Asterisk typically occur when there is a patch associated with the improvement.  You will almost certainly get more involvement in this discussion if you take it to the mailing list.

Second, we do not allow JIRA issues for improvements or new features to remain open when there is no patch associated with them.  If we did, we'd end up with thousands of requests for changes in behavior or new features with nothing for developers to act on.  If you'd like to contribute a patch that'd be fantastic, but until that time this issue will be closed.  As soon as you do have a patch, please contact a bug marshal in #asterisk-bugs or on the asterisk-bugs/asterisk-dev mailing list and we'd be happy to reopen this issue.

Finally, some answers to your questions:
# ODBC voicemail not storing multiple attachments is a long known and well understood limitation.  There are lots of ways this limitation could be addressed, all of which have limitations as well - so "best" may not be the appropriate way to describe any solution.  The real question should be: what all functionality should an ODBC backend support?  Keep in mind some of the following issues when thinking of a solution:
* Multiple storage types would require more complex queries to the database.
* Multiple storage types would require significantly higher storage requirements.
* Multiple storage types may not be necessary if Asterisk supports a media container format, e.g., Matroska.

# Drastic changes to the schema is *highly* discouraged.  Making the upgrade path from one version of Asterisk to another very difficult should always be done as a last resort, and major changes should only be done in non-LTS versions.  A small schema changes was made in Asterisk 11, and we spent a lot of time trying to not make that change.  In the end we had to, but - apart from adding the new column to the voicemail tables - an administrator does not have to manipulate the data in any way.  Asterisk actually will 'upgrade' the voicemail information when it detects that it has data missing the new column's value.  Any change to the schema should attempt to make the upgrade process as painless as possible.

So, to answer your specific questions, my personal opinion is:
1. In a non-LTS version, pick the option that has the best schema for a relational database, which seems to be option (c).  Provide a script to convert the database from one version of Asterisk to the next.
1.1. Extremely important.  I do not think a patch like this would be appropriate for an LTS release, nor do I think it would be appropriate unless the patch included utilities to aid the upgrade process and strong defensive programming in app_voicemail to prevent errors in data conversion from impacting the system.
2. The entire system of macros is itself fundamentally flawed, but that's a different patch.  If you want to 'optimize' this operation (although I wouldn't recommend optimizing something until you know the impact of your operation), you can always add parameters to the RETRIEVE macro, such as the channel that the message is being retrieved for.  That way you can inspect the supported formats and retrieve only those formats that the channel supports.

Again, if you are serious about writing a patch to support than by all means please do so, but further discussion about this should take place in the asterisk-dev mailing list or the #asterisk-dev channel.
  
> Let ODBC store multiple formats
> -------------------------------
>
>                 Key: ASTERISK-20400
>                 URL: https://issues.asterisk.org/jira/browse/ASTERISK-20400
>             Project: Asterisk
>          Issue Type: Improvement
>      Security Level: None
>          Components: Applications/app_voicemail/ODBC
>            Reporter: David Laban
>
> http://ofps.oreilly.com/titles/9780596517342/asterisk-DB.html notes that ODBC only supports one voicemail format, but it is not noted in the example configs or anywhere else, including the code (!).
> The problem that I am *actually* trying to solve is that the default wav49[*] format is not supported by android, and I am not prepared to send raw PCM over a GSM connection. The only other common codec between asterisk's app_voicemail and android (ogg vorbis) is not supported by iPhone. The ovbious solution therefore is to store multiple formats in the database. 
> [*] I assume that wav49 is the default for a good reason (winxp compatibility?)
> I'm in the process of speccing up my changes now, and estimating how much work it will be. I will use this bug to track my progress. If anyone has any suggestions on the storage schema, please pipe up now. The voicemail_messages table has the following schema:
> Table: voicemail_messages
> Create Table: CREATE TABLE `voicemail_messages` (
>   `dir` char(255) NOT NULL default '',
>   `msgnum` int(4) NOT NULL default '0',
>   `context` char(80) default NULL,
>   `macrocontext` char(80) default NULL,
>   `callerid` char(80) default NULL,
>   `origtime` int(11) default NULL,
>   `duration` int(11) default NULL,
>   `recording` blob,
>   `flag` char(30) default NULL,
>   `category` char(30) default NULL,
>   `mailboxuser` char(30) default NULL,
>   `mailboxcontext` char(30) default NULL,
>   PRIMARY KEY  (`dir`,`msgnum`)
> ) ENGINE=MyISAM DEFAULT CHARSET=latin1
> I draw your attention to: PRIMARY KEY ('dir', 'msgnum'). 'dir' is typically something like:
> /var/spool/asterisk/voicemail/default/1234/INBOX
> and 'msgnum' is an integer.
> This gives me the choice of:
> a) Insert the parallel formats as something like:
> 'msgnum': ogg/var/spool/asterisk/voicemail/default/1234/INBOX
> b) Create a parallel table with PRIMARY KEY (`dir`, `msgnum`, `format`), but keep the existing voicemail_messages as the primary table.
> c) Break backwards compatibility by removing the `recording` blob from the table and inserting a reference field that you can look up in a table that looks something like:
> (
> Table: voicemail_messages_data
> Create Table: CREATE TABLE `voicemail_messages` (
>   `format` char(10) NOT NULL default '',
>   `recording_id` int(4) NOT NULL default '0',
>   `data` blob,
>   `refcount` int(4) default '1',
> PRIMARY KEY (`recording_id`, `format`)
> )
> d) Give up, and try a different tactic.
> Note that any changes I make will involve keeping the various formats in sync using:
> #ifdef ODBC_STORAGE
> static char odbc_database[80];
> static char odbc_table[80];
> #define RETRIEVE(a,b,c,d) retrieve_file(a,b)
> #define DISPOSE(a,b) remove_file(a,b)
> #define STORE(a,b,c,d,e,f,g,h,i,j) store_file(a,b,c,d)
> #define EXISTS(a,b,c,d) (message_exists(a,b))
> #define RENAME(a,b,c,d,e,f,g,h) (rename_file(a,b,c,d,e,f))
> #define COPY(a,b,c,d,e,f,g,h) (copy_file(a,b,c,d,e,f))
> #define DELETE(a,b,c,d) (delete_file(a,b))
> 1. Which approach is best? c) would probably be less work, because you only need to update recording_id.
> 1.1. How much do we care about backwards compatibility of the DB schema?
> 2. How much can I optimise if I make RETRIEVE etc. parse the fmt argument? 
> (A naive implementation of RETREIVE would cause a pull of all formats from the server to local storage, even if app_voicemail already knows which format is best for the channel.)
> Comments from someone who has experience hacking app_voicemail would be greatly appreciated.
> David.

--
This message is automatically generated by JIRA.
For more information on JIRA, see: http://www.atlassian.com/software/jira