Re: [PATCH] SQL function to report log message

On Mon, Aug 31, 2015 at 10:08 PM, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com>
wrote:

>
>
> 2015-09-01 6:59 GMT+02:00 Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com>:
>
>>
>>
>> 2015-08-31 20:43 GMT+02:00 dinesh kumar <dineshkumar02(at)gmail(dot)com>:
>>
>>> Hi,
>>>
>>> On Sat, Aug 29, 2015 at 4:22 PM, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com>
>>> wrote:
>>>
>>>> Hi
>>>>
>>>> I am starting to work review of this patch
>>>>
>>>> 2015-07-13 9:54 GMT+02:00 dinesh kumar <dineshkumar02(at)gmail(dot)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.

Regards,
Dinesh
manojadinesh.blogspot.com

Regards
>
> Pavel
>
>
>>
>>> Thanks in advance.
>>>
>>> Regards,
>>> Dinesh
>>>
>>>>
>>>> Regards
>>>>
>>>> Pavel
>>>>
>>>>
>>>>>
>>>>> Regards,
>>>>> Dinesh
>>>>> manojadinesh.blogspot.com
>>>>>
>>>>
>>>>
>>>
>>
>