Thread: [WIP] RE: [HACKERS] DECLARE STATEMENT setting up a connection inECPG

> 
> 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-owner@postgresql.org
> [mailto:pgsql-hackers-owner@postgresql.org] On Behalf Of Michael Meskes
> Sent: Tuesday, November 22, 2016 2:57 AM
> To: Ideriha, Takeshi <ideriha.takeshi@jp.fujitsu.com>
> 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

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

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

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

> -----Original Message-----
> From: Michael Meskes [mailto:meskes@postgresql.org]
> Sent: Sunday, January 1, 2017 7:18 PM
> To: Ideriha, Takeshi <ideriha.takeshi@jp.fujitsu.com>
> Cc: pgsql-hackers@postgresql.org
> Subject: Re: [WIP] RE: [HACKERS] 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

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

> 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

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

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

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

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

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

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

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

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

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

> On 30 Mar 2017, at 09:11, Ideriha, Takeshi <ideriha.takeshi@jp.fujitsu.com> 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