Re: [WIP] RE: DECLARE STATEMENT setting up a connectionin ECPG - Mailing list pgsql-hackers

Thank you very much for thorough review and sorry for late replay.
Attched is 002_declareStmt_ecpglib_v7.patch and I haven't revised doc patch yet.

>002_declareStmt_ecpglib_v5.patch:

>+ 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.

Fixed.

>+ con = ecpg_get_connection(connection_name);
>+ if (!con)
>+ {
>+ return;
>+ }
>
>No need of {}.

Fixed.

>+ 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)

I remove the above check because we check the duplication when parsing ECPGCursorStmt token at ecpg.trailer and in the
existingmaster code we also check the cursor name duplication only when pre-compilineg pgc code.
 

Regarding similar codes, I added ecpg_raise() assertion.

>+ 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.

Removed ecpg_init() and added checking if con exists or not.

>-#endif   /* _ECPG_LIB_EXTERN_H */
>+#endif   /* _ECPG_LIB_EXTERN_H */
>
>Not related change.

Fixed.

>+ * 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"

Fixed.

>+ struct declared_statement *cur = NULL;
>+ struct declared_statement *prev = NULL;
>+ struct declared_statement *next = NULL;
>
>The above logic can be written without "next" pointer.
>That way it should be simpler.

Fixed.

>-#endif   /* _ECPGTYPE_H */
>+#endif /* _ECPGTYPE_H */
>
>Not related change.

Fixed.


>+ 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?

As you pointed out, the above thing should be discussed.
The current implementation is as follows:

ECPG pre-processor allows the DECLARE STATEMENT without using AT clause.
And the following statement after DECLARE STATEMENT such as PREPARE, EXECUTE are 
executed as usual on the current connection.

But there's a limitation here.
 (This limitation should be disccused earlier and be specified in the doc...
  but I didn't realize this clearly by myself, sorry)

When using DECLARE STATEMENT without AT clause
and using OPEN statement with AT clause, it doesn't work.

There's an example where you cannot fetch rows from db:
    EXEC SQL CONNECT TO db AS con;

    EXEC SQL DECLARE stmt STATEMENT;
    EXEC SQL AT con PREPARE stmt FROM :selectString;
    EXEC SQL AT con DECLARE cur CURSOR FOR stmt;
    EXEC SQL AT con OPEN cur;
         ...
    
This limitation looks troublesome for users,
so maybe I need to fix this implementation. 

regards,
Ideriha Takeshi