RE: ECPG: proposal for new DECLARE STATEMENT
Dear Hackers, I know I'm asking a big favor, but could you review(and commit) the patch? The status has become RFC last Nov., but no one checked this after that. Maybe Meskes is quite busy and have no time to review it. The main part of the patch is about 200 lines(It means not so long), and maybe I have reviewed other patches more than it. I will review more, so I'm happy if this commits until the end of next CF. Best Regards, Hayato Kuroda FUJITSU LIMITED
Re: ECPG: proposal for new DECLARE STATEMENT
The following review has been posted through the commitfest application: make installcheck-world: tested, passed Implements feature: tested, passed Spec compliant: tested, passed Documentation:tested, passed Recently I have been doing some work on ecpg. So I review this patch. No problem was found. The new status of this patch is: Ready for Committer
RE: ECPG: proposal for new DECLARE STATEMENT
Dear Tomas, Daniel, Michael, I missed your e-mails, and I apologize the very late reply. I want you to thank keeping the thread. > I'm not an ecpg expert (in fact I've never even used it), so my review > is pretty superficial, but I only found a couple of minor whitespace > issues (adding/removing a line/tab) - see the attached file. Thanks, I fixed it. > Kuroda-san, you mentioned the patch is WIP. What other bits you think > are missing / need improvement? I see you mentioned some documentation > is missing - I suppose that's one of the missing pieces? All functionalities I expect has been already implemented in the previous patch, and I thought that only doc and reviews were needed. Finally I attach new patch. This patch contains source changes, a test code, and documentation changes. This one is not WIP. I will try to review other topics on the next Commitfest. Best regards, Hayato Kuroda FUJITSU LIMITED -Original Message- From: Michael Meskes Sent: Tuesday, September 15, 2020 7:32 PM To: pgsql-hackers@lists.postgresql.org Subject: Re: ECPG: proposal for new DECLARE STATEMENT > This patch has now been silent for quite a while, unless someone is > interested > enough to bring it forward it seems about time to close it. I am interested but still short on time. I will definitely look into it as soon as I find some spare minutes. Michael -- Michael Meskes Michael at Fam-Meskes dot De Michael at Meskes dot (De|Com|Net|Org) Meskes at (Debian|Postgresql) dot Org DeclareStmt02.patch Description: DeclareStmt02.patch
Re: ECPG: proposal for new DECLARE STATEMENT
> This patch has now been silent for quite a while, unless someone is > interested > enough to bring it forward it seems about time to close it. I am interested but still short on time. I will definitely look into it as soon as I find some spare minutes. Michael -- Michael Meskes Michael at Fam-Meskes dot De Michael at Meskes dot (De|Com|Net|Org) Meskes at (Debian|Postgresql) dot Org
Re: ECPG: proposal for new DECLARE STATEMENT
> On 30 Mar 2020, at 18:53, David Steele wrote: > > On 1/11/20 10:41 PM, Tomas Vondra wrote: >> On Sun, Jan 12, 2020 at 03:52:48AM +0100, Tomas Vondra wrote: >>> ... >>> >>> I'm not an ecpg expert (in fact I've never even used it), so my review >>> is pretty superficial, but I only found a couple of minor whitespace >>> issues (adding/removing a line/tab) - see the attached file. >>> >> Meh, forgot to attach the file ... > > Any thoughts on Tomas' comments? > > A big part of moving a patch forward is keeping the thread active and > answering comments/review. This patch has now been silent for quite a while, unless someone is interested enough to bring it forward it seems about time to close it. cheers ./daniel
Re: ECPG: proposal for new DECLARE STATEMENT
On 1/11/20 10:41 PM, Tomas Vondra wrote: On Sun, Jan 12, 2020 at 03:52:48AM +0100, Tomas Vondra wrote: ... I'm not an ecpg expert (in fact I've never even used it), so my review is pretty superficial, but I only found a couple of minor whitespace issues (adding/removing a line/tab) - see the attached file. Meh, forgot to attach the file ... Any thoughts on Tomas' comments? A big part of moving a patch forward is keeping the thread active and answering comments/review. Regards, -- -David da...@pgmasters.net
Re: ECPG: proposal for new DECLARE STATEMENT
On Sun, Jan 12, 2020 at 03:52:48AM +0100, Tomas Vondra wrote: ... I'm not an ecpg expert (in fact I've never even used it), so my review is pretty superficial, but I only found a couple of minor whitespace issues (adding/removing a line/tab) - see the attached file. Meh, forgot to attach the file ... regards -- Tomas Vondra http://www.2ndQuadrant.com PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services diff --git a/src/interfaces/ecpg/preproc/ecpg.addons b/src/interfaces/ecpg/preproc/ecpg.addons index 01b36e3a3d..5fcc90dc84 100644 --- a/src/interfaces/ecpg/preproc/ecpg.addons +++ b/src/interfaces/ecpg/preproc/ecpg.addons @@ -60,7 +60,6 @@ ECPG: stmtPrepareStmt block check_declared_list($1.name); if ($1.type == NULL) output_prepare_statement($1.name, $1.stmt); - else if (strlen($1.type) == 0) { char *stmt = cat_str(3, mm_strdup("\""), $1.stmt, mm_strdup("\"")); diff --git a/src/interfaces/ecpg/preproc/ecpg.c b/src/interfaces/ecpg/preproc/ecpg.c index 1e8a3e6b60..93696ceb3c 100644 --- a/src/interfaces/ecpg/preproc/ecpg.c +++ b/src/interfaces/ecpg/preproc/ecpg.c @@ -495,7 +495,6 @@ main(int argc, char *const argv[]) } free(input_filename); - } } return ret_value; diff --git a/src/interfaces/ecpg/preproc/ecpg.trailer b/src/interfaces/ecpg/preproc/ecpg.trailer index 8e65d24036..052ec24077 100644 --- a/src/interfaces/ecpg/preproc/ecpg.trailer +++ b/src/interfaces/ecpg/preproc/ecpg.trailer @@ -21,7 +21,7 @@ statement: ecpgstart at stmt ';' { connection = NULL; } remove_typedefs(braces_open); remove_variables(braces_open--); if (braces_open == 0) - { + { free(current_function); current_function = NULL; } diff --git a/src/interfaces/ecpg/preproc/output.c b/src/interfaces/ecpg/preproc/output.c index 94bc433ed5..65d06d5794 100644 --- a/src/interfaces/ecpg/preproc/output.c +++ b/src/interfaces/ecpg/preproc/output.c @@ -258,4 +258,3 @@ output_escaped_str(char *str, bool quoted) if (quoted && str[0] == '"' && str[len] == '"') fputs("\"", base_yyout); } -
Re: ECPG: proposal for new DECLARE STATEMENT
Hi, On Thu, Oct 31, 2019 at 12:29:30PM +, kuroda.hay...@fujitsu.com wrote: Dear hackers, As declared last month, I propose again the new ECPG grammar, DECLARE STATEMENT. This had been committed once, but it removed from PG12 because of some problems. In this mail, I want to report some problems that previous implementation has, produce a new solution, and attach a WIP patch. [Basic function, Grammar, and Use case] This statement will be used for the purpose of designating a connection easily. Please see below: https://www.postgresql.org/message-id/flat/4E72940DA2BF16479384A86D54D0988A4D80D3C9@G01JPEXMBKW04 The Oracle's manual will also help your understanding: https://docs.oracle.com/en/database/oracle/oracle-database/19/lnpcc/embedded-SQL-statements-and-directives.html#GUID-0A30B7B4-BD91-42EA-AACE-2E9CBF7E9C1A [Issues] That's why this feature has been reverted. 1. The namespace of the identifier was not clear. If you use a same identifier for other SQL statements, these interfered each other and statements might be executed at the unexpected connection. 2. Declaring at the outside of functions was not allowed. This specification is quite different from the other declarative statements, so some users might be confused. For instance, the following example was rejected. ``` EXEC SQL DECLARE stmt STATEMENT; int main() { ... EXEC SQL DECLARE cur CURSOR FOR stmt; ... } ``` 3. These specifications were not compatible with other DBMSs. [Solutions] The namespace is set to be a file unit. This follows other DBMSs. When the DECLARE SATATEMENT statement is read, the name, identifier and the related connection are recorded. And if you use the declared identifier in order to prepare or declare cursor, the fourth argument for ECPGdo(it represents the connection) will be overwritten. This declaration is enabled only the precompile phase. [Limitations] The declaration must be appeared before using it. This also follows Pro*C precompiler. A WIP patch is attached. Confirm that all ECPG tests have passed, however, some documents are not included. They will be added later. I applied the pgindent as a test, but it might be failed because this is the first time for me. I see there were no reviews of this new patch, with the feature reimplemented after it was reverted from PG12 in September :-( I'm not an ecpg expert (in fact I've never even used it), so my review is pretty superficial, but I only found a couple of minor whitespace issues (adding/removing a line/tab) - see the attached file. Kuroda-san, you mentioned the patch is WIP. What other bits you think are missing / need improvement? I see you mentioned some documentation is missing - I suppose that's one of the missing pieces? For the record, there were two threads discussing the implementation [1] and then the revert [2]. [1] https://www.postgresql.org/message-id/flat/1F66B161998C704BABF8989B8A2AC0A313AA41%40G01JPEXMBYT05 [2] https://www.postgresql.org/message-id/flat/TY2PR01MB2443EC8286995378AEB7D9F8F5B10%40TY2PR01MB2443.jpnprd01.prod.outlook.com regards -- Tomas Vondra http://www.2ndQuadrant.com PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services