Summary:	ASTERISK-23737: application_name support in cdr_pgsql and res_config_pgsql
Reporter:	Gergely Dömsödi (doome)	Labels:
Date Opened:	2014-05-13 06:52:17	Date Closed:	2014-07-16 08:57:26
Priority:	Minor	Regression?
Status:	Closed/Complete	Components:	CDR/cdr_pgsql Resources/res_config_pgsql
Versions:	1.8.27.0 11.9.0	Frequency of Occurrence
Related Issues:
Environment:		Attachments:	( 0) pgsql_application_name.patch
Description:	Postgresql supports application_name connection parameter to ease the identification of client processes. The attached patch makes use of this feature both in cdr_pgsql.c and res_config_pgsql.c. As a side effect, cdr_pgsql.c is updated to use the newer postrgesql connection mechanism which res_config_pgsql uses.
Comments:	By: Matt Jordan (mjordan) 2014-05-13 08:03:57.308-0500 The patch provided here has numerous coding guideline violations. Please review the coding guidelines on the Asterisk wiki [1] and re-attach the patch. Once the patch has been cleaned up, feel free to post it to Review Board for code review [2]. Thanks! [1] https://wiki.asterisk.org/wiki/display/AST/Coding+Guidelines [2] https://wiki.asterisk.org/wiki/display/AST/Review+Board+Usage By: Matt Jordan (mjordan) 2014-05-30 17:50:50.973-0500 While support for this feature would be nice for {{cdr_pgsql}} and {{res_config_pgsql}}, the patch here is not ready for inclusion or code review. [~doome]: If you decide to clean up the patch and would like it submitted for code review, please submit it to Review Board. This issue can be re-opened when that occurs. Thanks! By: Gergely Dömsödi (doome) 2014-06-06 01:34:04.869-0500 cleaned up the patch By: Gergely Dömsödi (doome) 2014-06-06 01:53:07.386-0500 Posted patch (against current trunk) to Review Board: https://reviewboard.asterisk.org/r/3591/ By: Walter Doekes (wdoekes) 2014-06-06 03:27:01.344-0500 Shouldn't it just be hardcoded to asterisk? And does this work on all postgres versions? Or do we need a fallback pgconnect without the appname? Secondly -- I couldn't view your reviewboard issue, because the site has issues -- I see in your patch here that the coding guidelines aren't followed: {noformat} + if (!ast_strlen_zero(pgpassword)) + ast_str_append(&connInfo, 0, " password=%s", pgpassword); {noformat} add braces for *all* if/while etc.. {noformat} strcpy(dbappname,""); {noformat} use a space after the comma {noformat} + //conn = PQsetdbLogin(pghostname, pgdbport, NULL, NULL, pgdbname, pgdbuser, pgpassword); {noformat} stray garbage (and illegal c++-style comments) https://wiki.asterisk.org/wiki/display/AST/Coding+Guidelines By: Gergely Dömsödi (doome) 2014-06-06 03:45:45.957-0500 [~wdoekes]: Have you looked at the patch I submitted today? That doesn't have the problems You mentioned I think. By the way, I think this shouldn't be hardcoded into asterisk code, because there could be cases when a user is running more asterisks on the same box, which connect to the same db. By looking at the application name, the two instances are easily recognizable. By: Gergely Dömsödi (doome) 2014-06-06 03:53:24.921-0500 According to Postgres documentation, application_name support was added in 9.0, which was released 2010-09-20. By: Walter Doekes (wdoekes) 2014-06-06 04:40:49.532-0500 {quote} Have you looked at the patch I submitted today? That doesn't have the problems You mentioned I think. {quote} Most of them are indeed fixed. {quote} According to Postgres documentation, application_name support was added in 9.0, which was released 2010-09-20. {quote} I see that not supplying the appname should avoid any issues with older versions anyway, so this question wasn't that relevant. I don't like the WARNING there though. By: Gergely Dömsödi (doome) 2014-06-06 05:22:52.409-0500 You're right, I removed the warning. Updated the patch on review board and here.