2015-09-02 21:49 GMT+02:00 dinesh kumar <dineshkuma...@gmail.com>: > On Mon, Aug 31, 2015 at 10:08 PM, Pavel Stehule <pavel.steh...@gmail.com> > wrote: > >> >> >> 2015-09-01 6:59 GMT+02:00 Pavel Stehule <pavel.steh...@gmail.com>: >> >>> >>> >>> 2015-08-31 20:43 GMT+02:00 dinesh kumar <dineshkuma...@gmail.com>: >>> >>>> Hi, >>>> >>>> On Sat, Aug 29, 2015 at 4:22 PM, Pavel Stehule <pavel.steh...@gmail.com >>>> > wrote: >>>> >>>>> Hi >>>>> >>>>> I am starting to work review of this patch >>>>> >>>>> 2015-07-13 9:54 GMT+02:00 dinesh kumar <dineshkuma...@gmail.com>: >>>>> >>>>>> Hi All, >>>>>> >>>>>> Greetings for the day. >>>>>> >>>>>> Would like to discuss on below feature here. >>>>>> >>>>>> Feature: >>>>>> Having an SQL function, to write messages to log destination. >>>>>> >>>>>> Justification: >>>>>> As of now, we don't have an SQL function to write >>>>>> custom/application messages to log destination. We have "RAISE" clause >>>>>> which is controlled by >>>>>> log_ parameters. If we have an SQL function which works irrespective >>>>>> of log settings, that would be a good for many log parsers. What i mean >>>>>> is, >>>>>> in DBA point of view, if we route all our native OS stats to log files >>>>>> in a >>>>>> proper format, then we can have our log reporting tools to give most >>>>>> effective reports. Also, Applications can log their own messages to >>>>>> postgres log files, which can be monitored by DBAs too. >>>>>> >>>>>> Implementation: >>>>>> Implemented a new function "pg_report_log" which takes one >>>>>> argument as text, and returns void. I took, "LOG" prefix for all the >>>>>> reporting messages.I wasn't sure to go with new prefix for this, since >>>>>> these are normal LOG messages. Let me know, if i am wrong here. >>>>>> >>>>>> Here is the attached patch. >>>>>> >>>>> >>>>> This patch is not complex, but the implementation doesn't cover a >>>>> "ereport" well. >>>>> >>>>> Although this functionality should be replaced by custom function in >>>>> any PL (now or near future), I am not against to have this function in >>>>> core. There are lot of companies with strong resistance against stored >>>>> procedures - and sometimes this functionality can help with SQL debugging. >>>>> >>>>> Issues: >>>>> >>>>> 1. Support only MESSAGE field in exception - I am expecting to support >>>>> all fields: HINT, DETAIL, ... >>>>> >>>> >>>> Added these functionalities too. >>>> >>>> >>>>> 2. Missing regress tests >>>>> >>>> >>>> Adding here. >>>> >>>> >>>>> 3. the parsing ereport level should be public function shared with >>>>> PLpgSQL and other PL >>>>> >>>> >>>> Sorry Pavel. I am not getting your point here. Would you give me an >>>> example. >>>> >>> >>> The transformation: text -> error level is common task - and PLpgSQL it >>> does in pl_gram.y. My idea is to add new function to error utils named >>> "parse_error_level" and use it from PLpgSQL and from your code. >>> >>> >>>> >>>> >>>>> 4. should be hidestmt mandatory parameter? >>>>> >>>> >>>> I changed this argument's default value as "true". >>>> >>>> >>>>> 5. the function declaration is strange >>>>> >>>>> postgres=# \sf pg_report_log (text, anyelement, boolean) >>>>> CREATE OR REPLACE FUNCTION pg_catalog.pg_report_log(text, anyelement, >>>>> boolean) >>>>> RETURNS void >>>>> LANGUAGE sql >>>>> STABLE STRICT COST 1 >>>>> AS $function$SELECT pg_report_log($1::pg_catalog.text, >>>>> $2::pg_catalog.text, $3::boolean)$function$ >>>>> >>>>> Why polymorphic? It is useless on any modern release >>>>> >>>>> >>>> I took quote_ident(anyelement) as referral code, for implementing this. >>>> Could you guide me if I am doing wrong here. >>>> >>> >>> I was wrong - this is ok - it is emulation of force casting to text >>> >>> >>>> >>>> >>>>> postgres=# \sf pg_report_log (text, text, boolean) >>>>> CREATE OR REPLACE FUNCTION pg_catalog.pg_report_log(text, text, >>>>> boolean) >>>>> RETURNS void >>>>> LANGUAGE internal >>>>> IMMUTABLE STRICT >>>>> AS $function$pg_report_log$function$ >>>>> >>>>> Why stable, why immutable? This function should be volatile. >>>>> >>>>> Fixed these to volatile. >>>> >>>> >>>>> 6. using elog level enum as errcode is wrong idea - errcodes are >>>>> defined in table >>>>> http://www.postgresql.org/docs/9.4/static/errcodes-appendix.html >>>>> >>>> >>>> You mean, if the elevel is 'ERROR', then we need to allow errcode. Let >>>> me know your inputs. >>>> >>> >>> I was blind, but the code was not good. Yes, error and higher needs >>> error code. From ANSI/SQL anything can has error code "00 is ok", "01 .. >>> warnings" ... >>> >>> There is more possibilities - look to PLpgSQL implementation - it can be >>> optional parameter - it default can use ERRCODE_RAISE_EXCEPTION >>> >>> >>>> >>>> Adding new patch, with the above fixes. >>>> >>> >> the code looks better >> >> my objections: >> >> 1. I prefer default values be NULL >> > > Fixed it. > > >> 2. readability: >> * parsing error level should be in alone cycle >> * you don't need to use more ereport calls - one is good enough - look >> on implementation of stmt_raise in PLpgSQL >> > > Sorry for my ignorance. I have tried to implement parse_error_level in > pl_gram.y, but not able to do it. I was not able to parse the given string > with tokens, and return the error levels. I tried for a refferal code, but > not able to find any. Would you guide me on this. > > In this attached patch, I have minimized the ereport calls. Kindly check > and let me know. > > >> 3. test should be enhanced for optional parameters >> >> Fixed it. >
only few points: 1. missing to set errstate - any exception should to have some errcode value. There can be default like PLpgSQL ERRCODE_RAISE_EXCEPTION for any where elog_level >= error 2. the explicit setting context is not consistent with our PL - the context is implicit value only - not possible to set it explicitly. The behave of this field is not clear - but in this moment, the context is related to PostgreSQL area - not to application area. Regards Pavel > Regards, > Dinesh > manojadinesh.blogspot.com > > Regards >> >> Pavel >> >> >>> >>>> Thanks in advance. >>>> >>>> Regards, >>>> Dinesh >>>> >>>>> >>>>> Regards >>>>> >>>>> Pavel >>>>> >>>>> >>>>>> >>>>>> Regards, >>>>>> Dinesh >>>>>> manojadinesh.blogspot.com >>>>>> >>>>> >>>>> >>>> >>> >> >