Re: [PATCH] SQL function to report log message

On Fri, Sep 4, 2015 at 2:03 PM, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com>
wrote:

>
>
> 2015-09-02 21:49 GMT+02:00 dinesh kumar <dineshkumar02(at)gmail(dot)com>:
>
>> 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.
>>
>
> 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.

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

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