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

Hi Ideriha-san,

I created a patch.

Thank you. Would it be possible for you to re-create the patch without
the white-space changes?

I marked [WIP] to the title because some documentation is lacked and
format needs some fixing.

I noticed that the docs say the statement is a PostgreSQL addon.
However, I think other databases do have the same statement, or don't
they? 

Michael
--
Michael Meskes
Michael at Fam-Meskes dot De, Michael at Meskes dot (De|Com|Net|Org)
Meskes at (Debian|Postgresql) dot Org
Jabber: michael at xmpp dot meskes dot org
VfL Borussia! Força Barça! SF 49ers! Use Debian GNU/Linux, PostgreSQL

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

Hi
Thank you for looking over my patch.

Thank you. Would it be possible for you to re-create the patch without the
white-space changes?

I'm sorry for adding redundant white-spaces.
Attached is a correct version.

I noticed that the docs say the statement is a PostgreSQL addon.
However, I think other databases do have the same statement, or don't they?

Yes, other databases such as Oracle and DB2 also have the same statement.
I fixed docs and mentioned the above databases in the compatibility section.

But I'm not sure whether I should mention the other databases explicitly
because the other command docs don't mention Oracle or so explicitly in compatibility section
as long as I read.

Regards,
Ideriha, Takeshi

On Fri, Jan 6, 2017 at 6:10 PM, Ideriha, Takeshi
<ideriha.takeshi@jp.fujitsu.com> wrote:

Hi
Thank you for looking over my patch.

Thank you. Would it be possible for you to re-create the patch without the
white-space changes?

I'm sorry for adding redundant white-spaces.
Attached is a correct version.

I noticed that the docs say the statement is a PostgreSQL addon.
However, I think other databases do have the same statement, or don't they?

Yes, other databases such as Oracle and DB2 also have the same statement.
I fixed docs and mentioned the above databases in the compatibility section.

But I'm not sure whether I should mention the other databases explicitly
because the other command docs don't mention Oracle or so explicitly in compatibility section
as long as I read.

Idehira-san, this is a very intrusive patch... It really needs a
specific reviewer to begin with, but really it would be nice if you
could review someone else's patch, with a difficulty rather equivalent
to what we have here.

By the way, I have been able to crash your patch when running the
regression tests:
(lldb) bt
* thread #1: tid = 0x0000, 0x00007fff89a828b0
libsystem_platform.dylib`_platform_strcmp + 176, stop reason = signal
SIGSTOP
* frame #0: 0x00007fff89a828b0 libsystem_platform.dylib`_platform_strcmp + 176
frame #1: 0x000000010c835bc3
libecpg.6.dylib`ecpg_release_declared_statement(connection_name="con3")
+ 83 at prepare.c:740
frame #2: 0x000000010c838103
libecpg.6.dylib`ECPGdisconnect(lineno=81, connection_name="ALL") + 179
at connect.c:697
frame #3: 0x000000010c811922 declare`main(argc=1,
argv=0x00007fff533ee320) + 434 at declare.pgc:81
frame #4: 0x00007fff932345ad libdyld.dylib`start + 1

You also need to add in src/interfaces/ecpg/test/sql/.gitignore new
entries related to the files you are adding and that get generated.
--
Michael

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

Idehira-san, this is a very intrusive patch... It really needs a specific
reviewer to begin with, but really it would be nice if you could review someone
else's patch, with a difficulty rather equivalent to what we have here.

Michael, thank you for taking a look at my patch and giving me an advice.
And sorry that I didn't follow a procedure of developing postgresql.
I think I should have sent a system design idea or small patch first and made it bigger and bigger step by step
instead of dumping a huge patch suddenly.

Yeah, I'm going to reviewing hackers' patches.

By the way, I have been able to crash your patch when running the regression
tests:

Thank you for checking and I'm going to handle this.

Regards,
Ideriha Takeshi

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

Hi,

I'm sorry but I think I don't have time to work on this patch in this CF.
I've gotten busy for another work.

So I moved this patch to next CF with "waiting on author" status.

Regards,
Ideriha Takeshi

-----Original Message-----
From: Ideriha, Takeshi
Sent: Friday, January 20, 2017 6:12 PM
To: 'Michael Paquier' <michael.paquier@gmail.com>
Cc: Michael Meskes <meskes@postgresql.org>; pgsql-hackers@postgresql.org
Subject: RE: [WIP] RE: [HACKERS] DECLARE STATEMENT setting up a connection in
ECPG

Idehira-san, this is a very intrusive patch... It really needs a
specific reviewer to begin with, but really it would be nice if you
could review someone else's patch, with a difficulty rather equivalent to

what we have here.

Michael, thank you for taking a look at my patch and giving me an advice.
And sorry that I didn't follow a procedure of developing postgresql.
I think I should have sent a system design idea or small patch first and made
it bigger and bigger step by step instead of dumping a huge patch suddenly.

Yeah, I'm going to reviewing hackers' patches.

By the way, I have been able to crash your patch when running the
regression
tests:

Thank you for checking and I'm going to handle this.

Regards,
Ideriha Takeshi

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

On Wed, Jan 25, 2017 at 2:58 PM, Ideriha, Takeshi
<ideriha.takeshi@jp.fujitsu.com> wrote:

Hi,

I'm sorry but I think I don't have time to work on this patch in this CF.
I've gotten busy for another work.

So I moved this patch to next CF with "waiting on author" status.

Thanks for doing so, that's a time-saver for me!
--
Michael

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

Hi

By the way, I have been able to crash your patch when running the regression

tests:

(lldb) bt

* thread #1: tid = 0x0000, 0x00007fff89a828b0

libsystem_platform.dylib`_platform_strcmp + 176, stop reason = signal SIGSTOP

* frame #0: 0x00007fff89a828b0 libsystem_platform.dylib`_platform_strcmp +

176

frame #1: 0x000000010c835bc3

libecpg.6.dylib`ecpg_release_declared_statement(connection_name="con3")

+ 83 at prepare.c:740

frame #2: 0x000000010c838103

libecpg.6.dylib`ECPGdisconnect(lineno=81, connection_name="ALL") + 179 at

connect.c:697

frame #3: 0x000000010c811922 declare`main(argc=1,

argv=0x00007fff533ee320) + 434 at declare.pgc:81

frame #4: 0x00007fff932345ad libdyld.dylib`start + 1

You also need to add in src/interfaces/ecpg/test/sql/.gitignore new entries

related to the files you are adding and that get generated.

Thank you very much for your test. I fixed this memory leak bug, and fixed .gitignore.

I also fixed some code style to fit coding conventions,

and splited my patch into 4 parts to improve readability:

* 001_declareStmt_preproc_v3.patch

* 002_declareStmt_ecpglib_v3.patch

* 003_declareStmt_doc_v3.patch

* 004_declareStmt_test_v3.patch

Here is a short summary:

[001_declareStmt_preproc_v3.patch]

This enables ecpg to pre-process "DECLARE prepared_name STATEMENT".

prepared_name is buffered to g_declared_list to check duplication of prepared_name.

After pre-processed, "DECLARE STATEMENT" is translated to ECPGdeclare().

And CUSRSOR STAETEMENT such as OPEN/FETCH/CLOSE cursor is translated into ECPGopen()/ECPGfetch()/ECPGclose().

These new function is defined at 002_declareStmt_ecpglib_v3.patch.

[002_declareStmt_ecpglib_v3.patch]

This patch mainly implements ECPGdeclare(), ECPGopen(), ECPGfetch(), ECPGclose().

ECPGdeclare() links the declared name and connection name.

Handling CURSOR things are originally done by ECPGdo().

But in order to handle connection linked to declared name,

the functions such as ECPGopen(), ECPGfetch()and ECPGclose() are introduced and these functions wraps ecpg_do().

[003_declareStmt_doc_v3.patch]

Docs. I wrote the DECLARE STATEMENT itself.

And added another example to ecpg-set-connection.

[004_declareStmt_test_v3.patch]

Regression test and answers.

I made them but I'm thinking these include too much test cases, don't they?

So I'm planning to make it smaller.

regards,

Ideriha Takeshi

Hi

[004_declareStmt_test_v3.patch]
Regression test and answers.
I made them but I'm thinking these include too much test cases, don't they?
So I'm planning to make it smaller.

I refactored the regression test patch (004_declareStmt_test_v4.patch)
to make it easier to read.

The other patches are same as previous versions.

Regards
Ideriha, Takeshi

From: pgsql-hackers-owner@postgresql.org [mailto:pgsql-hackers-owner@postgresql.org] On Behalf Of Ideriha, Takeshi
Sent: Tuesday, February 7, 2017 5:59 PM
To: Michael Paquier <michael.paquier@gmail.com>
Cc: Michael Meskes <meskes@postgresql.org>; pgsql-hackers@postgresql.org
Subject: Re: [WIP] RE: [HACKERS] DECLARE STATEMENT setting up a connection in ECPG

Hi

By the way, I have been able to crash your patch when running the regression

tests:

(lldb) bt

* thread #1: tid = 0x0000, 0x00007fff89a828b0

libsystem_platform.dylib`_platform_strcmp + 176, stop reason = signal SIGSTOP

* frame #0: 0x00007fff89a828b0 libsystem_platform.dylib`_platform_strcmp +

176

frame #1: 0x000000010c835bc3

libecpg.6.dylib`ecpg_release_declared_statement(connection_name="con3")

+ 83 at prepare.c:740

frame #2: 0x000000010c838103

libecpg.6.dylib`ECPGdisconnect(lineno=81, connection_name="ALL") + 179 at

connect.c:697

frame #3: 0x000000010c811922 declare`main(argc=1,

argv=0x00007fff533ee320) + 434 at declare.pgc:81

frame #4: 0x00007fff932345ad libdyld.dylib`start + 1

You also need to add in src/interfaces/ecpg/test/sql/.gitignore new entries

related to the files you are adding and that get generated.

Thank you very much for your test. I fixed this memory leak bug, and fixed .gitignore.

I also fixed some code style to fit coding conventions,

and splited my patch into 4 parts to improve readability:

* 001_declareStmt_preproc_v3.patch

* 002_declareStmt_ecpglib_v3.patch

* 003_declareStmt_doc_v3.patch

* 004_declareStmt_test_v3.patch

Here is a short summary:

[001_declareStmt_preproc_v3.patch]

This enables ecpg to pre-process "DECLARE prepared_name STATEMENT".

prepared_name is buffered to g_declared_list to check duplication of prepared_name.

After pre-processed, "DECLARE STATEMENT" is translated to ECPGdeclare().

And CUSRSOR STAETEMENT such as OPEN/FETCH/CLOSE cursor is translated into ECPGopen()/ECPGfetch()/ECPGclose().

These new function is defined at 002_declareStmt_ecpglib_v3.patch.

[002_declareStmt_ecpglib_v3.patch]

This patch mainly implements ECPGdeclare(), ECPGopen(), ECPGfetch(), ECPGclose().

ECPGdeclare() links the declared name and connection name.

Handling CURSOR things are originally done by ECPGdo().

But in order to handle connection linked to declared name,

the functions such as ECPGopen(), ECPGfetch()and ECPGclose() are introduced and these functions wraps ecpg_do().

[003_declareStmt_doc_v3.patch]

Docs. I wrote the DECLARE STATEMENT itself.

And added another example to ecpg-set-connection.

[004_declareStmt_test_v3.patch]

Regression test and answers.

I made them but I'm thinking these include too much test cases, don't they?

So I'm planning to make it smaller.

regards,

Ideriha Takeshi

Hi

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

$ git apply ../004_declareStmt_test_v4.patch
error: patch failed: src/interfaces/ecpg/test/expected/pgtypeslib-nan_test.c:82
error: src/interfaces/ecpg/test/expected/pgtypeslib-nan_test.c: patch does not apply

Regards,
Okano Naoki

Hi
Thank you for checking!

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

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
david@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

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 close
statement. Because of the above code addition, the code present in
stmtClosePortalStmt
is 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)

I am yet to review the other patches.

Regards,
Hari Babu
Fujitsu Australia

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 close
statement. Because of the above code addition, the code present in
stmtClosePortalStmt
is 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)

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.

+ 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

Hi, thank you very much for reviewing.
Attached is v6 patch.

There was a minor conflict in applying 004_declareXX patch.

I rebased 004_declareStmt_test_v6.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 close
statement. Because of the above code addition, the code present in stmtClosePortalStmt
is a dead code. So remove the code present in stmtClosePortalStmt or try to reuse it.

I remove almost all the stmtClosePortalStmt code
and just leave the interface which actually does nothing not to affect other codes.

But I'm not sure this implementation is good or not.
Maybe I should completely remove stmtClosePortalStmt code
and rename ClosePortalStmtCLOSEcursor_name to stmtClosePortalStmt to make code easier to understand.

+static void
+output_cursor_name(char *str)

This function needs some comments to explain the code flow for better understanding.

I add some explanation that output_cursor_name() filters escape sequences.

+/*
+ * Translate the EXEC SQL DECLARE STATEMENT into ECPGdeclare function
+ */

How about using Transform instead of Translate? (similar references also)

I changed two comments with the word Translate.

regards,
Ideriha, Takeshi

On Wed, Mar 22, 2017 at 7:57 PM, Ideriha, Takeshi <
ideriha.takeshi@jp.fujitsu.com> wrote:

Hi, thank you very much for reviewing.
Attached is v6 patch.

There was a minor conflict in applying 004_declareXX patch.

I rebased 004_declareStmt_test_v6.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 close

statement. Because of the above code addition, the code present in

stmtClosePortalStmt

is a dead code. So remove the code present in stmtClosePortalStmt or try

to reuse it.

I remove almost all the stmtClosePortalStmt code
and just leave the interface which actually does nothing not to affect
other codes.

But I'm not sure this implementation is good or not.
Maybe I should completely remove stmtClosePortalStmt code
and rename ClosePortalStmtCLOSEcursor_name to stmtClosePortalStmt to make
code easier to understand.

+static void
+output_cursor_name(char *str)

This function needs some comments to explain the code flow for better

understanding.

I add some explanation that output_cursor_name() filters escape sequences.

+/*
+ * Translate the EXEC SQL DECLARE STATEMENT into ECPGdeclare function
+ */

How about using Transform instead of Translate? (similar references also)

I changed two comments with the word Translate.

Thanks for the updated patch. It looks good to me.
I have some comments in the doc patch.

+
+  <para>
+   The third option is to declare a sql identifier linked to
+   the connection, for example:
+<programlisting>
+EXEC SQL AT <replaceable>connection-name</replaceable> DECLARE
<replaceable>statement-name</replaceable> STATEMENT;
+EXEC SQL PREPARE <replaceable>statement-name</replaceable> FROM
:<replaceable>dyn-string</replaceable>;
+</programlisting>
+   Once you link a sql identifier to a connection, you execute a dynamic
SQL
+   without AT clause.
+  </para>

The above code part should be moved to the below of the following code
location
and provide some example usage of it in the example code will be better.

<programlisting>
EXEC SQL SET CONNECTION <replaceable>connection-name</replaceable>;
</programlisting>

+    <para>
+     <command>DELARE CURSOR</command> with a SQL statement identifier can
be written before PREPARE.
+    </para>

what is the need of providing details about DECLARE CURSOR here?

+    <para>
+     AT clause cannot be used with the SQL statement which have been
identified by <command>DECLARE STATEMENT</command>
+    </para>

I didn't clearly understand the limitation here, If you can provide an
example here, it will be good.

The test patch looks good to me.

Regards,
Hari Babu
Fujitsu Australia

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
david@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

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

On Thu, Mar 30, 2017 at 1:57 PM, Ideriha, Takeshi <
ideriha.takeshi@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

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