Re: [HACKERS] [WIP] RE: DECLARE STATEMENT setting up a connection in ECPG

[Daniel Gustafsson]

> On 30 Mar 2017, at 09:11, Ideriha, Takeshi  
> wrote:
> 
> Thank you for prompt check!
>  
> >As per above test steps, it doesn't produce the results and doesn't
> >generate the error also. I feel this needs to be fixed.
>  
> >As we are at the end of commitfest, it is better you can move it
> >to next one commitfest and provide an updated patch to solve the
> >above problem.
>  
> I tottaly agreed.
> I moved to next CF with waiting on author.

This patch was moved to the current commitfest (and to the previous one from
the 201701 CF).  Have you had the chance to address the review comments such
that there is an update expected within this CF?

cheers ./daniel

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [WIP] RE: DECLARE STATEMENT setting up a connection in ECPG

[Ideriha, Takeshi]

Thank you for prompt check!

>As per above test steps, it doesn't produce the results and doesn't
>generate the error also. I feel this needs to be fixed.

>As we are at the end of commitfest, it is better you can move it
>to next one commitfest and provide an updated patch to solve the
>above problem.

I tottaly agreed.
I moved to next CF with waiting on author.

Regards,
Ideriha Takeshi

Re: [HACKERS] [WIP] RE: DECLARE STATEMENT setting up a connection in ECPG

[Haribabu Kommi]

On Thu, Mar 30, 2017 at 1:57 PM, Ideriha, Takeshi <
ideriha.take...@jp.fujitsu.com> wrote:

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

As per above test steps, it doesn't produce the results and doesn't
generate the error also. I feel this needs to be fixed.

As we are at the end of commitfest, it is better you can move it
to next one commitfest and provide an updated patch to solve the
above problem.

Regards,
Hari Babu
Fujitsu Australia

Re: [HACKERS] [WIP] RE: DECLARE STATEMENT setting up a connection in ECPG

[Ideriha, Takeshi]

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 existing master 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


002_declareStmt_ecpglib_v7.patch
Description: 002_declareStmt_ecpglib_v7.patch

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [WIP] RE: DECLARE STATEMENT setting up a connection in ECPG

[David Steele]

Hi Takeshi,

On 3/23/17 1:33 AM, Haribabu Kommi wrote:


The test patch looks good to me.


This thread has been idle for five days.  Please respond with a new 
patch by 2017-03-30 00:00 AoE (UTC-12) or this submission will be marked 
"Returned with Feedback".


--
-David
da...@pgmasters.net


--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [WIP] RE: DECLARE STATEMENT setting up a connection in ECPG

[David Steele]

Hi Haribabu,

On 3/7/17 12:09 AM, Ideriha, Takeshi wrote:



I tried applying your patches. But it failed...
The error messages are as below.


Attached 004_declareStmt_test_v5.patch is a rebased one.
The rest of patches are same as older version.

Regards,
Ideriha, Takeshi


You are signed up to review this patch.  Do you know when you'll have a 
chance to do that?


Thanks,
--
-David
da...@pgmasters.net


--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers