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

[Haribabu Kommi]

On Wed, Mar 22, 2017 at 7:57 PM, Ideriha, Takeshi <
ideriha.take...@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.

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

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.


EXEC SQL SET CONNECTION connection-name;



+
+ DELARE CURSOR with a SQL statement identifier can
be written before PREPARE.
+

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

+
+ AT clause cannot be used with the SQL statement which have been
identified by DECLARE STATEMENT
+

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

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

[Haribabu Kommi]

On Wed, Mar 22, 2017 at 12:51 PM, Haribabu Kommi 
wrote:

>
>
> On Tue, Mar 7, 2017 at 4:09 PM, Ideriha, Takeshi <
> ideriha.take...@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

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

[Haribabu Kommi]

On Tue, Mar 7, 2017 at 4:09 PM, Ideriha, Takeshi <
ideriha.take...@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

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

[Okano, Naoki]

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

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

[Michael Paquier]

On Wed, Jan 25, 2017 at 2:58 PM, Ideriha, Takeshi
 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

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

[Ideriha, Takeshi]

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.paqu...@gmail.com>
> Cc: Michael Meskes <mes...@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

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

[Ideriha, Takeshi]

> 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

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

[Michael Paquier]

On Fri, Jan 6, 2017 at 6:10 PM, Ideriha, Takeshi
 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 = 0x, 0x7fff89a828b0
libsystem_platform.dylib`_platform_strcmp + 176, stop reason = signal
SIGSTOP
  * frame #0: 0x7fff89a828b0 libsystem_platform.dylib`_platform_strcmp + 176
frame #1: 0x00010c835bc3
libecpg.6.dylib`ecpg_release_declared_statement(connection_name="con3")
+ 83 at prepare.c:740
frame #2: 0x00010c838103
libecpg.6.dylib`ECPGdisconnect(lineno=81, connection_name="ALL") + 179
at connect.c:697
frame #3: 0x00010c811922 declare`main(argc=1,
argv=0x7fff533ee320) + 434 at declare.pgc:81
frame #4: 0x7fff932345ad 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

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

[Michael Meskes]

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

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

[Ideriha, Takeshi]

> 
> I'm looking forward to seeing your patch.
>

I created a patch.

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

I'm going to post this patch to the January CF. 
But it's my first time to send a patch so please excuse me if there's something 
you feel bad with.

Regards, 
Ideriha Takeshi

> -Original Message-
> From: pgsql-hackers-ow...@postgresql.org
> [mailto:pgsql-hackers-ow...@postgresql.org] On Behalf Of Michael Meskes
> Sent: Tuesday, November 22, 2016 2:57 AM
> To: Ideriha, Takeshi 
> Cc: pgsql-hackers@postgresql.org
> Subject: Re: [HACKERS] DECLARE STATEMENT setting up a connection in ECPG
> 
> > I wanted to say that in order to use the connection pointed by the
> > DECLARE STATEMENT some functions like ECPGdo() would be modified or
> > new function would be added under the directory ecpglib/.
> >
> > This modification or new function will be used to get the connection
> > by statement_name.
> 
> Ah, now I understand. Thank you for your explanation.
> 
> I'm looking forward to seeing your patch.
> 
> 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


001_declareStatement_v1.patch.tar.gz
Description: 001_declareStatement_v1.patch.tar.gz

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