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

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.

Regards

Pavel

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