Re: [PATCH] SQL function to report log message

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
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
>>>>
>>>
>>>
>>
>