Обсуждение: defer statement logging until after parse
The attached patch is for review, not application. It defers logging
statements until after they have been parsed. There should be no
observable difference in behaviour if there is a successful parse, and
on error the following traces appear:
line:3 ERROR: syntax error at or near "se3d2" at character 1
line:4 LOG: parse error in statement: se3d2 aaa;
Basically, I want to know that this will not break anything, and if so I
will use it as the basis for a selective statement logging facility,
based on the command tag(s) of the parsed statement(s), and
incorporating Greg Stark's suggesion of a "syntax error" logging level.
cheers
andrew
Index: src/backend/tcop/postgres.c
===================================================================
RCS file: /projects/cvsroot/pgsql-server/src/backend/tcop/postgres.c,v
retrieving revision 1.394
diff -c -r1.394 postgres.c
*** src/backend/tcop/postgres.c 9 Mar 2004 04:43:07 -0000 1.394
--- src/backend/tcop/postgres.c 11 Mar 2004 16:31:10 -0000
***************
*** 118,123 ****
--- 118,128 ----
static bool ignore_till_sync = false;
/*
+ * place to save the input pointer in case of a parse error
+ */
+ static char *parse_input_string = NULL;
+
+ /*
* If an unnamed prepared statement exists, it's stored here.
* We keep it separate from the hashtable kept by commands/prepare.c
* in order to reduce overhead for short-lived queries.
***************
*** 462,476 ****
{
List *raw_parsetree_list;
- if (log_statement)
- ereport(LOG,
- (errmsg("statement: %s", query_string)));
-
if (log_parser_stats)
ResetUsage();
raw_parsetree_list = raw_parser(query_string);
if (log_parser_stats)
ShowUsage("PARSER STATISTICS");
--- 467,487 ----
{
List *raw_parsetree_list;
if (log_parser_stats)
ResetUsage();
+ parse_input_string = query_string;
+
raw_parsetree_list = raw_parser(query_string);
+ /* if we get here there was no parse error */
+
+ parse_input_string = NULL;
+
+ if (log_statement)
+ ereport(LOG,
+ (errmsg("statement: %s", query_string)));
+
if (log_parser_stats)
ShowUsage("PARSER STATISTICS");
***************
*** 2747,2752 ****
--- 2758,2775 ----
send_rfq = true; /* initially, or after error */
/*
+ * if parse_input_string is not null, we must have got here through a
+ * parser error, in which case we log it if appropriate.
+ */
+
+ if (log_statement && parse_input_string != NULL)
+ ereport(LOG,
+ (errmsg("parse error in statement: %s", parse_input_string)));
+
+ parse_input_string = NULL;
+
+
+ /*
* Non-error queries loop here.
*/
The problem I see with this patch is that it doesn't print the error
query on a syntax error. That seems wrong.
I think you should print the query before parsing if they are asking for
all queries to be logged, and print them after parsing if they want only
DDL or DDL and data modification queries. I realize that duplicates
some function calls, but I don't see any other way.
---------------------------------------------------------------------------
Andrew Dunstan wrote:
>
> The attached patch is for review, not application. It defers logging
> statements until after they have been parsed. There should be no
> observable difference in behaviour if there is a successful parse, and
> on error the following traces appear:
>
> line:3 ERROR: syntax error at or near "se3d2" at character 1
> line:4 LOG: parse error in statement: se3d2 aaa;
>
> Basically, I want to know that this will not break anything, and if so I
> will use it as the basis for a selective statement logging facility,
> based on the command tag(s) of the parsed statement(s), and
> incorporating Greg Stark's suggesion of a "syntax error" logging level.
>
> cheers
>
> andrew
>
> Index: src/backend/tcop/postgres.c
> ===================================================================
> RCS file: /projects/cvsroot/pgsql-server/src/backend/tcop/postgres.c,v
> retrieving revision 1.394
> diff -c -r1.394 postgres.c
> *** src/backend/tcop/postgres.c 9 Mar 2004 04:43:07 -0000 1.394
> --- src/backend/tcop/postgres.c 11 Mar 2004 16:31:10 -0000
> ***************
> *** 118,123 ****
> --- 118,128 ----
> static bool ignore_till_sync = false;
>
> /*
> + * place to save the input pointer in case of a parse error
> + */
> + static char *parse_input_string = NULL;
> +
> + /*
> * If an unnamed prepared statement exists, it's stored here.
> * We keep it separate from the hashtable kept by commands/prepare.c
> * in order to reduce overhead for short-lived queries.
> ***************
> *** 462,476 ****
> {
> List *raw_parsetree_list;
>
> - if (log_statement)
> - ereport(LOG,
> - (errmsg("statement: %s", query_string)));
> -
> if (log_parser_stats)
> ResetUsage();
>
> raw_parsetree_list = raw_parser(query_string);
>
> if (log_parser_stats)
> ShowUsage("PARSER STATISTICS");
>
> --- 467,487 ----
> {
> List *raw_parsetree_list;
>
> if (log_parser_stats)
> ResetUsage();
>
> + parse_input_string = query_string;
> +
> raw_parsetree_list = raw_parser(query_string);
>
> + /* if we get here there was no parse error */
> +
> + parse_input_string = NULL;
> +
> + if (log_statement)
> + ereport(LOG,
> + (errmsg("statement: %s", query_string)));
> +
> if (log_parser_stats)
> ShowUsage("PARSER STATISTICS");
>
> ***************
> *** 2747,2752 ****
> --- 2758,2775 ----
> send_rfq = true; /* initially, or after error */
>
> /*
> + * if parse_input_string is not null, we must have got here through a
> + * parser error, in which case we log it if appropriate.
> + */
> +
> + if (log_statement && parse_input_string != NULL)
> + ereport(LOG,
> + (errmsg("parse error in statement: %s", parse_input_string)));
> +
> + parse_input_string = NULL;
> +
> +
> + /*
> * Non-error queries loop here.
> */
>
>
> ---------------------------(end of broadcast)---------------------------
> TIP 2: you can get off all lists at once with the unregister command
> (send "unregister YourEmailAddressHere" to majordomo@postgresql.org)
--
Bruce Momjian | http://candle.pha.pa.us
pgman@candle.pha.pa.us | (610) 359-1001
+ If your life is a hard drive, | 13 Roberts Road
+ Christ can be your backup. | Newtown Square, Pennsylvania 19073
Bruce Momjian wrote: >The problem I see with this patch is that it doesn't print the error >query on a syntax error. That seems wrong. > It does print it. In fact the example I gave below which is from a real trace shows it being printed. It is just printed after the error message rather than before. You solution doesn't appear to address the problem of what to do if they ask for only DDL and one of those generates a syntax error. cheers andrew > >I think you should print the query before parsing if they are asking for >all queries to be logged, and print them after parsing if they want only >DDL or DDL and data modification queries. I realize that duplicates >some function calls, but I don't see any other way. > >--------------------------------------------------------------------------- > >Andrew Dunstan wrote: > > >>The attached patch is for review, not application. It defers logging >>statements until after they have been parsed. There should be no >>observable difference in behaviour if there is a successful parse, and >>on error the following traces appear: >> >> line:3 ERROR: syntax error at or near "se3d2" at character 1 >> line:4 LOG: parse error in statement: se3d2 aaa; >> >>Basically, I want to know that this will not break anything, and if so I >>will use it as the basis for a selective statement logging facility, >>based on the command tag(s) of the parsed statement(s), and >>incorporating Greg Stark's suggesion of a "syntax error" logging level. >> >>cheers >> >>andrew >> >> >> > > > >
Andrew Dunstan wrote: > > > Bruce Momjian wrote: > > >The problem I see with this patch is that it doesn't print the error > >query on a syntax error. That seems wrong. > > > > It does print it. In fact the example I gave below which is from a real > trace shows it being printed. It is just printed after the error message > rather than before. > > You solution doesn't appear to address the problem of what to do if they > ask for only DDL and one of those generates a syntax error. My comment was that if they type "UP8ATE", and it is a syntax error, we have no way to know if it was a DDL or not, so we don't print it. My idea was to take log_statement, and instead of true/false, have it be all, ddl, mod, or off/none/false(?). You keep the existing test for log_statement where it is, but test for 'all' now, and after parse, you check for ddl or mod, and print in those cases if the tag matches. If they want ddl and errors, they can use log_min_error_statement to see just statement error, and set log_statement accordingly. -- Bruce Momjian | http://candle.pha.pa.us pgman@candle.pha.pa.us | (610) 359-1001 + If your life is a hard drive, | 13 Roberts Road + Christ can be your backup. | Newtown Square, Pennsylvania 19073
Bruce Momjian said: > Andrew Dunstan wrote: >> >> >> Bruce Momjian wrote: >> >> >The problem I see with this patch is that it doesn't print the error >> >query on a syntax error. That seems wrong. >> > >> >> It does print it. In fact the example I gave below which is from a >> real trace shows it being printed. It is just printed after the error >> message rather than before. >> >> You solution doesn't appear to address the problem of what to do if >> they ask for only DDL and one of those generates a syntax error. > > My comment was that if they type "UP8ATE", and it is a syntax error, we > have no way to know if it was a DDL or not, so we don't print it. > > My idea was to take log_statement, and instead of true/false, have it > be all, ddl, mod, or off/none/false(?). You keep the existing test for > log_statement where it is, but test for 'all' now, and after parse, you > check for ddl or mod, and print in those cases if the tag matches. > > If they want ddl and errors, they can use log_min_error_statement to > see just statement error, and set log_statement accordingly. > The problem is that you are anticipating my solution for the selectivity issue before I have written or submitted it. My question was different and narrower - namely will the patch I sent, as it stands, and forgetting the selectivity issue for the moment, break anything? When I actually send in a patch to implement statement log selectivity, I will give you free license to pull it to bits to your heart's content. cheers andrew
Bruce Momjian <pgman@candle.pha.pa.us> writes:
> My idea was to take log_statement, and instead of true/false, have it be
> all, ddl, mod, or off/none/false(?). You keep the existing test for
> log_statement where it is, but test for 'all' now, and after parse, you
> check for ddl or mod, and print in those cases if the tag matches.
Has any of this discussion taken into account the fact that a
querystring may contain multiple commands?
regards, tom lane
Tom Lane wrote: >Bruce Momjian <pgman@candle.pha.pa.us> writes: > > >>My idea was to take log_statement, and instead of true/false, have it be >>all, ddl, mod, or off/none/false(?). You keep the existing test for >>log_statement where it is, but test for 'all' now, and after parse, you >>check for ddl or mod, and print in those cases if the tag matches. >> >> > >Has any of this discussion taken into account the fact that a >querystring may contain multiple commands? > > > It is certainly in my mind - hence the array of parse trees, no? What does the parser do if one of the statements has an error and the others are OK? I was (perhaps unwarrantedly) assuming that if you got a clean return from the parser then none of the statements had a parse error. cheers andrew
Andrew Dunstan <andrew@dunslane.net> writes:
> Tom Lane wrote:
>> Has any of this discussion taken into account the fact that a
>> querystring may contain multiple commands?
> What does the parser do if one of the statements has an error and the
> others are OK?
The whole thing is rejected. This is just an instance of the general
rule that processing of the entire querystring is abandoned at the first
error.
The current definition of log_statement has no problem because we print
the whole string, once, before parsing starts. If you put a printout
into the per-parse-tree loop then I think you are going to get multiple
printouts of the same string. You could add some state to prevent more
than one printout per querystring, but even then you'll get complaints
"I asked for DDL only, why did it print this SELECT?". ISTM the only
way to make it "work" without obvious implementation artifacts is to
actually break down the string into individual commands, which is more
work than I think this feature is worth.
regards, tom lane
Tom Lane wrote: > >The current definition of log_statement has no problem because we print >the whole string, once, before parsing starts. If you put a printout >into the per-parse-tree loop then I think you are going to get multiple >printouts of the same string. > I didn't intend to - I intended to do it before that. All or nothing deal. e.g. if they want DML then if any of the queries has insert/update/delete/copy they get the whole query string. >You could add some state to prevent more >than one printout per querystring, but even then you'll get complaints >"I asked for DDL only, why did it print this SELECT?". ISTM the only >way to make it "work" without obvious implementation artifacts is to >actually break down the string into individual commands, which is more >work than I think this feature is worth. > > > Well, it gets worse than that when you consider that I tend to put all my DML inside a stored proc and have the client call "select myproc(args)". However, people have asked for a facility to filter out stuff, especially to filter out select statements they are not interested in. I agree that it is simpleminded, and I wouldn't use it. But we can put warnings about likely effects in the docs. Or we can abandon the whole idea and remove it from the TODO list. I'm not burning up with desire to do this. cheers andrew
Andrew Dunstan wrote: > >> It does print it. In fact the example I gave below which is from a > >> real trace shows it being printed. It is just printed after the error > >> message rather than before. > >> > >> You solution doesn't appear to address the problem of what to do if > >> they ask for only DDL and one of those generates a syntax error. > > > > My comment was that if they type "UP8ATE", and it is a syntax error, we > > have no way to know if it was a DDL or not, so we don't print it. > > > > My idea was to take log_statement, and instead of true/false, have it > > be all, ddl, mod, or off/none/false(?). You keep the existing test for > > log_statement where it is, but test for 'all' now, and after parse, you > > check for ddl or mod, and print in those cases if the tag matches. > > > > If they want ddl and errors, they can use log_min_error_statement to > > see just statement error, and set log_statement accordingly. > > > > The problem is that you are anticipating my solution for the selectivity > issue before I have written or submitted it. My question was different and > narrower - namely will the patch I sent, as it stands, and forgetting the > selectivity issue for the moment, break anything? > > When I actually send in a patch to implement statement log selectivity, I > will give you free license to pull it to bits to your heart's content. Well, if that is the question, then I don't want to reorder the query printout from the error. -- Bruce Momjian | http://candle.pha.pa.us pgman@candle.pha.pa.us | (610) 359-1001 + If your life is a hard drive, | 13 Roberts Road + Christ can be your backup. | Newtown Square, Pennsylvania 19073
Tom Lane wrote: > Andrew Dunstan <andrew@dunslane.net> writes: > > Tom Lane wrote: > >> Has any of this discussion taken into account the fact that a > >> querystring may contain multiple commands? > > > What does the parser do if one of the statements has an error and the > > others are OK? > > The whole thing is rejected. This is just an instance of the general > rule that processing of the entire querystring is abandoned at the first > error. > > The current definition of log_statement has no problem because we print > the whole string, once, before parsing starts. If you put a printout > into the per-parse-tree loop then I think you are going to get multiple > printouts of the same string. You could add some state to prevent more > than one printout per querystring, but even then you'll get complaints > "I asked for DDL only, why did it print this SELECT?". ISTM the only > way to make it "work" without obvious implementation artifacts is to > actually break down the string into individual commands, which is more > work than I think this feature is worth. I think it is acceptable to print the entire query string once if one part of it has a DDL or mod statement. -- Bruce Momjian | http://candle.pha.pa.us pgman@candle.pha.pa.us | (610) 359-1001 + If your life is a hard drive, | 13 Roberts Road + Christ can be your backup. | Newtown Square, Pennsylvania 19073
Bruce Momjian wrote: >Well, if that is the question, then I don't want to reorder the query >printout from the error. > > > OK. I'll let someone else do it. I have no need for it. Forget I spoke. cheers andrew