On Fri, Sep 4, 2015 at 1:08 PM, Pavel Stehule <pavel.steh...@gmail.com> wrote:
> > > 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. >> > > you have a true - in this case we can use YYTEXT - so the code can be some > like > > if (tok != ';') > { > if (parse_elog_level(yytext, &elog_level) > { > ... > } > } > > but it means double string comparation, what is not good, or removing elog > levels from keyword list (what is surely out of area of this patch). So > using it in PLpgSQL was not practical idea. I am sorry. > > Hi, No Worries. Thanks a lot for your guidance on this patch. Regards, Dinesh manojadinesh.blogspot.com > Regards > > Pavel > > >> 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. >> >> Regards, >> Dinesh >> manojadinesh.blogspot.com >> >> Regards >>> >>> Pavel >>> >>> >>>> >>>>> Thanks in advance. >>>>> >>>>> Regards, >>>>> Dinesh >>>>> >>>>>> >>>>>> Regards >>>>>> >>>>>> Pavel >>>>>> >>>>>> >>>>>>> >>>>>>> Regards, >>>>>>> Dinesh >>>>>>> manojadinesh.blogspot.com >>>>>>> >>>>>> >>>>>> >>>>> >>>> >>> >> >
