Re: [PATCH] SQL function to report log message

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.
>
> Thanks in advance.
>
> Regards,
> Dinesh
>
>>
>> Regards
>>
>> Pavel
>>
>>
>>>
>>> Regards,
>>> Dinesh
>>> manojadinesh.blogspot.com
>>>
>>
>>
>