Re: [WIP] RE: [HACKERS] DECLARE STATEMENT setting up a connection in ECPG - Mailing list pgsql-hackers
| From | Haribabu Kommi |
|---|---|
| Subject | Re: [WIP] RE: [HACKERS] DECLARE STATEMENT setting up a connection in ECPG |
| Date | |
| Msg-id | CAJrrPGf9287AAdx6UJhtM75fNLc4Kvi8CHQhzoRFjnZf-8TGVg@mail.gmail.com Whole thread Raw |
| In response to | Re: [WIP] RE: [HACKERS] DECLARE STATEMENT setting up a connection in ECPG (Haribabu Kommi <kommi.haribabu@gmail.com>) |
| List | pgsql-hackers |
On Wed, Mar 22, 2017 at 12:51 PM, Haribabu Kommi <kommi.haribabu@gmail.com> wrote:
On Tue, Mar 7, 2017 at 4:09 PM, Ideriha, Takeshi <ideriha.takeshi@jp.fujitsu.com> wrote:
Attached 004_declareStmt_test_v5.patch is a rebased one.
The rest of patches are same as older version.Thanks for the update patch. I started reviewing the patches.There was a minor conflict in applying 004_declareXX patch.Some comments in 001_declareStmt_preproc_v5.patch: + if (INFORMIX_MODE)+ {+ if (pg_strcasecmp(stmt+strlen("close "), "database") == 0) + {+ if (connection)+ mmerror(PARSE_ERROR, ET_ERROR, "AT option not allowed in CLOSE DATABASE statement");++ fprintf(base_yyout, "{ ECPGdisconnect(__LINE__, \"CURRENT\");");+ whenever_action(2);+ free(stmt);+ break;+ }+ }The same code block is present in the stmtClosePortalStmt condition to verify the closestatement. Because of the above code addition, the code present in stmtClosePortalStmtis a dead code. So remove the code present in stmtClosePortalStmt or try to reuse it.+static void+output_cursor_name(char *str)This function needs some comments to explain the code flow for better understanding.+/*+ * Translate the EXEC SQL DECLARE STATEMENT into ECPGdeclare function+ */How about using Transform instead of Translate? (similar references also)
+ struct connection *f = NULL;
+
ecpg_init_sqlca(sqlca);
for (con = all_connections; con;)
{
- struct connection *f = con;
+ f = con;
What is the need of moving the structure declartion
to outside, i didn't find any usage of it later.
+ con = ecpg_get_connection(connection_name);
+ if (!con)
+ {
+ return;
+ }
No need of {}.
+ if (find_cursor(cursor_name, con))
+ {
+ /*
+ * Should never goto here, because ECPG has checked the duplication of
+ * the cursor in pre-compile stage.
+ */
+ return;
+ }
Do we really need this check? If it is for additional check, How about checking
it with an Assert? (check for similar instances)
+ if (!ecpg_init(con, real_connection_name, line))
+ return false;
What is the need of ecpg_init call? why the same is not done in other functions.
Better if you provide some comments about the need of the function call.
-#endif /* _ECPG_LIB_EXTERN_H */
+#endif /* _ECPG_LIB_EXTERN_H */
Not related change.
+ if(connection_name == NULL)
+ {
+ /*
+ * Going to here means not using AT clause in the DECLARE STATEMENT
+ * We don't allocate a node to store the declared name because the
+ * DECLARE STATEMENT without using AT clause will be ignored.
+ */
+ return true;
+ }
I am not sure that just ignore the declare statement may be wrong.
I feel whether such case is possible? Does the preprocessor allows it?
+ * Find the declared name referred by the cursor,
+ * then return the connection name used by the declared name.
How about rewriting the above statement as follows? This is because
we are not finding the declare name, as we are looping through all
the declare statements for this cursor.
"Find the connection name by referring the declared statements
cursors by using the provided cursor name"
+ struct declared_statement *cur = NULL;
+ struct declared_statement *prev = NULL;
+ struct declared_statement *next = NULL;
+
+ if(connection_name == NULL)
+ return;
+
+ cur = g_declared_list;
+ while(cur)
+ {
+ next = cur->next;
+ if (strcmp(cur->connection_name, connection_name) == 0)
+ {
+ /* If find then release the declared node from the list */
+ if(prev)
+ prev->next = next;
+ else
+ g_declared_list = next;
+
+ ecpg_log("ecpg_release_declared_statement: declared name %s is released\n", cur->name);
+
+ ecpg_free(cur->name);
+ ecpg_free(cur->connection_name);
+ ecpg_free(cur->cursor_name);
+ ecpg_free(cur);
+
+ /* One connection can be used by multiple declared name, so no break here */
+ }
+ else
+ prev = cur;
+
+ cur = next;
+ }
The above logic can be written without "next" pointer.
That way it should be simpler.
-#endif /* _ECPGTYPE_H */
+#endif /* _ECPGTYPE_H */
Not related change.
Regards,
Hari Babu
Fujitsu Australia
pgsql-hackers by date: