Thread: ECPG doesn't compile CREATE AS EXECUTE properly.

Hello.

While I looked a patch, I found that the following ECPG statement
generates uncompilable .c source.

EXEC SQL CREATE TABLE t AS stmt;

ecpgtest.c:
#line 42 "ecpgtest.pgc"

        printf("1:dbname=%s\n", dbname);
        { ECPGdo(__LINE__, 0, 1, NULL, 0, ECPGst_execute, create table t as execute "stmt", ECPGt_EOIT, ECPGt_EORT);

This is apparently broken. The cause is that the rule ExecutStmt is
assumed to return a statement name when type is empty (or null), while
it actually returns a full statement for the CREATE TABLE AS EXECUTE
syntax.

Separating "CREATE TABLE AS EXECUTE" from ExecuteStmt would be cleaner
but I avoided to change the syntax tree. Instead the attched make
distinction of $$.type of ExecuteStmt between NULL and "" to use to
notify the returned name is name of a prepared statement or a full
statement.

I'll post the test part later.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center
From 75a99097a988c81802d659cf8a58d74060d36409 Mon Sep 17 00:00:00 2001
From: Kyotaro Horiguchi <horikyota.ntt@gmail.com>
Date: Thu, 1 Jul 2021 14:56:42 +0900
Subject: [PATCH v1] Fix ECPG's CREATE TABLE AS EXECUTE

The syntax was precompiled into uncompilable .c statement.

Avoiding impact on parse tree structure, use ExecuteStmt.type to
notify whether the returned string is a statment name or a full
statement.
---
 src/interfaces/ecpg/preproc/ecpg.addons | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/src/interfaces/ecpg/preproc/ecpg.addons b/src/interfaces/ecpg/preproc/ecpg.addons
index b6e3412cef..820608605b 100644
--- a/src/interfaces/ecpg/preproc/ecpg.addons
+++ b/src/interfaces/ecpg/preproc/ecpg.addons
@@ -34,7 +34,9 @@ ECPG: stmtUpdateStmt block
 ECPG: stmtExecuteStmt block
     {
         check_declared_list($1.name);
-        if ($1.type == NULL || strlen($1.type) == 0)
+        if ($1.type == NULL)
+            output_statement($1.name, 1, ECPGst_normal);
+        else if (strlen($1.type) == 0)
             output_statement($1.name, 1, ECPGst_execute);
         else
         {
@@ -362,14 +364,18 @@ ECPG: ExecuteStmtEXECUTEprepared_nameexecute_param_clauseexecute_rest block
     {
         $$.name = $2;
         $$.type = $3;
+        if ($$.type == NULL)
+           $$.type = "";
     }
 ECPG: ExecuteStmtCREATEOptTempTABLEcreate_as_targetASEXECUTEprepared_nameexecute_param_clauseopt_with_dataexecute_rest
block
     {
         $$.name = cat_str(8,mm_strdup("create"),$2,mm_strdup("table"),$4,mm_strdup("as execute"),$7,$8,$9);
+        $$.type = NULL;
     }
 ECPG:
ExecuteStmtCREATEOptTempTABLEIF_PNOTEXISTScreate_as_targetASEXECUTEprepared_nameexecute_param_clauseopt_with_dataexecute_rest
block
     {
         $$.name = cat_str(8,mm_strdup("create"),$2,mm_strdup("table if not exists"),$7,mm_strdup("as
execute"),$10,$11,$12);
+        $$.type = NULL;
     }
 ECPG: DeclareCursorStmtDECLAREcursor_namecursor_optionsCURSORopt_holdFORSelectStmt block
     {
-- 
2.27.0

At Thu, 01 Jul 2021 18:45:25 +0900 (JST), Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote in 
> I'll post the test part later.

A version incluedes the test part.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center
From 6ef902608483f395983da5063db3e0af8d61ed4e Mon Sep 17 00:00:00 2001
From: Kyotaro Horiguchi <horikyota.ntt@gmail.com>
Date: Thu, 1 Jul 2021 14:56:42 +0900
Subject: [PATCH v2 1/2] Fix ECPG's CREATE TABLE AS EXECUTE

The syntax was precompiled into uncompilable .c statement.

Avoiding impact on parse tree structure, use ExecuteStmt.type to
notify whether the returned string is a statment name or a full
statement.
---
 src/interfaces/ecpg/preproc/ecpg.addons | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/src/interfaces/ecpg/preproc/ecpg.addons b/src/interfaces/ecpg/preproc/ecpg.addons
index b6e3412cef..820608605b 100644
--- a/src/interfaces/ecpg/preproc/ecpg.addons
+++ b/src/interfaces/ecpg/preproc/ecpg.addons
@@ -34,7 +34,9 @@ ECPG: stmtUpdateStmt block
 ECPG: stmtExecuteStmt block
     {
         check_declared_list($1.name);
-        if ($1.type == NULL || strlen($1.type) == 0)
+        if ($1.type == NULL)
+            output_statement($1.name, 1, ECPGst_normal);
+        else if (strlen($1.type) == 0)
             output_statement($1.name, 1, ECPGst_execute);
         else
         {
@@ -362,14 +364,18 @@ ECPG: ExecuteStmtEXECUTEprepared_nameexecute_param_clauseexecute_rest block
     {
         $$.name = $2;
         $$.type = $3;
+        if ($$.type == NULL)
+           $$.type = "";
     }
 ECPG: ExecuteStmtCREATEOptTempTABLEcreate_as_targetASEXECUTEprepared_nameexecute_param_clauseopt_with_dataexecute_rest
block
     {
         $$.name = cat_str(8,mm_strdup("create"),$2,mm_strdup("table"),$4,mm_strdup("as execute"),$7,$8,$9);
+        $$.type = NULL;
     }
 ECPG:
ExecuteStmtCREATEOptTempTABLEIF_PNOTEXISTScreate_as_targetASEXECUTEprepared_nameexecute_param_clauseopt_with_dataexecute_rest
block
     {
         $$.name = cat_str(8,mm_strdup("create"),$2,mm_strdup("table if not exists"),$7,mm_strdup("as
execute"),$10,$11,$12);
+        $$.type = NULL;
     }
 ECPG: DeclareCursorStmtDECLAREcursor_namecursor_optionsCURSORopt_holdFORSelectStmt block
     {
-- 
2.27.0

From a1bc18dacddae1af616d1ce167cae1c246483a2c Mon Sep 17 00:00:00 2001
From: Kyotaro Horiguchi <horikyota.ntt@gmail.com>
Date: Thu, 1 Jul 2021 19:13:09 +0900
Subject: [PATCH v2 2/2] test part

---
 .../ecpg/test/expected/sql-execute.c          | 29 ++++++++++++++-----
 .../ecpg/test/expected/sql-execute.stderr     | 22 ++++++++++----
 src/interfaces/ecpg/test/sql/execute.pgc      |  3 ++
 3 files changed, 41 insertions(+), 13 deletions(-)

diff --git a/src/interfaces/ecpg/test/expected/sql-execute.c b/src/interfaces/ecpg/test/expected/sql-execute.c
index 10e9ad56b5..f5e9cfd664 100644
--- a/src/interfaces/ecpg/test/expected/sql-execute.c
+++ b/src/interfaces/ecpg/test/expected/sql-execute.c
@@ -302,29 +302,42 @@ if (sqlca.sqlcode < 0) sqlprint();}
         printf("name[%d]=%8.8s\tamount[%d]=%d\tletter[%d]=%c\n", i, n, i, a, i, l);
     }
 
+    { ECPGdo(__LINE__, 0, 1, NULL, 0, ECPGst_normal, "create table newt as execute \"f\" ( 2 )", ECPGt_EOIT,
ECPGt_EORT);
+#line 107 "execute.pgc"
+
+if (sqlca.sqlcode < 0) sqlprint();}
+#line 107 "execute.pgc"
+
+
     { ECPGdeallocate(__LINE__, 0, NULL, "f");
-#line 107 "execute.pgc"
+#line 109 "execute.pgc"
 
 if (sqlca.sqlcode < 0) sqlprint();}
-#line 107 "execute.pgc"
+#line 109 "execute.pgc"
+
+    { ECPGdo(__LINE__, 0, 1, NULL, 0, ECPGst_normal, "drop table newt", ECPGt_EOIT, ECPGt_EORT);
+#line 110 "execute.pgc"
+
+if (sqlca.sqlcode < 0) sqlprint();}
+#line 110 "execute.pgc"
 
     { ECPGdo(__LINE__, 0, 1, NULL, 0, ECPGst_normal, "drop table test", ECPGt_EOIT, ECPGt_EORT);
-#line 108 "execute.pgc"
+#line 111 "execute.pgc"
 
 if (sqlca.sqlcode < 0) sqlprint();}
-#line 108 "execute.pgc"
+#line 111 "execute.pgc"
 
     { ECPGtrans(__LINE__, NULL, "commit");
-#line 109 "execute.pgc"
+#line 112 "execute.pgc"
 
 if (sqlca.sqlcode < 0) sqlprint();}
-#line 109 "execute.pgc"
+#line 112 "execute.pgc"
 
     { ECPGdisconnect(__LINE__, "CURRENT");
-#line 110 "execute.pgc"
+#line 113 "execute.pgc"
 
 if (sqlca.sqlcode < 0) sqlprint();}
-#line 110 "execute.pgc"
+#line 113 "execute.pgc"
 
 
     return 0;
diff --git a/src/interfaces/ecpg/test/expected/sql-execute.stderr
b/src/interfaces/ecpg/test/expected/sql-execute.stderr
index d8bc3c6524..26b72cf27f 100644
--- a/src/interfaces/ecpg/test/expected/sql-execute.stderr
+++ b/src/interfaces/ecpg/test/expected/sql-execute.stderr
@@ -156,15 +156,27 @@
 [NO_PID]: sqlca: code: 0, state: 00000
 [NO_PID]: ecpg_get_data on line 94: RESULT: t offset: -1; array: no
 [NO_PID]: sqlca: code: 0, state: 00000
-[NO_PID]: deallocate_one on line 107: name f
+[NO_PID]: ecpg_execute on line 107: query: create table newt as execute "f" ( 2 ); with 0 parameter(s) on connection
main
 [NO_PID]: sqlca: code: 0, state: 00000
-[NO_PID]: ecpg_execute on line 108: query: drop table test; with 0 parameter(s) on connection main
+[NO_PID]: ecpg_execute on line 107: using PQexec
 [NO_PID]: sqlca: code: 0, state: 00000
-[NO_PID]: ecpg_execute on line 108: using PQexec
+[NO_PID]: ecpg_process_output on line 107: OK: SELECT 1
 [NO_PID]: sqlca: code: 0, state: 00000
-[NO_PID]: ecpg_process_output on line 108: OK: DROP TABLE
+[NO_PID]: deallocate_one on line 109: name f
 [NO_PID]: sqlca: code: 0, state: 00000
-[NO_PID]: ECPGtrans on line 109: action "commit"; connection "main"
+[NO_PID]: ecpg_execute on line 110: query: drop table newt; with 0 parameter(s) on connection main
+[NO_PID]: sqlca: code: 0, state: 00000
+[NO_PID]: ecpg_execute on line 110: using PQexec
+[NO_PID]: sqlca: code: 0, state: 00000
+[NO_PID]: ecpg_process_output on line 110: OK: DROP TABLE
+[NO_PID]: sqlca: code: 0, state: 00000
+[NO_PID]: ecpg_execute on line 111: query: drop table test; with 0 parameter(s) on connection main
+[NO_PID]: sqlca: code: 0, state: 00000
+[NO_PID]: ecpg_execute on line 111: using PQexec
+[NO_PID]: sqlca: code: 0, state: 00000
+[NO_PID]: ecpg_process_output on line 111: OK: DROP TABLE
+[NO_PID]: sqlca: code: 0, state: 00000
+[NO_PID]: ECPGtrans on line 112: action "commit"; connection "main"
 [NO_PID]: sqlca: code: 0, state: 00000
 [NO_PID]: deallocate_one on line 0: name i
 [NO_PID]: sqlca: code: 0, state: 00000
diff --git a/src/interfaces/ecpg/test/sql/execute.pgc b/src/interfaces/ecpg/test/sql/execute.pgc
index 43171bb77c..59f5dca963 100644
--- a/src/interfaces/ecpg/test/sql/execute.pgc
+++ b/src/interfaces/ecpg/test/sql/execute.pgc
@@ -104,7 +104,10 @@ exec sql end declare section;
         printf("name[%d]=%8.8s\tamount[%d]=%d\tletter[%d]=%c\n", i, n, i, a, i, l);
     }
 
+    exec sql create table newt as execute f(2);
+
     exec sql deallocate f;
+    exec sql drop table newt;
     exec sql drop table test;
     exec sql commit;
     exec sql disconnect;
-- 
2.27.0

On Thu, Jul 01, 2021 at 06:45:25PM +0900, Kyotaro Horiguchi wrote:
> Separating "CREATE TABLE AS EXECUTE" from ExecuteStmt would be cleaner
> but I avoided to change the syntax tree. Instead the attched make
> distinction of $$.type of ExecuteStmt between NULL and "" to use to
> notify the returned name is name of a prepared statement or a full
> statement.

I am not so sure, and using an empty string makes the code a bit
harder to follow.  How would that look with the grammar split you have
in mind?  Maybe that makes the code more consistent with the PREPARE
block a couple of lines above?
--
Michael

Thanks for the comment.

At Tue, 6 Jul 2021 11:17:47 +0900, Michael Paquier <michael@paquier.xyz> wrote in 
> On Thu, Jul 01, 2021 at 06:45:25PM +0900, Kyotaro Horiguchi wrote:
> > Separating "CREATE TABLE AS EXECUTE" from ExecuteStmt would be cleaner
> > but I avoided to change the syntax tree. Instead the attched make
> > distinction of $$.type of ExecuteStmt between NULL and "" to use to
> > notify the returned name is name of a prepared statement or a full
> > statement.
> 
> I am not so sure, and using an empty string makes the code a bit
> harder to follow.  How would that look with the grammar split you have

I agree to that.

> in mind?  Maybe that makes the code more consistent with the PREPARE
> block a couple of lines above?

More accurately, I didn't come up with the way to split out some of
the rule-components in a rule out as another rule using the existing
infrastructure.

preproc.y:

 ExecuteStmt:
EXECUTE prepared_name execute_param_clause execute_rest
    {}
| CREATE OptTemp TABLE create_as_target AS EXECUTE prepared_name execute_param_clause opt_with_data execute_rest
    {}
| CREATE OptTemp TABLE IF_P NOT EXISTS create_as_target AS EXECUTE prepared_name execute_param_clause opt_with_data
execute_rest
    {}
;

I can directly edit this as the following:

 ExecuteStmt:
EXECUTE prepared_name execute_param_clause execute_rest
    {}
;

CreateExecuteStmt:
| CREATE OptTemp TABLE create_as_target AS EXECUTE prepared_name execute_param_clause opt_with_data execute_rest
    {}
| CREATE OptTemp TABLE IF_P NOT EXISTS create_as_target AS EXECUTE prepared_name execute_param_clause opt_with_data
execute_rest
    {}
;

Then add the following component to the rule "stmt".

| CreateExecuteStmt:
  { output_statement(..., ECPGst_normal); }

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

On Tue, Jul 06, 2021 at 05:47:34PM +0900, Kyotaro Horiguchi wrote:
> More accurately, I didn't come up with the way to split out some of
> the rule-components in a rule out as another rule using the existing
> infrastructure.
>
> [...]
>
> Then add the following component to the rule "stmt".

I see.  That sounds interesting as solution, and consistent with the
existing cursor queries.  The ECPG parser is not that advanced, and we
may not really need to make it more complicated with sub-clause
handling like INEs.  So I'd be rather in favor of what you are
describing here.
--
Michael