Keep ECPG comment for log_min_duration_statement
Hi,
I'd like to make it easier to analyze slow queries in ECPG using
log_min_duration_statement. Especially when DBA and C application developer
is different, DBA can feedback slow embedded query to the developer without
difficulty and mistakes.
When our customers log and look into slower queries in C programs with
embedded SQL, they use log_min_duration_statement.Using this logging option,
SQL statement slower than a threshold will be displayed, but comments won't.
That's because the pre-compiler (ECPG) removes all the comments when the
pre-compiler converts C with embedded SQL to normal C code.
Without comments, DBA has difficulty with identifying to which C code
the slow query belongs. And the exact slow query issue cannot be reported
to the developer.
So, I'd like to modify ecpg command not to remove some specific comments.
[Use-cases]
Here is a scenario:
1) Writing comments to embedded C file
a) Describe the detailed usage of SQL in each comment.
b) Allocating id to each SQL.
- application developer need to create corresponding table between id
and the detailed description of SQL
2) DBA takes advantage of comments especially when:
a) Similar comments are displayed here and there.
In such a case, each comment plays a role as an identifier and makes it
easier for DBA to identify SQL he/she looking for.
b) DBA and C application developer are different.
DBA can tell an application developer which query is slow without mistakes.
[Interface]
add a new option "--enable-parse-comment" to ecpg command.
<usage> ecpg --enable-parse-comment ,..., prog.pgc
This option enables ecpg command to pass on block comments (/* 〜 */) to converted C file.
The conditions to enable processing "block comments" as follows:
- a block comment can be used with SELECT, INSERT, UPDATE, or DELETE
- a block comment can be placed right after keywords: SELECT, INSERT, UPDATE, DELETE or With
- other than those above error will occur
- line comment(--) are ignored, which is same as log output when logging libpq application
[Example]
1)[Correct comment position] this comment position is right after SELECT
EXEC SQL SELECT /* qid=3, at line 30 in yourApp.ecpg */ * INTO :C1, :C2 FROM T1 WHERE C1=1;
2)[Incorrect comment position] this comment position is bad(error will occur)
EXEC SQL /* qid=3, at line 30 in yourApp.ecpg */ SELECT * INTO :C1, :C2 FROM T1 WHERE C1=1;
As far as I searched, there seems no discussion on this topic.
Please let me know if exists.
Regards,
Okano Naoki
Fujitsu
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Hi,
So, I'd like to modify ecpg command not to remove some specific
comments.
...
[Interface]
add a new option "--enable-parse-comment" to ecpg command.
<usage> ecpg --enable-parse-comment ,..., prog.pgc
This option enables ecpg command to pass on block comments (/* 〜 */)
to converted C file.
The reason for not keeping the comments in the statement was simply to
make the parser simpler. If you added this feature, do we still see a
reason for keeping the old version? Or in other words, shouldn't we
make the "enable-parse-comment" version the default without a new
option?
Michael
--
Dr. Michael Meskes, Geschaeftsfuehrer/CEO
Tel.: +49 (0)2166 / 99 01 0
E-Mail: michael.meskes@credativ.com
IM: mme@jabber.credativ.com
PGP: BBBC 58B4 5994 CF9C CC56 BCDA DF23 DA33 9697 8EB3
credativ international GmbH, HRB Moenchengladbach 15543,
Trompeterallee 108, 41189 Moenchengladbach, Germany
Geschaeftsfuehrung: Dr. Michael Meskes, Joerg Folz
======================================
Global: http://credativ.com
Germany: http://credativ.de
Netherlands: http://credativ.nl
India: http://credativ.in
UK: http://credativ.co.uk
USA: http://credativ.us
======================================
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Hi,
Michael wrote:
The reason for not keeping the comments in the statement was simply to
make the parser simpler. If you added this feature, do we still see a
reason for keeping the old version? Or in other words, shouldn't we
make the "enable-parse-comment" version the default without a new
option?
Thank you for your feedback!
As I said in the first e-mail, there are some restrictions of comment position in my implementation. I am concerned that an error will occur in .pgc files users made in old version.
So, this feature should be a new option.
When the pre-compiler(ECPG) converts C with embedded SQL to normal C code, gram.y is used for syntactic analysis. I need to change gram.y for comments in SQL statement.
But I do not come up with better idea that gram.y is not affected.
If you are interested in my implementation in detail, please check the [WIP]patch I attached.
I am appreciated if you give me any idea or opinion.
Regards,
Okano Naoki
Fujitsu
Attachments:
[WIP]enable-parse-comment.patchapplication/octet-stream; name="[WIP]enable-parse-comment.patch"Download
diff --git a/doc/src/sgml/ref/ecpg-ref.sgml b/doc/src/sgml/ref/ecpg-ref.sgml
index 8bfb47c4d7..a6d38a3daa 100644
--- a/doc/src/sgml/ref/ecpg-ref.sgml
+++ b/doc/src/sgml/ref/ecpg-ref.sgml
@@ -95,6 +95,26 @@ PostgreSQL documentation
</varlistentry>
<varlistentry>
+ <term><option>--enable-parse-comment </option></term>
+ <listitem>
+ <para>
+ Enables the comment in SQL statements. If this option is not specified,
+ the comment will be removed as a result of the ecpg precompile and be
+ disabled. The SQL statements that can be specified in comment are
+ <command>INSERT</command>, <command>UPDATE</command>, and
+ <command>DELETE</command>. The locations in which the comment can
+ be specified are immediately after one of the <command>SELECT</command>,
+ <command>INSERT</command>, <command>UPDATE</command>, <command>DELETE</command>,
+ or <literal>WITH</literal> keywords. A syntax error will occur if any other
+ location is specified. An example keeping the comment in SQL statement.
+<programlisting>
+EXEC SQL SELECT /* qid=3, at line 30 in yourApp.ecpg */ * INTO :C1, :C2 FROM T1 WHERE C1=1;
+</programlisting>
+ </para>
+ </listitem>
+ </varlistentry>
+
+ <varlistentry>
<term><option>-i</option></term>
<listitem>
<para>
diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y
index e833b2eba5..31267d9098 100644
--- a/src/backend/parser/gram.y
+++ b/src/backend/parser/gram.y
@@ -574,7 +574,7 @@ static Node *makeRecursiveViewSelect(char *relname, List *aliases, Node *query);
%type <list> partbound_datum_list
%type <partrange_datum> PartitionRangeDatum
%type <list> range_datum_list
-
+%type <str> parse_comment
/*
* Non-keyword token types. These are hard-wired into the "flex" lexer.
* They must be listed first so that their numeric codes do not depend on
@@ -588,6 +588,7 @@ static Node *makeRecursiveViewSelect(char *relname, List *aliases, Node *query);
%token <ival> ICONST PARAM
%token TYPECAST DOT_DOT COLON_EQUALS EQUALS_GREATER
%token LESS_EQUALS GREATER_EQUALS NOT_EQUALS
+%token <str> SQL_PARSE_COMMENT
/*
* If you want to make any keyword changes, update the keyword table in
@@ -10314,14 +10315,14 @@ DeallocateStmt: DEALLOCATE name
*****************************************************************************/
InsertStmt:
- opt_with_clause INSERT INTO insert_target insert_rest
+ opt_with_clause INSERT parse_comment INTO insert_target insert_rest
opt_on_conflict returning_clause
{
- $5->relation = $4;
- $5->onConflictClause = $6;
- $5->returningList = $7;
- $5->withClause = $1;
- $$ = (Node *) $5;
+ $6->relation = $5;
+ $6->onConflictClause = $7;
+ $6->returningList = $8;
+ $6->withClause = $1;
+ $$ = (Node *) $6;
}
;
@@ -10445,14 +10446,14 @@ returning_clause:
*
*****************************************************************************/
-DeleteStmt: opt_with_clause DELETE_P FROM relation_expr_opt_alias
+DeleteStmt: opt_with_clause DELETE_P parse_comment FROM relation_expr_opt_alias
using_clause where_or_current_clause returning_clause
{
DeleteStmt *n = makeNode(DeleteStmt);
- n->relation = $4;
- n->usingClause = $5;
- n->whereClause = $6;
- n->returningList = $7;
+ n->relation = $5;
+ n->usingClause = $6;
+ n->whereClause = $7;
+ n->returningList = $8;
n->withClause = $1;
$$ = (Node *)n;
}
@@ -10514,18 +10515,18 @@ opt_nowait_or_skip:
*
*****************************************************************************/
-UpdateStmt: opt_with_clause UPDATE relation_expr_opt_alias
+UpdateStmt: opt_with_clause UPDATE parse_comment relation_expr_opt_alias
SET set_clause_list
from_clause
where_or_current_clause
returning_clause
{
UpdateStmt *n = makeNode(UpdateStmt);
- n->relation = $3;
- n->targetList = $5;
- n->fromClause = $6;
- n->whereClause = $7;
- n->returningList = $8;
+ n->relation = $4;
+ n->targetList = $6;
+ n->fromClause = $7;
+ n->whereClause = $8;
+ n->returningList = $9;
n->withClause = $1;
$$ = (Node *)n;
}
@@ -10767,33 +10768,33 @@ select_clause:
* However, this is not checked by the grammar; parse analysis must check it.
*/
simple_select:
- SELECT opt_all_clause opt_target_list
+ SELECT parse_comment opt_all_clause opt_target_list
into_clause from_clause where_clause
group_clause having_clause window_clause
{
SelectStmt *n = makeNode(SelectStmt);
- n->targetList = $3;
- n->intoClause = $4;
- n->fromClause = $5;
- n->whereClause = $6;
- n->groupClause = $7;
- n->havingClause = $8;
- n->windowClause = $9;
+ n->targetList = $4;
+ n->intoClause = $5;
+ n->fromClause = $6;
+ n->whereClause = $7;
+ n->groupClause = $8;
+ n->havingClause = $9;
+ n->windowClause = $10;
$$ = (Node *)n;
}
- | SELECT distinct_clause target_list
+ | SELECT parse_comment distinct_clause target_list
into_clause from_clause where_clause
group_clause having_clause window_clause
{
SelectStmt *n = makeNode(SelectStmt);
- n->distinctClause = $2;
- n->targetList = $3;
- n->intoClause = $4;
- n->fromClause = $5;
- n->whereClause = $6;
- n->groupClause = $7;
- n->havingClause = $8;
- n->windowClause = $9;
+ n->distinctClause = $3;
+ n->targetList = $4;
+ n->intoClause = $5;
+ n->fromClause = $6;
+ n->whereClause = $7;
+ n->groupClause = $8;
+ n->havingClause = $9;
+ n->windowClause = $10;
$$ = (Node *)n;
}
| values_clause { $$ = $1; }
@@ -10841,24 +10842,24 @@ simple_select:
* Recognizing WITH_LA here allows a CTE to be named TIME or ORDINALITY.
*/
with_clause:
- WITH cte_list
+ WITH parse_comment cte_list
{
$$ = makeNode(WithClause);
- $$->ctes = $2;
+ $$->ctes = $3;
$$->recursive = false;
$$->location = @1;
}
- | WITH_LA cte_list
+ | WITH_LA parse_comment cte_list
{
$$ = makeNode(WithClause);
- $$->ctes = $2;
+ $$->ctes = $3;
$$->recursive = false;
$$->location = @1;
}
- | WITH RECURSIVE cte_list
+ | WITH parse_comment RECURSIVE cte_list
{
$$ = makeNode(WithClause);
- $$->ctes = $3;
+ $$->ctes = $4;
$$->recursive = true;
$$->location = @1;
}
@@ -10885,6 +10886,11 @@ opt_with_clause:
| /*EMPTY*/ { $$ = NULL; }
;
+parse_comment:
+ SQL_PARSE_COMMENT { $$ = NULL; }
+ | /*EMPTY*/ { $$ = NULL; }
+ ;
+
into_clause:
INTO OptTempTableName
{
diff --git a/src/interfaces/ecpg/preproc/ecpg.addons b/src/interfaces/ecpg/preproc/ecpg.addons
index ca3efadc48..d8397ca412 100644
--- a/src/interfaces/ecpg/preproc/ecpg.addons
+++ b/src/interfaces/ecpg/preproc/ecpg.addons
@@ -324,6 +324,15 @@ ECPG: DeclareCursorStmtDECLAREcursor_namecursor_optionsCURSORopt_holdFORSelectSt
/* We put this text into a comment, so we better remove [*][/]. */
c2[0] = '.';
c2[1] = '.';
+
+ if (enable_parse_comment == true)
+ {
+ while ((c2 = strstr(c2, "*/")))
+ {
+ c2[0] = '.';
+ c2[1] = '.';
+ }
+ }
}
comment = cat_str(3, mm_strdup("/*"), c1, mm_strdup("*/"));
diff --git a/src/interfaces/ecpg/preproc/ecpg.c b/src/interfaces/ecpg/preproc/ecpg.c
index fa80bb289e..9a0978c976 100644
--- a/src/interfaces/ecpg/preproc/ecpg.c
+++ b/src/interfaces/ecpg/preproc/ecpg.c
@@ -18,7 +18,8 @@ bool autocommit = false,
force_indicator = true,
questionmarks = false,
regression_mode = false,
- auto_prepare = false;
+ auto_prepare = false,
+ enable_parse_comment = false;
char *output_filename;
@@ -54,6 +55,7 @@ help(const char *progname)
" \"no_indicator\", \"prepare\", \"questionmarks\"\n"));
printf(_(" --regression run in regression testing mode\n"));
printf(_(" -t turn on autocommit of transactions\n"));
+ printf(_(" --enable-parse-comment enable a block comment /* ... /* in the SQL statement\n"));
printf(_(" -V, --version output version information, then exit\n"));
printf(_(" -?, --help show this help, then exit\n"));
printf(_("\nIf no output file is specified, the name is formed by adding .c to the\n"
@@ -112,11 +114,13 @@ add_preprocessor_define(char *define)
}
#define ECPG_GETOPT_LONG_REGRESSION 1
+#define ECPG_GETOPT_LONG_ENABLE_PARSE_COMMENT 2
int
main(int argc, char *const argv[])
{
static struct option ecpg_options[] = {
{"regression", no_argument, NULL, ECPG_GETOPT_LONG_REGRESSION},
+ {"enable-parse-comment", no_argument, NULL, ECPG_GETOPT_LONG_ENABLE_PARSE_COMMENT},
{NULL, 0, NULL, 0}
};
@@ -238,6 +242,9 @@ main(int argc, char *const argv[])
progname);
#endif
break;
+ case ECPG_GETOPT_LONG_ENABLE_PARSE_COMMENT:
+ enable_parse_comment = true;
+ break;
default:
fprintf(stderr, _("Try \"%s --help\" for more information.\n"), argv[0]);
return ILLEGAL_OPTION;
diff --git a/src/interfaces/ecpg/preproc/ecpg.trailer b/src/interfaces/ecpg/preproc/ecpg.trailer
index 1c108795de..05519edd33 100644
--- a/src/interfaces/ecpg/preproc/ecpg.trailer
+++ b/src/interfaces/ecpg/preproc/ecpg.trailer
@@ -1766,6 +1766,8 @@ quoted_ident_stringvar: name
{ $$ = make3_str(mm_strdup("("), $1, mm_strdup(")")); }
;
+ecpg_parse_comment: SQL_PARSE_COMMENT { $$ = $1; } ;
+
/*
* C stuff
*/
diff --git a/src/interfaces/ecpg/preproc/ecpg.type b/src/interfaces/ecpg/preproc/ecpg.type
index ac6aa000ac..5fac6cc1a9 100644
--- a/src/interfaces/ecpg/preproc/ecpg.type
+++ b/src/interfaces/ecpg/preproc/ecpg.type
@@ -66,6 +66,7 @@
%type <str> ecpg_sconst
%type <str> ecpg_using
%type <str> ecpg_xconst
+%type <str> ecpg_parse_comment
%type <str> enum_definition
%type <str> enum_type
%type <str> execstring
@@ -143,3 +144,5 @@
%type <type> var_type
%type <action> action
+
+%type <str> SQL_PARSE_COMMENT
diff --git a/src/interfaces/ecpg/preproc/extern.h b/src/interfaces/ecpg/preproc/extern.h
index 0ce3a6e424..8c1c035a32 100644
--- a/src/interfaces/ecpg/preproc/extern.h
+++ b/src/interfaces/ecpg/preproc/extern.h
@@ -25,7 +25,8 @@ extern bool autocommit,
force_indicator,
questionmarks,
regression_mode,
- auto_prepare;
+ auto_prepare,
+ enable_parse_comment;
extern int braces_open,
ret_value,
struct_level,
diff --git a/src/interfaces/ecpg/preproc/parse.pl b/src/interfaces/ecpg/preproc/parse.pl
index 8a401304ec..18d315fcbf 100644
--- a/src/interfaces/ecpg/preproc/parse.pl
+++ b/src/interfaces/ecpg/preproc/parse.pl
@@ -38,7 +38,8 @@ my %replace_token = (
'FCONST' => 'ecpg_fconst',
'Sconst' => 'ecpg_sconst',
'IDENT' => 'ecpg_ident',
- 'PARAM' => 'ecpg_param',);
+ 'PARAM' => 'ecpg_param',
+ 'SQL_PARSE_COMMENT' => 'ecpg_parse_comment',);
# or in the block
my %replace_string = (
diff --git a/src/interfaces/ecpg/preproc/pgc.l b/src/interfaces/ecpg/preproc/pgc.l
index b894a33e53..d6a91fe90a 100644
--- a/src/interfaces/ecpg/preproc/pgc.l
+++ b/src/interfaces/ecpg/preproc/pgc.l
@@ -117,6 +117,7 @@ static struct _if_value
* <xdolq> $foo$ quoted strings
* <xui> quoted identifier with Unicode escapes
* <xus> quoted string with Unicode escapes
+ * <xpc> enable parser comments in SQL statement
*/
%x xb
@@ -134,6 +135,7 @@ static struct _if_value
%x xskip
%x xui
%x xus
+%x xpc
/* Bit string
*/
@@ -382,6 +384,37 @@ cppline {space}*#([^i][A-Za-z]*|{if}|{ifdef}|{ifndef}|{import})((\/\*[^*/]*\*+
<SQL>{whitespace} { /* ignore */ }
+<xpc>{xcstart} {
+ xcdepth++;
+ /* Put back any characters past slash-star; see above */
+ yyless(2);
+ addlit(yytext, yyleng);
+ }
+
+<xpc>{xcstop} {
+ addlit(yytext, yyleng);
+ if (xcdepth <= 0)
+ {
+ BEGIN(state_before);
+ token_start = NULL;
+ base_yylval.str = mm_strdup(literalbuf);
+ return SQL_PARSE_COMMENT;
+ }
+ else
+ {
+ xcdepth--;
+ }
+ }
+
+<xpc>{xcinside} {
+ addlit(yytext, yyleng);
+ }
+<xpc>{op_chars} {
+ addlit(yytext, yyleng);
+ }
+
+<xpc><<EOF>> { mmfatal(PARSE_ERROR, "unterminated /* comment in SQL statement"); }
+
<C>{xcstart} {
token_start = yytext;
state_before = YYSTATE;
@@ -392,13 +425,26 @@ cppline {space}*#([^i][A-Za-z]*|{if}|{ifdef}|{ifndef}|{import})((\/\*[^*/]*\*+
fputs("/*", yyout);
}
<SQL>{xcstart} {
- token_start = yytext;
- state_before = YYSTATE;
- xcdepth = 0;
- BEGIN(xcsql);
- /* Put back any characters past slash-star; see above */
- yyless(2);
- fputs("/*", yyout);
+ if(enable_parse_comment == true)
+ {
+ token_start = yytext;
+ state_before = YYSTATE;
+ xcdepth = 0;
+ BEGIN(xpc);
+ yyless(3);
+ startlit();
+ addlit(yytext, yyleng);
+ }
+ else
+ {
+ token_start = yytext;
+ state_before = YYSTATE;
+ xcdepth = 0;
+ BEGIN(xcsql);
+ /* Put back any characters past slash-star; see above */
+ yyless(2);
+ fputs("/*", yyout);
+ }
}
<xcc>{xcstart} { ECHO; }
<xcsql>{xcstart} {
As I said in the first e-mail, there are some restrictions of comment
position in my implementation. I am concerned that an error will
occur in .pgc files users made in old version.
So, this feature should be a new option.
I see, this makes sense.
When the pre-compiler(ECPG) converts C with embedded SQL to normal C
code, gram.y is used for syntactic analysis. I need to change gram.y
for comments in SQL statement.
But I do not come up with better idea that gram.y is not affected.
If you are interested in my implementation in detail, please check
the [WIP]patch I attached.
I'm not sure we would want to change the backend parser for something
only used in ecpg. Actually I'm pretty sure we don't.
I can see two possible solutions. One would be to replace the parser
rules. Please see parse.pl for details. Some rules are already replaced
by ecpg specific ones. However, the more rules we replace the more
manual syncing effort we need for changes in gram.y.
The other option I can see, albeit without looking into details, is
allowing all comments and then testing it for the right syntax after
parsing. This could potentially also solve the above mentioned option
problem.
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
Thank you for your kind advise!
Michael Meskes wrote:
The other option I can see, albeit without looking into details, is
allowing all comments and then testing it for the right syntax after
parsing. This could potentially also solve the above mentioned option
problem.
This idea sounds great. But I am not sure that I can understand it correctly.
I understood the concept of this idea as following. Is it right?
1. The parser ignores comments/*...*/ as usual. That is, parser does not
identify comments as a token.
2. After parsing, we parse again only to extract comments.
3. comments are put back or forth in SQL statement.
Regards,
Okano Naoki
Fujitsu
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Michael Meskes wrote:
The other option I can see, albeit without looking into details, is
allowing all comments and then testing it for the right syntax
after
parsing. This could potentially also solve the above mentioned
option
problem.This idea sounds great. But I am not sure that I can understand it
correctly.I understood the concept of this idea as following. Is it right?
1. The parser ignores comments/*...*/ as usual. That is, parser does
not
identify comments as a token.
I guess it'd be easier to accept each comment as a token and then parse
the token text afterwards.
2. After parsing, we parse again only to extract comments.
Not sure if we can do that without creating a lot of overhead.
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
Michael Meskes wrote:
Michael Meskes wrote:
The other option I can see, albeit without looking into details, is
allowing all comments and then testing it for the right syntax after
parsing. This could potentially also solve the above mentioned
option problem.This idea sounds great. But I am not sure that I can understand it
correctly.I understood the concept of this idea as following. Is it right?
1. The parser ignores comments/*...*/ as usual. That is, parser does
not
identify comments as a token.I guess it'd be easier to accept each comment as a token and then parse the token
text afterwards.2. After parsing, we parse again only to extract comments.
Not sure if we can do that without creating a lot of overhead.
I see. Based on your advice, I try to make a patch.
I will attach a patch when I finish it.
Regards,
Okano Naoki
Fujitsu
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers