2015-09-05 8:35 GMT+02:00 dinesh kumar <dineshkuma...@gmail.com>: > > > On Fri, Sep 4, 2015 at 2:03 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. >>> >>> 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 >> >> > Fixed It. >
it should be a optional parameter, please fix doc, there are not any difference between mandatory and optional parametere > > >> 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. >> > > OK. Shall i remove the context field from this function. > ok > > Regards, > Dinesh > manojadinesh.blogspot.com > > Regards >> >> Pavel >> >> >>> Regards, >>> Dinesh >>> manojadinesh.blogspot.com >>> >>> Regards >>>> >>>> Pavel >>>> >>>> >>>>> >>>>>> Thanks in advance. >>>>>> >>>>>> Regards, >>>>>> Dinesh >>>>>> >>>>>>> >>>>>>> Regards >>>>>>> >>>>>>> Pavel >>>>>>> >>>>>>> >>>>>>>> >>>>>>>> Regards, >>>>>>>> Dinesh >>>>>>>> manojadinesh.blogspot.com >>>>>>>> >>>>>>> >>>>>>> >>>>>> >>>>> >>>> >>> >> >
