Re: [PATCH] SQL function to report log message

2015-09-05 8:35 GMT+02:00 dinesh kumar <dineshkumar02(at)gmail(dot)com>:

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

it should be a optional parameter,

please fix doc, there are not any difference between mandatory and optional
parametere

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

ok

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