[Asterisk-code-review] res_config_pgsql: Permit more than one database and set parameters. (asterisk[master])

[Eric Dantie]

Attention is currently required from: Sean Bright, N A, Joshua Colp.

Eric Dantie has posted comments on this change. ( https://gerrit.asterisk.org/c/asterisk/+/19921 )

Change subject: res_config_pgsql: Permit more than one database and set parameters.
......................................................................


Patch Set 14: Code-Review+1

(11 comments)

Commit Message:

https://gerrit.asterisk.org/c/asterisk/+/19921/comment/3fbe10ae_16929b87 
PS3, Line 7: Add parameter dburl to res_config_pgsql to permit adding more than one database and parameters
> Still not quite there, something like this would be better: […]
Done


Commit Message:

https://gerrit.asterisk.org/c/asterisk/+/19921/comment/d233c24e_25883a2b 
PS10, Line 7: res/res_config_pgsql permit more than one database.
> Comma needed after res_config_pgsql. […]
Done


Patchset:

PS14: 
Thanks


File configs/samples/res_pgsql.conf.sample:

https://gerrit.asterisk.org/c/asterisk/+/19921/comment/180a5835_7e43457f 
PS3, Line 10: dburl=postgresql://127.0.0.1:5432/asterisk?target_session_attrs=any&application_name=asterisk
> Is this a valid configuration? Both dburl and dbhost are now specified in this sample configuration
It's the same as the default parameters with the error that target_session_attrs must be changed.

target_session_attrs=read-write

I've kept the others values for compatibility with older versions, but should it keep it that way or just using dburi?


File configs/samples/res_pgsql.conf.sample:

https://gerrit.asterisk.org/c/asterisk/+/19921/comment/0ba7bcf6_a44130f4 
PS10, Line 8: ;
> It should be specified here how the configuration works when dburi is specified and how it interacts […]
Ack


File configs/samples/res_pgsql.conf.sample:

https://gerrit.asterisk.org/c/asterisk/+/19921/comment/39e17f66_37bf4065 
PS11, Line 9: ; - With the other parameters dbhost, ...
> It's a little unclear where you're going with the ellipses here
Done


File res/res_config_pgsql.c:

https://gerrit.asterisk.org/c/asterisk/+/19921/comment/e29acb82_ec73aea3 
PS3, Line 58: #define MAX_URL_OPTION_SIZE 256
> Where does 256 come from?
It's a arbitrary value. But it should be long enough for example the dburl I use is: 
dburl=postgresql://asterisk:asterisk@lpa-pg-tfe.contactel.es:5432,tfe-pg-tfe.contactel.es:5432,mtp-pg-tfe.contactel.es:5432/pruebas?target_session_attrs=read-write

I don't known if there is a way to get the length dynamically


https://gerrit.asterisk.org/c/asterisk/+/19921/comment/9ee79ad6_479d12b7 
PS3, Line 1584: 		ast_debug(1, "pgsqlConn=%p\n", pgsqlConn);
> This should be removed, it doesn't really help.
You are right


File res/res_config_pgsql.c:

https://gerrit.asterisk.org/c/asterisk/+/19921/comment/76d80d66_2be1c535 
PS7, Line 1464: 		ast_log(LOG_DEBUG,
> Don't ast_log to LOG_DEBUG directly. Use the ast_debug macro.
Done


https://gerrit.asterisk.org/c/asterisk/+/19921/comment/8b7efa03_690b1fcf 
PS7, Line 1468: 
> Extra line is not needed here
Done


File res_config_pgsql.txt:

https://gerrit.asterisk.org/c/asterisk/+/19921/comment/2c85f983_5b71c5f9 
PS10, Line 1: Subject: Add support for multiples databases
> The subject for CHANGES entries should be the name of the module. […]
Done



-- 
To view, visit https://gerrit.asterisk.org/c/asterisk/+/19921
To unsubscribe, or for help writing mail filters, visit https://gerrit.asterisk.org/settings

Gerrit-Project: asterisk
Gerrit-Branch: master
Gerrit-Change-Id: I9ca8705c96acdd4a10989d981a5c2583a160aa44
Gerrit-Change-Number: 19921
Gerrit-PatchSet: 14
Gerrit-Owner: Eric Dantie <edantie at gmail.com>
Gerrit-Reviewer: Eric Dantie <edantie at gmail.com>
Gerrit-Reviewer: Friendly Automation
Gerrit-Reviewer: Joshua Colp <jcolp at sangoma.com>
Gerrit-Reviewer: N A <asterisk at phreaknet.org>
Gerrit-Reviewer: Sean Bright <sean at seanbright.com>
Gerrit-Attention: Sean Bright <sean at seanbright.com>
Gerrit-Attention: N A <asterisk at phreaknet.org>
Gerrit-Attention: Joshua Colp <jcolp at sangoma.com>
Gerrit-Comment-Date: Thu, 16 Mar 2023 11:42:50 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Comment-In-Reply-To: N A <asterisk at phreaknet.org>
Comment-In-Reply-To: Joshua Colp <jcolp at sangoma.com>
Gerrit-MessageType: comment
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20230316/ad411a63/attachment-0001.html>