Re: [PATCH] SQL function to report log message

Hi Pavel,

On Sat, Sep 5, 2015 at 12:36 AM, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com>
wrote:

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

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

Thanks much.

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

--

Regards,
Dinesh
manojadinesh.blogspot.com