Обсуждение: possible proposal plpgsql GET DIAGNOSTICS oid = PG_ROUTINE_OID

Hi

I have a question about the possibility of simply getting the name of the currently executed function. The reason for this request is simplification of writing debug messages.

GET DIAGNOSTICS _oid = PG_ROUTINE_OID;

RAISE NOTICE '... % ... %', _oid, _oid::regproc::text;

The advantage of this dynamic access to function name is always valid value not sensitive to some renaming or moving between schemas.

I am able to separate a name from context, but it can be harder to write this separation really robustly. It can be very easy to enhance the GET DIAGNOSTICS statement to return the oid of currently executed function. 

Do you think it can be useful feature?

The implementation should be trivial.

Comments, notes?

Regards

Pavel

On Tue, Feb 07, 2023 at 08:48:22PM +0100, Pavel Stehule wrote:
>
> I have a question about the possibility of simply getting the name of the
> currently executed function. The reason for this request is simplification
> of writing debug messages.
>
> GET DIAGNOSTICS _oid = PG_ROUTINE_OID;
> RAISE NOTICE '... % ... %', _oid, _oid::regproc::text;
>
> The advantage of this dynamic access to function name is always valid value
> not sensitive to some renaming or moving between schemas.
>
> I am able to separate a name from context, but it can be harder to write
> this separation really robustly. It can be very easy to enhance the GET
> DIAGNOSTICS statement to return the oid of currently executed function.
>
> Do you think it can be useful feature?

+1, it would have been quite handy in a few of my projects.

hi

st 8. 2. 2023 v 7:33 odesílatel Julien Rouhaud <rjuju123@gmail.com> napsal:

On Tue, Feb 07, 2023 at 08:48:22PM +0100, Pavel Stehule wrote:
>
> GET DIAGNOSTICS _oid = PG_ROUTINE_OID;
> RAISE NOTICE '... % ... %', _oid, _oid::regproc::text;
>
> Do you think it can be useful feature?

+1, it would have been quite handy in a few of my projects.

it can looks like that

create or replace function foo(a int)
returns int as $$
declare s text; n text; o oid;
begin
  get diagnostics s = pg_current_routine_signature,
                  n = pg_current_routine_name,
                  o = pg_current_routine_oid;
  raise notice 'sign:%,  name:%,  oid:%', s, n, o;
  return a;
end;
$$ language plpgsql;
CREATE FUNCTION
(2023-02-08 09:04:03) postgres=# select foo(10);
NOTICE:  sign:foo(integer),  name:foo,  oid:16392
┌─────┐
│ foo │
╞═════╡
│  10 │
└─────┘
(1 row)

The name - pg_routine_oid can be confusing, because there is not clean if it is oid of currently executed routine or routine from top of exception

Regards

Pavel

On Wed, Feb 8, 2023 at 3:08 AM Pavel Stehule <pavel.stehule@gmail.com> wrote:

hi

st 8. 2. 2023 v 7:33 odesílatel Julien Rouhaud <rjuju123@gmail.com> napsal:

On Tue, Feb 07, 2023 at 08:48:22PM +0100, Pavel Stehule wrote:
>
> GET DIAGNOSTICS _oid = PG_ROUTINE_OID;
> RAISE NOTICE '... % ... %', _oid, _oid::regproc::text;
>
> Do you think it can be useful feature?

+1, it would have been quite handy in a few of my projects.

it can looks like that

create or replace function foo(a int)
returns int as $$
declare s text; n text; o oid;
begin
  get diagnostics s = pg_current_routine_signature,
                  n = pg_current_routine_name,
                  o = pg_current_routine_oid;
  raise notice 'sign:%,  name:%,  oid:%', s, n, o;
  return a;
end;
$$ language plpgsql;
CREATE FUNCTION
(2023-02-08 09:04:03) postgres=# select foo(10);
NOTICE:  sign:foo(integer),  name:foo,  oid:16392
┌─────┐
│ foo │
╞═════╡
│  10 │
└─────┘
(1 row)

The name - pg_routine_oid can be confusing, because there is not clean if it is oid of currently executed routine or routine from top of exception

Regards

Pavel

I agree that the name changed to pg_current_routine_...  makes the most sense, great call...

+1   

On Wed, Feb 8, 2023 at 10:56 AM Kirk Wolak <wolakk@gmail.com> wrote:

On Wed, Feb 8, 2023 at 3:08 AM Pavel Stehule <pavel.stehule@gmail.com> wrote:

hi

st 8. 2. 2023 v 7:33 odesílatel Julien Rouhaud <rjuju123@gmail.com> napsal:

On Tue, Feb 07, 2023 at 08:48:22PM +0100, Pavel Stehule wrote:
>
> GET DIAGNOSTICS _oid = PG_ROUTINE_OID;
> RAISE NOTICE '... % ... %', _oid, _oid::regproc::text;
>
> Do you think it can be useful feature?

+1, it would have been quite handy in a few of my projects.

it can looks like that

create or replace function foo(a int)
returns int as $$
declare s text; n text; o oid;
begin
  get diagnostics s = pg_current_routine_signature,
                  n = pg_current_routine_name,
                  o = pg_current_routine_oid;
  raise notice 'sign:%,  name:%,  oid:%', s, n, o;
  return a;
end;
$$ language plpgsql;
CREATE FUNCTION
(2023-02-08 09:04:03) postgres=# select foo(10);
NOTICE:  sign:foo(integer),  name:foo,  oid:16392
┌─────┐
│ foo │
╞═════╡
│  10 │
└─────┘
(1 row)

The name - pg_routine_oid can be confusing, because there is not clean if it is oid of currently executed routine or routine from top of exception

Regards

Pavel

I agree that the name changed to pg_current_routine_...  makes the most sense, great call...

+1   

Okay, I reviewed this.  I tested it (allocating too small of varchar's for values, various "signature types"),

and also a performance test... Wow, on my VM, 10,000 Calls in a loop was 2-4ms...

The names are clear.  Again, I tested with various options, and including ROW_COUNT, or not.

This functions PERFECTLY....  Except there are no documentation changes.

Because of that, I set it to Waiting on Author.  

Which might be unfair, because I could take a stab at doing the documentation (but docs are not compiling on my setup yet).

The documentation changes are simple enough.

If I can get the docs compiled on my rig, I will see if I can make the changes, and post an updated patch,

that contains both...

But I don't want to be stepping on toes, or having it look like I am taking credit.

Regards - Kirk

On Sun, Mar 26, 2023 at 5:37 PM Kirk Wolak <wolakk@gmail.com> wrote:

On Wed, Feb 8, 2023 at 10:56 AM Kirk Wolak <wolakk@gmail.com> wrote:

On Wed, Feb 8, 2023 at 3:08 AM Pavel Stehule <pavel.stehule@gmail.com> wrote:

hi

st 8. 2. 2023 v 7:33 odesílatel Julien Rouhaud <rjuju123@gmail.com> napsal:

On Tue, Feb 07, 2023 at 08:48:22PM +0100, Pavel Stehule wrote:
>
> GET DIAGNOSTICS _oid = PG_ROUTINE_OID;
> RAISE NOTICE '... % ... %', _oid, _oid::regproc::text;
>
> Do you think it can be useful feature?

+1, it would have been quite handy in a few of my projects.

it can looks like that

create or replace function foo(a int)
returns int as $$
declare s text; n text; o oid;
begin
  get diagnostics s = pg_current_routine_signature,
                  n = pg_current_routine_name,
                  o = pg_current_routine_oid;
  raise notice 'sign:%,  name:%,  oid:%', s, n, o;
  return a;
end;
$$ language plpgsql;
CREATE FUNCTION
(2023-02-08 09:04:03) postgres=# select foo(10);
NOTICE:  sign:foo(integer),  name:foo,  oid:16392
┌─────┐
│ foo │
╞═════╡
│  10 │
└─────┘
(1 row)

The name - pg_routine_oid can be confusing, because there is not clean if it is oid of currently executed routine or routine from top of exception

Regards

Pavel

I agree that the name changed to pg_current_routine_...  makes the most sense, great call...

+1   

Okay, I reviewed this.  I tested it (allocating too small of varchar's for values, various "signature types"),

and also a performance test... Wow, on my VM, 10,000 Calls in a loop was 2-4ms...

The names are clear.  Again, I tested with various options, and including ROW_COUNT, or not.

This functions PERFECTLY....  Except there are no documentation changes.

Because of that, I set it to Waiting on Author.  

Which might be unfair, because I could take a stab at doing the documentation (but docs are not compiling on my setup yet).

The documentation changes are simple enough.

If I can get the docs compiled on my rig, I will see if I can make the changes, and post an updated patch,

that contains both...

But I don't want to be stepping on toes, or having it look like I am taking credit.

Regards - Kirk

Okay, I have modified the documentation and made sure it compiles.  They were simple enough changes.

I am attaching this updated patch.

I have marked the item Ready for Commiter...

Thanks for your patience.  I now have a workable hacking environment!

Regards - Kirk

 

Pavel Stehule <pavel.stehule@gmail.com> writes:
> po 27. 3. 2023 v 5:36 odesílatel Kirk Wolak <wolakk@gmail.com> napsal:
>> I have marked the item Ready for Commiter...

> Thank you for doc and for review

I'm kind of surprised there was any interest in this proposal at all,
TBH, but apparently there is some.  Still, I think you over-engineered
it by doing more than the original proposal of making the function OID
available.  The other things can be had by casting the OID to regproc
or regprocedure, so I'd be inclined to add just one new keyword not
three.  Besides, your implementation is a bit inconsistent: relying
on fn_signature could return a result that is stale or doesn't conform
to the current search_path.

                        regards, tom lane

Pavel Stehule <pavel.stehule@gmail.com> writes:
> There is reduced patch + regress tests

One more thing: I do not think it's appropriate to allow this in
GET STACKED DIAGNOSTICS.  That's about reporting the place where
an error occurred, not the current location.  Eventually it might
be interesting to retrieve the OID of the function that contained
the error, but that would be a pretty complicated patch and I am
not sure it's worth it.  In the meantime I think we should just
forbid it.

If we do that, then the confusion you were concerned about upthread
goes away and we could shorten the keyword back down to "pg_routine_oid",
which seems like a good thing for our carpal tunnels.

Thoughts?

                        regards, tom lane

Pavel Stehule <pavel.stehule@gmail.com> writes:
> út 4. 4. 2023 v 16:20 odesílatel Tom Lane <tgl@sss.pgh.pa.us> napsal:
>> If we do that, then the confusion you were concerned about upthread
>> goes away and we could shorten the keyword back down to "pg_routine_oid",
>> which seems like a good thing for our carpal tunnels.

> has sense

OK, pushed like that with some cosmetic adjustments (better test
case, mostly).

                        regards, tom lane