From: | Haribabu Kommi <kommi(dot)haribabu(at)gmail(dot)com> |
---|---|
To: | "Ideriha, Takeshi" <ideriha(dot)takeshi(at)jp(dot)fujitsu(dot)com> |
Cc: | "Okano, Naoki" <okano(dot)naoki(at)jp(dot)fujitsu(dot)com>, Michael Meskes <meskes(at)postgresql(dot)org>, Michael Paquier <michael(dot)paquier(at)gmail(dot)com>, "pgsql-hackers(at)postgresql(dot)org" <pgsql-hackers(at)postgresql(dot)org> |
Subject: | Re: [WIP] RE: DECLARE STATEMENT setting up a connection in ECPG |
Date: | 2017-03-23 05:33:54 |
Message-ID: | CAJrrPGfhmYHgNvYU_o+w8b0JrWwuRTejgKGn3siXn6KyvQDVqA@mail.gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
On Wed, Mar 22, 2017 at 7:57 PM, Ideriha, Takeshi <
ideriha(dot)takeshi(at)jp(dot)fujitsu(dot)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
From | Date | Subject | |
---|---|---|---|
Next Message | Rushabh Lathia | 2017-03-23 05:38:04 | dblink module printing unnamed connection (with commit acaf7ccb94) |
Previous Message | Craig Ringer | 2017-03-23 05:11:13 | Re: Logical decoding on standby |