Re: [PATCHES] defer statement logging until after parse
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 ---(end of broadcast)--- TIP 8: explain analyze is your friend
Re: [PATCHES] defer statement logging until after parse
Tom Lane wrote: > Andrew Dunstan <[EMAIL PROTECTED]> 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 [EMAIL PROTECTED] | (610) 359-1001 + If your life is a hard drive, | 13 Roberts Road + Christ can be your backup.| Newtown Square, Pennsylvania 19073 ---(end of broadcast)--- TIP 7: don't forget to increase your free space map settings
Re: [PATCHES] defer statement logging until after parse
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 [EMAIL PROTECTED] | (610) 359-1001 + If your life is a hard drive, | 13 Roberts Road + Christ can be your backup.| Newtown Square, Pennsylvania 19073 ---(end of broadcast)--- TIP 5: Have you checked our extensive FAQ? http://www.postgresql.org/docs/faqs/FAQ.html
Re: [PATCHES] defer statement logging until after parse
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 ---(end of broadcast)--- TIP 8: explain analyze is your friend
Re: [PATCHES] defer statement logging until after parse
Andrew Dunstan <[EMAIL PROTECTED]> 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 ---(end of broadcast)--- TIP 4: Don't 'kill -9' the postmaster
Re: [PATCHES] defer statement logging until after parse
Bruce Momjian <[EMAIL PROTECTED]> 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 ---(end of broadcast)--- TIP 1: subscribe and unsubscribe commands go to [EMAIL PROTECTED]
Re: [PATCHES] defer statement logging until after parse
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 ---(end of broadcast)--- TIP 8: explain analyze is your friend
Re: [PATCHES] defer statement logging until after parse
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 [EMAIL PROTECTED] | (610) 359-1001 + If your life is a hard drive, | 13 Roberts Road + Christ can be your backup.| Newtown Square, Pennsylvania 19073 ---(end of broadcast)--- TIP 5: Have you checked our extensive FAQ? http://www.postgresql.org/docs/faqs/FAQ.html
Re: [PATCHES] defer statement logging until after parse
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 ---(end of broadcast)--- TIP 6: Have you searched our list archives? http://archives.postgresql.org
Re: [PATCHES] defer statement logging until after parse
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 - 1.394 > --- src/backend/tcop/postgres.c 11 Mar 2004 16:31:10 - > *** > *** 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 [EMAIL PROTECTED]) -- Bruce Momjian| http://candle.pha.pa.us [EMAIL PROTECTED] | (610) 359-1001 + If your life is a hard drive, | 13 Roberts Road + Christ can be your backup.| Newtown Square, Pennsylvania 19073 ---(end of broadcast)--- TIP 2: you can get off all lists at once with the unregister command (send "unregister YourEmailAddressHere" to [EMAIL PROTECTED])
[PATCHES] 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 - 1.394 --- src/backend/tcop/postgres.c 11 Mar 2004 16:31:10 - *** *** 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 [EMAIL PROTECTED])