Re: [PATCH] SQL function to report log message

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

you have a true - in this case we can use YYTEXT - so the code can be some
like

if (tok != ';')
{
if (parse_elog_level(yytext, &elog_level)
{
...
}
}

but it means double string comparation, what is not good, or removing elog
levels from keyword list (what is surely out of area of this patch). So
using it in PLpgSQL was not practical idea. I am sorry.

Regards

Pavel

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