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 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 3. test should be enhanced for optional parameters Regards Pavel > >> Thanks in advance. >> >> Regards, >> Dinesh >> >>> >>> Regards >>> >>> Pavel >>> >>> >>>> >>>> Regards, >>>> Dinesh >>>> manojadinesh.blogspot.com >>>> >>> >>> >> >
