Thread: [PATCH] SQL function to report log message

On Mon, Jul 13, 2015 at 4:54 PM, dinesh kumar <dineshkumar02@gmail.com> wrote:
> 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.

What's the actual use case for this feature other than it would be
good to have it?

A log message is here to give information about the
state of something that happens in backend, but in the case of this
function the event that happens is the content of the function itself.
It also adds a new log level for something that has a unique usage,
which looks like an overkill to me. Btw, you could do something more
advanced with simply an extension if you really want to play with this
area... But I am dubious about what kind of applications would use
that.
--
Michael

On 7/13/15 12:39 PM, dinesh kumar wrote:

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

    What's the actual use case for this feature other than it would be
    good to have it?


That's a good question Michael.

When i was working as a DBA for a different RDBMS, developers used to
write some serious APP errors, Followed by instructions into some sort
of log and trace files.
Since, DBAs didn't have the permission to check app logs, which was
owned by Ops team.

In my old case, application was having serious OOM issues, which was
crashing frequently after the deployment. It wasn't the consistent
behavior from the app side, hence they used to sent  a copy all APP
metrics to trace files, and we were monitoring the DB what was happening
during the spike on app servers.

Spewing a bunch of stuff into the postgres log doesn't seem like an improvement here.

I don't really see the point of what you're describing here. Just do something like RAISE WARNING which should normally be high enough to make it into the logs. Or use a pl language that will let you write your own logfile on the server (ie: plperlu).

I didn't mean that, we need to have this feature, since we have it on
other RDBMS. I don't see a reason, why don't we have this in our PG too.

I see the similar item in our development list
<http://www.postgresql.org/message-id/53A8E96E.9060507@2ndquadrant.com>.

That's not at all what that item is talking about. It's talking about exposing ereport as a SQL function, without altering the rest of our logging behavior.

--
Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX
Data in Trouble? Get it in Treble! http://BlueTreble.com

On 7/13/15 3:39 PM, dinesh kumar wrote:

    I don't really see the point of what you're describing here. Just do
    something like RAISE WARNING which should normally be high enough to
    make it into the logs. Or use a pl language that will let you write
    your own logfile on the server (ie: plperlu).

True. Using plperlu, shall we bypass our log_* settings. If it's true, i
wasn't sure about it.

plperlu can do anything the server can do. Including fun things like appending to any file the server can write to or executing things like `rm -rf pg_xlog`.

        I didn't mean that, we need to have this feature, since we have
        it on
        other RDBMS. I don't see a reason, why don't we have this in our
        PG too.

        I see the similar item in our development list
        <http://www.postgresql.org/message-id/53A8E96E.9060507@2ndquadrant.com>.


    That's not at all what that item is talking about. It's talking
    about exposing ereport as a SQL function, without altering the rest
    of our logging behavior.


Ah. It's' my bad interpretation. Let me work on it, and will send a new
patch as a wrapper sql function for ereport.

You might want to present a plan for that; it's not as trivial as it sounds due to how ereport works. In particular, I'd want to see (at minimum) the same functionality that plpgsql's RAISE command now provides (errdetail, errhint, etc).

--
Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX
Data in Trouble? Get it in Treble! http://BlueTreble.com

Jim Nasby <Jim.Nasby@BlueTreble.com> writes:
> On 7/13/15 3:39 PM, dinesh kumar wrote:
>> Ah. It's' my bad interpretation. Let me work on it, and will send a new
>> patch as a wrapper sql function for ereport.

> You might want to present a plan for that; it's not as trivial as it
> sounds due to how ereport works. In particular, I'd want to see (at
> minimum) the same functionality that plpgsql's RAISE command now
> provides (errdetail, errhint, etc).

The real question is why the existing functionality in plpgsql isn't
sufficient.  Somebody who wants a "log from SQL" function can easily
write a simple plpgsql function that does exactly what they want,
with no more or fewer bells-n-whistles than they need.  If we try
to create a SQL function that does all that, it's likely to be a mess
to use, even with named arguments.

I'm not necessarily against the basic idea, but I think inventing
something that actually offers an increment in usability compared
to the existing alternative is going to be harder than it sounds.

                        regards, tom lane

On Thu, Jul 23, 2015 at 10:56 AM, dinesh kumar <dineshkumar02@gmail.com> wrote:
> Hi All,
>
> On Mon, Jul 13, 2015 at 2:12 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>
>> Jim Nasby <Jim.Nasby@BlueTreble.com> writes:
>> > On 7/13/15 3:39 PM, dinesh kumar wrote:
>> >> Ah. It's' my bad interpretation. Let me work on it, and will send a new
>> >> patch as a wrapper sql function for ereport.
>>
>> > You might want to present a plan for that; it's not as trivial as it
>> > sounds due to how ereport works. In particular, I'd want to see (at
>> > minimum) the same functionality that plpgsql's RAISE command now
>> > provides (errdetail, errhint, etc).
>>
>
> Jim,
>
> For now, I  worked on (ERROR Level, ERROR Message, HIDE ERROR Stmt). In our
> to do item description, I found this wrapper needs to return "Anyelement".
> But, I believe, return "VOID" is enough for this function. Let me know if I
> erred here.
>
> In design phase,
>
> 1. I took a CustomDataType with the elevel code, elevel text
>
> 2. Populated this CDT with all existing pre-processors, except {FATAL,
> PANIC}. Since, we don't expose these to client.
>
> 3. By matching the user elevel text, processing the report log function.
>
> Find the attached patch with implementation.

Btw, if you want to get more attention for your patch as well as
reviews, you should consider registering to the next commit fest of
September:
https://commitfest.postgresql.org/6/
Regards,
--
Michael

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.

Regards,
Dinesh
manojadinesh.blogspot.com

Hi

I am starting to work review of this patch

2015-07-13 9:54 GMT+02:00 dinesh kumar <dineshkumar02@gmail.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, ...

2. Missing regress tests

3. the parsing ereport level should be public function shared with PLpgSQL and other PL

4. should be hidestmt mandatory parameter?

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

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.

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

Regards

Pavel

 

Regards,
Dinesh
manojadinesh.blogspot.com

Hi,

On Sat, Aug 29, 2015 at 4:22 PM, Pavel Stehule <pavel.stehule@gmail.com> wrote:

Hi

I am starting to work review of this patch

2015-07-13 9:54 GMT+02:00 dinesh kumar <dineshkumar02@gmail.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.

 

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.

 

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.

Adding new patch, with the above fixes.

Thanks in advance.

Regards,
Dinesh

Regards

Pavel

 

Regards,
Dinesh
manojadinesh.blogspot.com

2015-08-31 20:43 GMT+02:00 dinesh kumar <dineshkumar02@gmail.com>:

Hi,

On Sat, Aug 29, 2015 at 4:22 PM, Pavel Stehule <pavel.stehule@gmail.com> wrote:

Hi

I am starting to work review of this patch

2015-07-13 9:54 GMT+02:00 dinesh kumar <dineshkumar02@gmail.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

On 8/31/15 11:59 PM, Pavel Stehule wrote:

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.

Wouldn't it be better to create an ENUM of error levels instead of inventing more parsing code?

Though, I guess ENUMs are case sensitive, but I'd rather solve that by creating a CI ENUM, which would be useful across the board...
--
Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX
Experts in Analytics, Data Architecture and PostgreSQL
Data in Trouble? Get it in Treble! http://BlueTreble.com

On 9/1/15 12:47 AM, Pavel Stehule wrote:

    Wouldn't it be better to create an ENUM of error levels instead of
    inventing more parsing code?


Do you think SQL ENUM? I little bit afraid about possible problems with
pg_upgrade.

It is not simple question - the ENUM can be interesting from custom
space perspective, but from our internal perspective the parsing
function is more practical - and faster. The error level is our internal
value, and users should not to read it - for this purpouse is error level.

My thought is that there's a fair amount of places where we do string comparison for not a great reason. Perhaps a better example is data that comes back from a trigger; AFTER/BEFORE, INSERT/UPDATE/..., which is more expensive to setup the variables for (strdup a fixed string, which means a palloc), and then comparisons are done as text varlena (iirc).

Instead if this information came back as an ENUM the variable would be a simple int as would the comparison. We'd still have a raw string being parsed in the function body, but that would happen once during initial compilation and it would be replaced with an ENUM value.

For RAISE, AFAIK we still end up converting the raw string into a varlena CONST, which means a palloc. If it was an ENUM it'd be converted to an int.

If we're worried about the overhead of the enum machinery we could create a relcache for system enums, but I suspect that even without that it'd be a win over the string stuff. Especially since I bet most people run UTF8.

--
Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX
Experts in Analytics, Data Architecture and PostgreSQL
Data in Trouble? Get it in Treble! http://BlueTreble.com

2015-09-01 6:59 GMT+02:00 Pavel Stehule <pavel.stehule@gmail.com>:

2015-08-31 20:43 GMT+02:00 dinesh kumar <dineshkumar02@gmail.com>:

Hi,

On Sat, Aug 29, 2015 at 4:22 PM, Pavel Stehule <pavel.stehule@gmail.com> wrote:

Hi

I am starting to work review of this patch

2015-07-13 9:54 GMT+02:00 dinesh kumar <dineshkumar02@gmail.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

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

3. test should be enhanced for optional parameters

Regards

Pavel

 

Thanks in advance.

Regards,
Dinesh

Regards

Pavel

 

Regards,
Dinesh
manojadinesh.blogspot.com

On Mon, Aug 31, 2015 at 10:08 PM, Pavel Stehule <pavel.stehule@gmail.com> wrote:

2015-09-01 6:59 GMT+02:00 Pavel Stehule <pavel.stehule@gmail.com>:

2015-08-31 20:43 GMT+02:00 dinesh kumar <dineshkumar02@gmail.com>:

Hi,

On Sat, Aug 29, 2015 at 4:22 PM, Pavel Stehule <pavel.stehule@gmail.com> wrote:

Hi

I am starting to work review of this patch

2015-07-13 9:54 GMT+02:00 dinesh kumar <dineshkumar02@gmail.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.

Regards,

Dinesh

manojadinesh.blogspot.com

Regards

Pavel

 

Thanks in advance.

Regards,
Dinesh

Regards

Pavel

 

Regards,
Dinesh
manojadinesh.blogspot.com

On Mon, Aug 31, 2015 at 10:08 PM, Pavel Stehule <pavel.stehule@gmail.com> wrote:

2015-09-01 6:59 GMT+02:00 Pavel Stehule <pavel.stehule@gmail.com>:

2015-08-31 20:43 GMT+02:00 dinesh kumar <dineshkumar02@gmail.com>:

Hi,

On Sat, Aug 29, 2015 at 4:22 PM, Pavel Stehule <pavel.stehule@gmail.com> wrote:

Hi

I am starting to work review of this patch

2015-07-13 9:54 GMT+02:00 dinesh kumar <dineshkumar02@gmail.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.

Regards,

Dinesh

manojadinesh.blogspot.com

Regards

Pavel

 

Thanks in advance.

Regards,
Dinesh

Regards

Pavel

 

Regards,
Dinesh
manojadinesh.blogspot.com

2015-09-02 21:49 GMT+02:00 dinesh kumar <dineshkumar02@gmail.com>:

On Mon, Aug 31, 2015 at 10:08 PM, Pavel Stehule <pavel.stehule@gmail.com> wrote:

2015-09-01 6:59 GMT+02:00 Pavel Stehule <pavel.stehule@gmail.com>:

2015-08-31 20:43 GMT+02:00 dinesh kumar <dineshkumar02@gmail.com>:

Hi,

On Sat, Aug 29, 2015 at 4:22 PM, Pavel Stehule <pavel.stehule@gmail.com> wrote:

Hi

I am starting to work review of this patch

2015-07-13 9:54 GMT+02:00 dinesh kumar <dineshkumar02@gmail.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

2015-09-02 21:49 GMT+02:00 dinesh kumar <dineshkumar02@gmail.com>:

On Mon, Aug 31, 2015 at 10:08 PM, Pavel Stehule <pavel.stehule@gmail.com> wrote:

2015-09-01 6:59 GMT+02:00 Pavel Stehule <pavel.stehule@gmail.com>:

2015-08-31 20:43 GMT+02:00 dinesh kumar <dineshkumar02@gmail.com>:

Hi,

On Sat, Aug 29, 2015 at 4:22 PM, Pavel Stehule <pavel.stehule@gmail.com> wrote:

Hi

I am starting to work review of this patch

2015-07-13 9:54 GMT+02:00 dinesh kumar <dineshkumar02@gmail.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

On Fri, Sep 4, 2015 at 2:03 PM, Pavel Stehule <pavel.stehule@gmail.com> wrote:

2015-09-02 21:49 GMT+02:00 dinesh kumar <dineshkumar02@gmail.com>:

On Mon, Aug 31, 2015 at 10:08 PM, Pavel Stehule <pavel.stehule@gmail.com> wrote:

2015-09-01 6:59 GMT+02:00 Pavel Stehule <pavel.stehule@gmail.com>:

2015-08-31 20:43 GMT+02:00 dinesh kumar <dineshkumar02@gmail.com>:

Hi,

On Sat, Aug 29, 2015 at 4:22 PM, Pavel Stehule <pavel.stehule@gmail.com> wrote:

Hi

I am starting to work review of this patch

2015-07-13 9:54 GMT+02:00 dinesh kumar <dineshkumar02@gmail.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.

 

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. 

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

2015-09-05 8:35 GMT+02:00 dinesh kumar <dineshkumar02@gmail.com>:

On Fri, Sep 4, 2015 at 2:03 PM, Pavel Stehule <pavel.stehule@gmail.com> wrote:

2015-09-02 21:49 GMT+02:00 dinesh kumar <dineshkumar02@gmail.com>:

On Mon, Aug 31, 2015 at 10:08 PM, Pavel Stehule <pavel.stehule@gmail.com> wrote:

2015-09-01 6:59 GMT+02:00 Pavel Stehule <pavel.stehule@gmail.com>:

2015-08-31 20:43 GMT+02:00 dinesh kumar <dineshkumar02@gmail.com>:

Hi,

On Sat, Aug 29, 2015 at 4:22 PM, Pavel Stehule <pavel.stehule@gmail.com> wrote:

Hi

I am starting to work review of this patch

2015-07-13 9:54 GMT+02:00 dinesh kumar <dineshkumar02@gmail.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

Hi

attached patch with fixed broken error message

Regards

Pavel

On Sun, Sep 6, 2015 at 3:39 PM, Pavel Stehule <pavel.stehule@gmail.com> wrote:

Hi

attached patch with fixed broken error message

Regards

Pavel

Hi Pavel,

Thanks much for taking care of it. Patch looks great.

--

Regards,

Dinesh

manojadinesh.blogspot.com

On Sun, Sep 6, 2015 at 4:00 PM, dinesh kumar <dineshkumar02@gmail.com> wrote:

On Sun, Sep 6, 2015 at 3:39 PM, Pavel Stehule <pavel.stehule@gmail.com> wrote:

Hi

attached patch with fixed broken error message

Regards

Pavel

Hi Pavel,

Thanks much for taking care of it. Patch looks great.

Hi Pavel,

Could you let me know, what status value i need to change in commitfest's UI.

 

--

Regards,

Dinesh

manojadinesh.blogspot.com

--

Regards,

Dinesh

manojadinesh.blogspot.com

2015-09-06 13:12 GMT+02:00 dinesh kumar <dineshkumar02@gmail.com>:

On Sun, Sep 6, 2015 at 4:00 PM, dinesh kumar <dineshkumar02@gmail.com> wrote:

On Sun, Sep 6, 2015 at 3:39 PM, Pavel Stehule <pavel.stehule@gmail.com> wrote:

Hi

attached patch with fixed broken error message

Regards

Pavel

Hi Pavel,

Thanks much for taking care of it. Patch looks great.

Hi Pavel,

Could you let me know, what status value i need to change in commitfest's UI.

if you have not objections, the status can be "ready for commiter"

Regards

Pavel
 

 

--

Regards,

Dinesh

manojadinesh.blogspot.com

--

Regards,

Dinesh

manojadinesh.blogspot.com

On Wed, Jul 22, 2015 at 9:56 PM, dinesh kumar <dineshkumar02@gmail.com> wrote:
>> The real question is why the existing functionality in plpgsql isn't
>> sufficient.  Somebody who wants a "log from SQL" function can easily
>> write a simple plpgsql function that does exactly what they want,
>> with no more or fewer bells-n-whistles than they need.  If we try
>> to create a SQL function that does all that, it's likely to be a mess
>> to use, even with named arguments.
>>
>> I'm not necessarily against the basic idea, but I think inventing
>> something that actually offers an increment in usability compared
>> to the existing alternative is going to be harder than it sounds.
>>
>
> I agree with your inputs. We can build  pl/pgsql function as alternative for
> this.
>
> My initial proposal, and implementation was, logging messages to log file
> irrespectively of our log settings. I was not sure we can do this with some
> pl/perlu. And then, I started working on our to do item,
> ereport, wrapper callable from SQL, and found it can be useful to have a
> direct function call with required log level.

But, why?

I just took a look at the latest patch and I can't see why it's any
better than just using PL/pgsql's RAISE statement.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Thu, Sep 10, 2015 at 01:32:10AM +0200, Andres Freund wrote:
> On 2015-09-09 18:27:51 -0400, Robert Haas wrote:
> > On Wed, Sep 9, 2015 at 11:37 AM, dinesh kumar <dineshkumar02@gmail.com> wrote:
> > > Sure, it’s a clear fact that, we can implement this function
> > > with RAISE statements.
> >
> > Given that, I suggest we just forget the whole thing.
>
> I'm not convinced. Sure, it's easy, but I by now have written the
> respective function dozens of times. Why should we force that on
> everyone?

+20 or so here, that being the number of times I recall offhand having
written the function.

Cheers,
David.
--
David Fetter <david@fetter.org> http://fetter.org/
Phone: +1 415 235 3778  AIM: dfetter666  Yahoo!: dfetter
Skype: davidfetter      XMPP: david.fetter@gmail.com

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

dinesh kumar wrote:

> Also, I was under impression that, all our TODO
> <https://wiki.postgresql.org/wiki/Todo> items are filtered for the real use
> cases. Is my impression wrong. If I wanted to work on another TODO item,
> where i need to take a look.

Your impression is completely, absolutely, horribly wrong.

The TODO contains some ideas that are good but postponed, other ideas
that are bad but we didn't know at the time they were recorded, other
ideas that we don't know either way.  Before doing anything on an item
from the TODO list, you should first read the linked threads (if any),
and keep track when they end with an email saying "what an awful idea".
If this doesn't happen, _search_ for other threads not linked on the
TODO list that also deal with the same topic; note if they end the same
way (if you find such threads, it's useful to add a link to them in the
TODO item). 

Even if you can't find overly negative opinions about some item, discuss
it here before doing any actual coding.

I wonder if we need a new page TONOTDO or something like that.

--
Álvaro Herrera                http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

dinesh kumar wrote:

> Also, I was under impression that, all our TODO
> <https://wiki.postgresql.org/wiki/Todo> items are filtered for the real use
> cases. Is my impression wrong. If I wanted to work on another TODO item,
> where i need to take a look.

Your impression is completely, absolutely, horribly wrong.

The TODO contains some ideas that are good but postponed, other ideas
that are bad but we didn't know at the time they were recorded, other
ideas that we don't know either way.  Before doing anything on an item
from the TODO list, you should first read the linked threads (if any),
and keep track when they end with an email saying "what an awful idea".
If this doesn't happen, _search_ for other threads not linked on the
TODO list that also deal with the same topic; note if they end the same
way (if you find such threads, it's useful to add a link to them in the
TODO item).

Even if you can't find overly negative opinions about some item, discuss
it here before doing any actual coding.

I wonder if we need a new page TONOTDO or something like that.

On 9/10/15 10:56 AM, Andres Freund wrote:

>The only complaint I've seen in this thread that seems like a valid
>deficiency is that RAISE can't deal with treating the error severity level
>as a variable.  But surely we should address that as a new RAISE feature,
>not by inventing a SQL wrapper that will need to reproduce every existing
>RAISE feature before it can think about solving anything new.

That seems like something independently useful.

fa
If we're up for that the other thing I'd add is having raise ignore anything supplied by USING that's NULL, instead of treating it as an error. That would make it very easy to create a wrapper function that exposes the full capabilities of RAISE.

--
Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX
Experts in Analytics, Data Architecture and PostgreSQL
Data in Trouble? Get it in Treble! http://BlueTreble.com

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 10/15/15 11:51 PM, Pavel Stehule wrote:

I don't think so ignoring NULL in RAISE statement is good idea (it is
not safe). We can replace NULL by some string (like "NULL") by default.
I am thinking about other possibilities.

What I was trying to say is that if the argument to a USING option is NULL then RAISE should skip over it, as if it hadn't been applied at all. Similar to how the code currently tests for \0.

1. some RAISE statement flag - but there was strong disagreement when I
did it last time
2. some plpgsql GUC variables like plpgsq.raise_ignore_null
3. accept a function from this patch

Now, I am thinking so @3 is good option. It can be really useful as last
rescue for other PL without possibility to raise rich PostgreSQL
exception - currently PLPythonu, partially PLPerl (where are more
issues), probably in others.

I agree, assuming the patch exposes all the stuff you can do with USING in plpgsql.
--
Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX
Experts in Analytics, Data Architecture and PostgreSQL
Data in Trouble? Get it in Treble! http://BlueTreble.com

On 10/17/15 11:49 AM, Pavel Stehule wrote:

2015-10-17 18:42 GMT+02:00 Jim Nasby <Jim.Nasby@bluetreble.com
<mailto:Jim.Nasby@bluetreble.com>>:

    On 10/15/15 11:51 PM, Pavel Stehule wrote:

        I don't think so ignoring NULL in RAISE statement is good idea
        (it is
        not safe). We can replace NULL by some string (like "NULL") by
        default.
        I am thinking about other possibilities.


    What I was trying to say is that if the argument to a USING option
    is NULL then RAISE should skip over it, as if it hadn't been applied
    at all. Similar to how the code currently tests for \0.


I understand, but I don't prefer this behave. The NULL is strange value
and should be signalized.

So instead of raising the message we wanted, we throw a completely different exception? How does that make sense?

More to the point, if RAISE operated this way then it would be trivial to create a fully functional plpgsql wrapper around it.

--
Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX
Experts in Analytics, Data Architecture and PostgreSQL
Data in Trouble? Get it in Treble! http://BlueTreble.com

On Mon, Oct 19, 2015 at 7:59 PM, Jim Nasby <Jim.Nasby@bluetreble.com> wrote:
> I fail to see how doing
>
> HINT: NULL
>
> is much better than just not raising a HINT at all...

I'm not a huge fan of this patch, as previously noted, but I certainly
agree that if we're going to do it, we should ignore a null argument,
not print out the word "NULL".  Who would ever want that behavior?

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Tue, Oct 20, 2015 at 11:09 AM, Pavel Stehule <pavel.stehule@gmail.com> wrote:
> Probably it was my request. I don't like to using NULL as value, that should
> be ignored. The "hint" is clean, there NULL can be ignored, but what about
> DETAIL or MESSAGE?

If the field is required - as MESSAGE is - then its absence is an
error.  If the field is optional, treat a NULL if the parameter were
not supplied.

> I am strong in my opinion about PLpgSQL RAISE statement behave, but on
> second hand, proposed function should not be 100% same as RAISE stmt. More
> we can simply add a parameter like "ignore_nulls"

I would be willing to bet you a drink that 99.9% of people will want
the behavior Jim is advocating, so I don't think this should be
configurable.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Tue, Oct 20, 2015 at 11:29 AM, Pavel Stehule <pavel.stehule@gmail.com> wrote:
> 2015-10-20 17:15 GMT+02:00 Robert Haas <robertmhaas@gmail.com>:
>> On Tue, Oct 20, 2015 at 11:09 AM, Pavel Stehule <pavel.stehule@gmail.com>
>> wrote:
>> > Probably it was my request. I don't like to using NULL as value, that
>> > should
>> > be ignored. The "hint" is clean, there NULL can be ignored, but what
>> > about
>> > DETAIL or MESSAGE?
>>
>> If the field is required - as MESSAGE is - then its absence is an
>> error.  If the field is optional, treat a NULL if the parameter were
>> not supplied.
>
> I understand well, what was proposed. Personally I see small risk, but I am
> thinking so can be useful if users can choose between two possibilities
> (strict, and NULL tolerant). For some adhoc work it can be useful.

You haven't made any attempt to explain why that behavior would be
useful to anyone except that saying some information might be lost.
But what field of an error report can sensibly be populated with the
word NULL, and nothing else?

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

2015-10-20 20:05 GMT+02:00 Robert Haas <robertmhaas@gmail.com>:

On Tue, Oct 20, 2015 at 11:29 AM, Pavel Stehule <pavel.stehule@gmail.com> wrote:
> 2015-10-20 17:15 GMT+02:00 Robert Haas <robertmhaas@gmail.com>:
>> On Tue, Oct 20, 2015 at 11:09 AM, Pavel Stehule <pavel.stehule@gmail.com>
>> wrote:
>> > Probably it was my request. I don't like to using NULL as value, that
>> > should
>> > be ignored. The "hint" is clean, there NULL can be ignored, but what
>> > about
>> > DETAIL or MESSAGE?
>>
>> If the field is required - as MESSAGE is - then its absence is an
>> error.  If the field is optional, treat a NULL if the parameter were
>> not supplied.
>
> I understand well, what was proposed. Personally I see small risk, but I am
> thinking so can be useful if users can choose between two possibilities
> (strict, and NULL tolerant). For some adhoc work it can be useful.

You haven't made any attempt to explain why that behavior would be
useful to anyone except that saying some information might be lost.
But what field of an error report can sensibly be populated with the
word NULL, and nothing else?

My previous idea was wrong (I didn't though well about all details). I am sorry. The implementation of variadic parameters in Postgres requires some default value - in this case the only one logical default value is NULL. And in this case, when the default is used, the NULL shouldn't be displayed. I propose following behave. The level and the message arguments are mandatory (has not default value), others are optional. The level is should not be NULL, the message can be NULL, and the NULL should be displayed, any others are ignored if holds NULL.  A alternative is - only the level will be mandatory, others will be optional, and then there are not any exception for message.

On 10/22/15 2:20 AM, dinesh kumar wrote:

2. Using this function, if we provide any "NULL" argument to the function,
  we should either skip it or report it. I see this is what the function
is doing.

postgres=# SELECT pg_report_log('INFO', 'NULL', false, NULL, NULL);
INFO:  NULL

postgres=# SELECT pg_report_log('INFO', 'NULL', false, 'NULL', 'NULL');
INFO:  NULL
DETAIL:  NULL /-- Are you suggesting to change this behaviour/
HINT:  NULL

It should operate the same as what was decided for RAISE.

I'd say it should also support the remaining RAISE options as well (COLUMN, CONSTRAINT, DATATYPE, TABLE, SCHEMA).

I think hide_statement is a better name than ishidestmt. It would be nice if RAISE supported that too...

I think the function should also allow specifying a condition name instead of a SQL state, same as RAISE does.

In other words, this function and raise should operate exactly the same unless there's a really strong reason not to. Otherwise it's just going to create confusion.

--
Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX
Experts in Analytics, Data Architecture and PostgreSQL
Data in Trouble? Get it in Treble! http://BlueTreble.com

On 10/22/15 2:20 AM, dinesh kumar wrote:

2. Using this function, if we provide any "NULL" argument to the function,
  we should either skip it or report it. I see this is what the function
is doing.

postgres=# SELECT pg_report_log('INFO', 'NULL', false, NULL, NULL);
INFO:  NULL

postgres=# SELECT pg_report_log('INFO', 'NULL', false, 'NULL', 'NULL');
INFO:  NULL
DETAIL:  NULL /-- Are you suggesting to change this behaviour/
HINT:  NULL

It should operate the same as what was decided for RAISE.

I'd say it should also support the remaining RAISE options as well (COLUMN, CONSTRAINT, DATATYPE, TABLE, SCHEMA).

I think hide_statement is a better name than ishidestmt. It would be nice if RAISE supported that too...

I think the function should also allow specifying a condition name instead of a SQL state, same as RAISE does.

In other words, this function and raise should operate exactly the same unless there's a really strong reason not to. Otherwise it's just going to create confusion.

--
Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX
Experts in Analytics, Data Architecture and PostgreSQL
Data in Trouble? Get it in Treble! http://BlueTreble.com

On 10/22/15 4:42 PM, Pavel Stehule wrote:

the behave of pg_report_log should not be exactly same as RAISE
statement in PLpgSQL.

That makes no sense to me. Having different behaviors is just going to lead to confusion.

If this function will be exactly same, then it
lost a sense and anybody can use RAISE statement.

It prevents everyone from reinventing the 'create a function wrapper around RAISE' wheel that several people on this list alone have admitted to. I think there's plenty of value in that.

--
Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX
Experts in Analytics, Data Architecture and PostgreSQL
Data in Trouble? Get it in Treble! http://BlueTreble.com

On 10/22/15 4:59 PM, Pavel Stehule wrote:

    It prevents everyone from reinventing the 'create a function wrapper
    around RAISE' wheel that several people on this list alone have
    admitted to. I think there's plenty of value in that.


I have different opinion, I am sorry. The RAISE statement is differently
designed with different possibility - the function is limited by using
variadic function, and should not to have same behave as RAISE. And I
don't like a idea to push RAISE to behave of variadic function.

I thought the only issue here was that RAISE currently pukes on a NULL input, and I thought you'd changed your mind and agreed that it makes sense for RAISE to just silently ignore anything that's NULL (except maybe for message). Am I wrong on one or both counts?

IIRC 3 or 4 people on this list liked the idea of a function roughly equivalent to RAISE, to avoid the make-work of writing that function. That's why I disagree with your statement that there's no point to this function even if it acts the same as RAISE.

--
Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX
Experts in Analytics, Data Architecture and PostgreSQL
Data in Trouble? Get it in Treble! http://BlueTreble.com

I haven't seen this discussed before, but I don't find the name
pg_report_log particularly good.  Why not jut pg_log?

On Sunday, November 15, 2015 8:51 PM, Craig Ringer <craig@2ndquadrant.com> wrote:

> I'd prefer to omit fields if explicitly assigned to NULL. You can
> always use coalesce if you want the string 'NULL'; I agree with
> others here that the vast majority of users will want the field
> just omitted.

+1

Unfortunately those writing the SQL standard chose to have a single
flag (NULL) to indicate either "unknown" or "not applicable".  That
causes problems where it's not clear which way the value should be
interpreted, but in this case it seems pretty clear that someone
passing a NULL parameter for hint to a function like this doesn't
mean "there is likely to be a valid value for hint, but I don't
know what it is" -- they mean there is no available hint, so please
don't show one.  Any other behavior seems rather silly.

--
Kevin Grittner
EDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 11/17/15 2:16 AM, Jim Nasby wrote:
> On 11/15/15 10:56 PM, dinesh kumar wrote:
>> So, shall we make this pg_report_log TO pg_write_log OR pg_ereport OR
>> <SOME OTHER GOOD SUGGESTIONS> from you.
>
> Why not pg_raise to mirror plpgsql? (The function does have the same
> semantics, right? It's not doing something like only sending to the log
> and not the client?)

I think the "raise" terminology is specific to plpgsql, as it actually
raises an exception in that language.