pl/pgperl Patch for adding $_FN detail just like triggers have for $_TD

On 2024-08-28 We 5:53 PM, Mark Murawski wrote:

Hi Hackers!

This would be version v1 of this feature

Basically, the subject says it all: pl/pgperl Patch for being able to
tell which function you're in.
This is a hashref so it will be possible to populate new and exciting
other details in the future as the need arises

This also greatly improves logging capabilities for things like
catching warnings,  Because as it stands right now, there's no
information that can assist with locating the source of a warning like
this:

# tail -f /var/log/postgresql.log
******* GOT A WARNING - Use of uninitialized value $prefix in
concatenation (.) or string at (eval 531) line 48.

Now, with $_FN you can do this:

CREATE OR REPLACE FUNCTION throw_warning() RETURNS text LANGUAGE
plperlu AS $function$

use warnings;
use strict;
use Data::Dumper;

$SIG{__WARN__} = sub {
  elog(NOTICE, Dumper($_FN));

  print STDERR "In Function: $_FN->{name}: $_[0]\n";
};

my $a;
print "$a"; # uninit!

return undef;

$function$
;

This patch is against 12 which is still our production branch. This
could easily be also patched against newer releases as well.

I've been using this code in production now for about 3 years, it's
greatly helped track down issues.  And there shouldn't be anything
platform-specific here, it's all regular perl API

I'm not sure about adding testing.  This is my first postgres patch,
so any guidance on adding regression testing would be appreciated.

The rationale for this has come from the need to know the source
function name, and we've typically resorted to things like this in the
past:

CREATE OR REPLACE FUNCTION throw_warning() RETURNS text LANGUAGE
plperlu AS $function$
my $function_name = 'throw_warning';
$SIG{__WARN__} = sub { print STDERR "In Function: $function_name:
$_[0]\n"; }
$function$
;

We've literally had to copy/paste this all over and it's something
that postgres should just 'give you' since it knows the name already,
just like when triggers pass you $_TD with all the pertinent information

A wishlist item would be for postgres plperl to automatically prepend
the function name and schema when throwing perl warnings so you don't
have to do your own __WARN__ handler, but this is the next best thing.

I'm not necessarily opposed to this, but the analogy to $_TD isn't
really apt.  You can't know the trigger data at compile time, whereas
you can know the function's name at compile time, using just the
mechanism you find irksome.

And if we're going to do it for plperl, shouldn't we do it for other PLs?

cheers

andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

On 8/29/24 11:56, Andrew Dunstan wrote:

On 2024-08-28 We 5:53 PM, Mark Murawski wrote:

Hi Hackers!

This would be version v1 of this feature

Basically, the subject says it all: pl/pgperl Patch for being able to
tell which function you're in.
This is a hashref so it will be possible to populate new and exciting
other details in the future as the need arises

This also greatly improves logging capabilities for things like
catching warnings,  Because as it stands right now, there's no
information that can assist with locating the source of a warning
like this:

# tail -f /var/log/postgresql.log
******* GOT A WARNING - Use of uninitialized value $prefix in
concatenation (.) or string at (eval 531) line 48.

Now, with $_FN you can do this:

CREATE OR REPLACE FUNCTION throw_warning() RETURNS text LANGUAGE
plperlu AS $function$

use warnings;
use strict;
use Data::Dumper;

$SIG{__WARN__} = sub {
  elog(NOTICE, Dumper($_FN));

  print STDERR "In Function: $_FN->{name}: $_[0]\n";
};

my $a;
print "$a"; # uninit!

return undef;

$function$
;

This patch is against 12 which is still our production branch. This
could easily be also patched against newer releases as well.

I've been using this code in production now for about 3 years, it's
greatly helped track down issues.  And there shouldn't be anything
platform-specific here, it's all regular perl API

I'm not sure about adding testing.  This is my first postgres patch,
so any guidance on adding regression testing would be appreciated.

The rationale for this has come from the need to know the source
function name, and we've typically resorted to things like this in
the past:

CREATE OR REPLACE FUNCTION throw_warning() RETURNS text LANGUAGE
plperlu AS $function$
my $function_name = 'throw_warning';
$SIG{__WARN__} = sub { print STDERR "In Function: $function_name:
$_[0]\n"; }
$function$
;

We've literally had to copy/paste this all over and it's something
that postgres should just 'give you' since it knows the name already,
just like when triggers pass you $_TD with all the pertinent information

A wishlist item would be for postgres plperl to automatically prepend
the function name and schema when throwing perl warnings so you don't
have to do your own __WARN__ handler, but this is the next best thing.

I'm not necessarily opposed to this, but the analogy to $_TD isn't
really apt.  You can't know the trigger data at compile time, whereas
you can know the function's name at compile time, using just the
mechanism you find irksome.

And if we're going to do it for plperl, shouldn't we do it for other PLs?

cheers

andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

Hi Andrew,

Thanks for the feedback.

1) Why is this not similar to _TD?  It literally operates identically.
At run-time it passes you $_TD  for triggers.    Same her for
functions.  This is all run-time.   What exactly is the issue you're
trying to point out?

2) I would agree that other PLs should get the same detail.  I don't
know the other ones as I've been only working in pl/perl.

On 2024-08-29 Th 1:01 PM, Mark Murawski wrote:

On 8/29/24 11:56, Andrew Dunstan wrote:

On 2024-08-28 We 5:53 PM, Mark Murawski wrote:

Hi Hackers!

This would be version v1 of this feature

Basically, the subject says it all: pl/pgperl Patch for being able
to tell which function you're in.
This is a hashref so it will be possible to populate new and
exciting other details in the future as the need arises

This also greatly improves logging capabilities for things like
catching warnings,  Because as it stands right now, there's no
information that can assist with locating the source of a warning
like this:

# tail -f /var/log/postgresql.log
******* GOT A WARNING - Use of uninitialized value $prefix in
concatenation (.) or string at (eval 531) line 48.

Now, with $_FN you can do this:

CREATE OR REPLACE FUNCTION throw_warning() RETURNS text LANGUAGE
plperlu AS $function$

use warnings;
use strict;
use Data::Dumper;

$SIG{__WARN__} = sub {
  elog(NOTICE, Dumper($_FN));

  print STDERR "In Function: $_FN->{name}: $_[0]\n";
};

my $a;
print "$a"; # uninit!

return undef;

$function$
;

This patch is against 12 which is still our production branch. This
could easily be also patched against newer releases as well.

I've been using this code in production now for about 3 years, it's
greatly helped track down issues.  And there shouldn't be anything
platform-specific here, it's all regular perl API

I'm not sure about adding testing.  This is my first postgres patch,
so any guidance on adding regression testing would be appreciated.

The rationale for this has come from the need to know the source
function name, and we've typically resorted to things like this in
the past:

CREATE OR REPLACE FUNCTION throw_warning() RETURNS text LANGUAGE
plperlu AS $function$
my $function_name = 'throw_warning';
$SIG{__WARN__} = sub { print STDERR "In Function: $function_name:
$_[0]\n"; }
$function$
;

We've literally had to copy/paste this all over and it's something
that postgres should just 'give you' since it knows the name
already, just like when triggers pass you $_TD with all the
pertinent information

A wishlist item would be for postgres plperl to automatically
prepend the function name and schema when throwing perl warnings so
you don't have to do your own __WARN__ handler, but this is the next
best thing.

I'm not necessarily opposed to this, but the analogy to $_TD isn't
really apt.  You can't know the trigger data at compile time, whereas
you can know the function's name at compile time, using just the
mechanism you find irksome.

And if we're going to do it for plperl, shouldn't we do it for other
PLs?

Hi Andrew,

Thanks for the feedback.

1) Why is this not similar to _TD?  It literally operates identically.
At run-time it passes you $_TD  for triggers.    Same her for
functions.  This is all run-time.   What exactly is the issue you're
trying to point out?

It's not the same as the trigger data case because the function name is
knowable at compile time, as in fact you have demonstrated. You just
find it a bit inconvenient to have to code for that knowledge. By
contrast, trigger data is ONLY knowable at run time.

But I don't see that it is such a heavy burden to have to write

  my $funcname = "foo";

or similar in your function.

cheers

andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

On 8/29/24 16:54, Andrew Dunstan wrote:

On 2024-08-29 Th 1:01 PM, Mark Murawski wrote:

On 8/29/24 11:56, Andrew Dunstan wrote:

On 2024-08-28 We 5:53 PM, Mark Murawski wrote:

Hi Hackers!

This would be version v1 of this feature

Basically, the subject says it all: pl/pgperl Patch for being able
to tell which function you're in.
This is a hashref so it will be possible to populate new and
exciting other details in the future as the need arises

This also greatly improves logging capabilities for things like
catching warnings,  Because as it stands right now, there's no
information that can assist with locating the source of a warning
like this:

# tail -f /var/log/postgresql.log
******* GOT A WARNING - Use of uninitialized value $prefix in
concatenation (.) or string at (eval 531) line 48.

Now, with $_FN you can do this:

CREATE OR REPLACE FUNCTION throw_warning() RETURNS text LANGUAGE
plperlu AS $function$

use warnings;
use strict;
use Data::Dumper;

$SIG{__WARN__} = sub {
  elog(NOTICE, Dumper($_FN));

  print STDERR "In Function: $_FN->{name}: $_[0]\n";
};

my $a;
print "$a"; # uninit!

return undef;

$function$
;

This patch is against 12 which is still our production branch. This
could easily be also patched against newer releases as well.

I've been using this code in production now for about 3 years, it's
greatly helped track down issues.  And there shouldn't be anything
platform-specific here, it's all regular perl API

I'm not sure about adding testing.  This is my first postgres
patch, so any guidance on adding regression testing would be
appreciated.

The rationale for this has come from the need to know the source
function name, and we've typically resorted to things like this in
the past:

CREATE OR REPLACE FUNCTION throw_warning() RETURNS text LANGUAGE
plperlu AS $function$
my $function_name = 'throw_warning';
$SIG{__WARN__} = sub { print STDERR "In Function: $function_name:
$_[0]\n"; }
$function$
;

We've literally had to copy/paste this all over and it's something
that postgres should just 'give you' since it knows the name
already, just like when triggers pass you $_TD with all the
pertinent information

A wishlist item would be for postgres plperl to automatically
prepend the function name and schema when throwing perl warnings so
you don't have to do your own __WARN__ handler, but this is the
next best thing.

I'm not necessarily opposed to this, but the analogy to $_TD isn't
really apt.  You can't know the trigger data at compile time,
whereas you can know the function's name at compile time, using just
the mechanism you find irksome.

And if we're going to do it for plperl, shouldn't we do it for other
PLs?

Hi Andrew,

Thanks for the feedback.

1) Why is this not similar to _TD?  It literally operates
identically. At run-time it passes you $_TD  for triggers. Same her
for functions.  This is all run-time.   What exactly is the issue
you're trying to point out?

It's not the same as the trigger data case because the function name
is knowable at compile time, as in fact you have demonstrated. You
just find it a bit inconvenient to have to code for that knowledge. By
contrast, trigger data is ONLY knowable at run time.

But I don't see that it is such a heavy burden to have to write

  my $funcname = "foo";

or similar in your function.

cheers

andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

Understood, regarding knowability.  Trigger data is definitely going to
be very dynamic in that regard.

No, It's not a heavy burden to hard code the function name.  But what my
ideal goal would be is this:

CREATE OR REPLACE FUNCTION throw_warning() RETURNS text LANGUAGE plperlu
AS $function$
use 'PostgresWarnHandler'; # <--- imagine this registers a WARN handler
and outputs $_FN->{name} for you as part of the final warning

my $a;
print $a;

.... etc

and then there's nothing 'hard coded' regarding the name of the
function, anywhere.  It just seems nonsensical that postgres plperl
can't send you the name of your registered function and there's
absolutely no way to get it other than hard coding the name
(redundantly, considering you're already know the name when you're
defining the function in the first place)

But even better would be catching the warn at the plperl level,
prepending the function name to the warn message, and then you only need:

CREATE OR REPLACE FUNCTION throw_warning() RETURNS text LANGUAGE plperlu
AS $function$

my $a;
print $a;

.... etc

And then in this hypothetical situation -- magic ensues, and you're left
with this:
# tail -f /var/log/postgresql.log
******* GOT A WARNING - Use of uninitialized value $a in concatenation
(.) or string in function public.throw_warning() line 1

On 2024-08-29 Th 5:50 PM, Mark Murawski wrote:

On 8/29/24 16:54, Andrew Dunstan wrote:

On 2024-08-29 Th 1:01 PM, Mark Murawski wrote:

On 8/29/24 11:56, Andrew Dunstan wrote:

On 2024-08-28 We 5:53 PM, Mark Murawski wrote:

Hi Hackers!

This would be version v1 of this feature

Basically, the subject says it all: pl/pgperl Patch for being able
to tell which function you're in.
This is a hashref so it will be possible to populate new and
exciting other details in the future as the need arises

This also greatly improves logging capabilities for things like
catching warnings,  Because as it stands right now, there's no
information that can assist with locating the source of a warning
like this:

# tail -f /var/log/postgresql.log
******* GOT A WARNING - Use of uninitialized value $prefix in
concatenation (.) or string at (eval 531) line 48.

Now, with $_FN you can do this:

CREATE OR REPLACE FUNCTION throw_warning() RETURNS text LANGUAGE
plperlu AS $function$

use warnings;
use strict;
use Data::Dumper;

$SIG{__WARN__} = sub {
  elog(NOTICE, Dumper($_FN));

  print STDERR "In Function: $_FN->{name}: $_[0]\n";
};

my $a;
print "$a"; # uninit!

return undef;

$function$
;

This patch is against 12 which is still our production branch.
This could easily be also patched against newer releases as well.

I've been using this code in production now for about 3 years,
it's greatly helped track down issues.  And there shouldn't be
anything platform-specific here, it's all regular perl API

I'm not sure about adding testing.  This is my first postgres
patch, so any guidance on adding regression testing would be
appreciated.

The rationale for this has come from the need to know the source
function name, and we've typically resorted to things like this in
the past:

CREATE OR REPLACE FUNCTION throw_warning() RETURNS text LANGUAGE
plperlu AS $function$
my $function_name = 'throw_warning';
$SIG{__WARN__} = sub { print STDERR "In Function: $function_name:
$_[0]\n"; }
$function$
;

We've literally had to copy/paste this all over and it's something
that postgres should just 'give you' since it knows the name
already, just like when triggers pass you $_TD with all the
pertinent information

A wishlist item would be for postgres plperl to automatically
prepend the function name and schema when throwing perl warnings
so you don't have to do your own __WARN__ handler, but this is the
next best thing.

I'm not necessarily opposed to this, but the analogy to $_TD isn't
really apt.  You can't know the trigger data at compile time,
whereas you can know the function's name at compile time, using
just the mechanism you find irksome.

And if we're going to do it for plperl, shouldn't we do it for
other PLs?

Hi Andrew,

Thanks for the feedback.

1) Why is this not similar to _TD?  It literally operates
identically. At run-time it passes you $_TD  for triggers. Same her
for functions.  This is all run-time.   What exactly is the issue
you're trying to point out?

It's not the same as the trigger data case because the function name
is knowable at compile time, as in fact you have demonstrated. You
just find it a bit inconvenient to have to code for that knowledge.
By contrast, trigger data is ONLY knowable at run time.

But I don't see that it is such a heavy burden to have to write

  my $funcname = "foo";

or similar in your function.

cheers

andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

Understood, regarding knowability.  Trigger data is definitely going
to be very dynamic in that regard.

No, It's not a heavy burden to hard code the function name.  But what
my ideal goal would be is this:

CREATE OR REPLACE FUNCTION throw_warning() RETURNS text LANGUAGE
plperlu AS $function$
use 'PostgresWarnHandler'; # <--- imagine this registers a WARN
handler and outputs $_FN->{name} for you as part of the final warning

my $a;
print $a;

.... etc

and then there's nothing 'hard coded' regarding the name of the
function, anywhere.  It just seems nonsensical that postgres plperl
can't send you the name of your registered function and there's
absolutely no way to get it other than hard coding the name
(redundantly, considering you're already know the name when you're
defining the function in the first place)

But even better would be catching the warn at the plperl level,
prepending the function name to the warn message, and then you only need:

CREATE OR REPLACE FUNCTION throw_warning() RETURNS text LANGUAGE
plperlu AS $function$

my $a;
print $a;

.... etc

And then in this hypothetical situation -- magic ensues, and you're
left with this:
# tail -f /var/log/postgresql.log
******* GOT A WARNING - Use of uninitialized value $a in concatenation
(.) or string in function public.throw_warning() line 1

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

On 2024-08-30 Fr 3:49 PM, Andrew Dunstan wrote:

Sorry for empty reply. Errant finger hit send.

cheers

andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

On 8/30/24 16:12, Andrew Dunstan wrote:

Sorry for empty reply. Errant finger hit send.

No problem.

So anyway... my main point is to highlight this:

On 2024-08-29 Th 5:50 PM, Mark Murawski wrote:

And then in this hypothetical situation -- magic ensues, and you're
left with this:
# tail -f /var/log/postgresql.log
******* GOT A WARNING - Use of uninitialized value $a in
concatenation (.) or string in function public.throw_warning() line 1

The essential element here is:  Why does every single developer who ever
wants to develop in plperl be forced to figure out (typically at the
worst possible time) that Postgres doesn't log the source function of
warning.  And then be forced to hard code their own function name as a
variable inside their function.  The typical situation is you test your
code, you push it to production, and then observe.  And then production
does something you didn't expect and throws a warning.  With the current
design, you have no idea what code threw the warning and you have to go
into every single possible plperl function and throw in hard coded
function names for logging. To me this is highly nonsensical to force
this on developers.

Pretty much every modern scripting language I've come across, has a way
to access dynamically: the name of the currently executing function. 
Either by way of a special variable, or a stack trace introspection. 
Being that this is Perl, sure... we can get caller() or a stack trace. 
But the design of plperl Postgres functions uses an anonymous coderef to
define the function, so there is no function name defined. I don't see
the harm in adding more information so that the running function can
know its own name.

Maybe another approach to the fix here is to give the function an actual
name, and when calling it, now you know where you are executing from. 
But in perl you cannot define a sub called schema.function, it
definitely does not compile.  So you would need some sort of name
mangling like postgres_plperl_schema_function so it's painfully obvious
where it came from.  So... this is why it's handy to just have a
variable, since you can format the called schema.function properly.

But, Ideally the even better solution or just catching and re-throwing
the warn handler sounds like it would be the best option.  I'll need to
look into this more since this is really my first jump into the perl-c
api and I've never worked with warn handlers at this level.