[Asterisk-code-review] cel pgsql: Fix schema query for get columns name (asterisk[master])

[Matt Jordan]

Matt Jordan has posted comments on this change.

Change subject: cel_pgsql: Fix schema query for get columns name
......................................................................


Patch Set 3: Code-Review-1

(4 comments)

https://gerrit.asterisk.org/#/c/120/3//COMMIT_MSG
Commit Message:

Line 7: cel_pgsql: Fix schema query for get columns name
      : 
      : This a port commit of 2d60b755 for cdr/cdr_pgsql.c
This patch is no longer a simple port of 2d60b755. It should describe what the patch is doing, particularly with its support of particular Postgres features.


https://gerrit.asterisk.org/#/c/120/3/cel/cel_pgsql.c
File cel/cel_pgsql.c:

Line 551: 		if (version >= 70300) {
I'm not a big fan of magic numbers. I'd prefer to see this as a #define at the top of the file that describes what the constant is.


Line 556: 				tmp_tablename = strchr(tmp_schemaname, '.');
The previous code used strrchr to strip the schema name from the table:

if ((tableptr = strrchr(table, '.'))) {
...

That will remove everything to the right of the _last_ occurrence. Thus, if the tablename/schema was:

foo.bar.yack

The code would previously set the table name to:

foo.bar

With your patch, it will now be:

table => foo
schema => bar.yack

I think you should keep the original logic of setting the schema to be the everything to the right of the last '.' in the table name.


Line 568: 			snprintf(sqlcmd, sizeof(sqlcmd), "SELECT a.attname, t.typname, a.attlen, a.attnotnull, d.adsrc, a.atttypmod FROM (((pg_catalog.pg_class c INNER JOIN pg_catalog.pg_namespace n ON n.oid = c.relnamespace AND c.relname = '%s' AND n.nspname = %s%s%s) INNER JOIN pg_catalog.pg_attribute a ON (NOT a.attisdropped) AND a.attnum > 0 AND a.attrelid = c.oid) INNER JOIN pg_catalog.pg_type t ON t.oid = a.atttypid) LEFT OUTER JOIN pg_attrdef d ON a.atthasdef AND d.adrelid = a.attrelid AND d.adnum = a.attnum ORDER BY n.nspname, c.relname, attnum",
        : 				tablename,
The single string is a little too long. Coding guidelines mandate a 90 column width. While we don't always obey that, I think this one might be a tad excessive.

You can always wrap a string in C as such:

"SELECT .... "
" ......     "
" ......     ",
tablename,

etc.


-- 
To view, visit https://gerrit.asterisk.org/120
To unsubscribe, visit https://gerrit.asterisk.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I691fd2cbc277fcba10e615f5884f8de5d8152f2c
Gerrit-PatchSet: 3
Gerrit-Project: asterisk
Gerrit-Branch: master
Gerrit-Owner: Rodrigo Ramirez Norambuena <decipher.hk at gmail.com>
Gerrit-Reviewer: Mark Michelson <mmichelson at digium.com>
Gerrit-Reviewer: Matt Jordan <mjordan at digium.com>
Gerrit-Reviewer: Rodrigo Ramirez Norambuena <decipher.hk at gmail.com>
Gerrit-HasComments: Yes