Re: pgsql: Make the pg_stat_activity view call a SRF

Log Message:
-----------
Make the pg_stat_activity view call a SRF (pg_stat_get_activity())
instead of calling a bunch of individual functions.

This function can also be called directly, taking a PID as an argument, to
return only the data for a single PID.

Modified Files:
--------------
pgsql/doc/src/sgml:
monitoring.sgml (r1.57 -> r1.58)
(http://anoncvs.postgresql.org/cvsweb.cgi/pgsql/doc/src/sgml/monitoring.sgml?r1=1.57&r2=1.58)
pgsql/src/backend/catalog:
system_views.sql (r1.49 -> r1.50)
(http://anoncvs.postgresql.org/cvsweb.cgi/pgsql/src/backend/catalog/system_views.sql?r1=1.49&r2=1.50)
pgsql/src/backend/utils/adt:
pgstatfuncs.c (r1.49 -> r1.50)
(http://anoncvs.postgresql.org/cvsweb.cgi/pgsql/src/backend/utils/adt/pgstatfuncs.c?r1=1.49&r2=1.50)
pgsql/src/include/catalog:
catversion.h (r1.455 -> r1.456)
(http://anoncvs.postgresql.org/cvsweb.cgi/pgsql/src/include/catalog/catversion.h?r1=1.455&r2=1.456)
pg_proc.h (r1.496 -> r1.497)
(http://anoncvs.postgresql.org/cvsweb.cgi/pgsql/src/include/catalog/pg_proc.h?r1=1.496&r2=1.497)
pgsql/src/test/regress/expected:
rules.out (r1.136 -> r1.137)
(http://anoncvs.postgresql.org/cvsweb.cgi/pgsql/src/test/regress/expected/rules.out?r1=1.136&r2=1.137)

mha(at)postgresql(dot)org (Magnus Hagander) writes:
> Make the pg_stat_activity view call a SRF (pg_stat_get_activity())
> instead of calling a bunch of individual functions.

> This function can also be called directly, taking a PID as an argument, to
> return only the data for a single PID.

Hmm, if you really intend the function to be used directly, then having
to provide the output column list for it is a pretty big usability hit.
Why not declare it with OUT parameters, instead?

regards, tom lane

Tom Lane wrote:
> mha(at)postgresql(dot)org (Magnus Hagander) writes:
> > Make the pg_stat_activity view call a SRF (pg_stat_get_activity())
> > instead of calling a bunch of individual functions.
>
> > This function can also be called directly, taking a PID as an
> > argument, to return only the data for a single PID.
>
> Hmm, if you really intend the function to be used directly, then
> having to provide the output column list for it is a pretty big
> usability hit. Why not declare it with OUT parameters, instead?

That does sound like a very good idea... I did notice that as being a
problem, but didn't know how to fix it without thinking more :-)
How do I do that in pg_proc.h? Is there some other function that does
this that I can peek at for inspiration?

//Magnus

Magnus Hagander <magnus(at)hagander(dot)net> writes:
> Tom Lane wrote:
>> Why not declare it with OUT parameters, instead?

> That does sound like a very good idea... I did notice that as being a
> problem, but didn't know how to fix it without thinking more :-)
> How do I do that in pg_proc.h? Is there some other function that does
> this that I can peek at for inspiration?

Sure, see pg_timezone_abbrevs(), pg_timezone_names().

(This only started to work recently, which is why we have so many
record-returning functions that don't do it that way. It might be
an idea to fix them all sooner or later.)

regards, tom lane

Tom Lane wrote:
> Magnus Hagander <magnus(at)hagander(dot)net> writes:
> > Tom Lane wrote:
> >> Why not declare it with OUT parameters, instead?
>
> > That does sound like a very good idea... I did notice that as being
> > a problem, but didn't know how to fix it without thinking more :-)
> > How do I do that in pg_proc.h? Is there some other function that
> > does this that I can peek at for inspiration?
>
> Sure, see pg_timezone_abbrevs(), pg_timezone_names().
>
> (This only started to work recently, which is why we have so many
> record-returning functions that don't do it that way. It might be
> an idea to fix them all sooner or later.)

Wow, that was easy. Not sure where I got the impression it was hard
from - probably last looked at it before it was made easy, and didn't
re-check :-)

Updated the pg_stat_get_activity() function to use this.

And yes, I agree that it's probably a very good idea to go over our
other SRFs and fix them all. I can take a look at that eventually, but
for now I think we stick it on the TODO?

//Magnus

Magnus Hagander wrote:
> > Sure, see pg_timezone_abbrevs(), pg_timezone_names().
> >
> > (This only started to work recently, which is why we have so many
> > record-returning functions that don't do it that way. It might be
> > an idea to fix them all sooner or later.)
>
> Wow, that was easy. Not sure where I got the impression it was hard
> from - probably last looked at it before it was made easy, and didn't
> re-check :-)
>
> Updated the pg_stat_get_activity() function to use this.
>
> And yes, I agree that it's probably a very good idea to go over our
> other SRFs and fix them all. I can take a look at that eventually, but
> for now I think we stick it on the TODO?

Added to TODO:

* Fix system views like pg_stat_all_tables to use set-returning
functions, rather than views of per-column functions

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +

Bruce Momjian wrote:
> Magnus Hagander wrote:
>>> Sure, see pg_timezone_abbrevs(), pg_timezone_names().
>>>
>>> (This only started to work recently, which is why we have so many
>>> record-returning functions that don't do it that way. It might be
>>> an idea to fix them all sooner or later.)
>> Wow, that was easy. Not sure where I got the impression it was hard
>> from - probably last looked at it before it was made easy, and didn't
>> re-check :-)
>>
>> Updated the pg_stat_get_activity() function to use this.
>>
>> And yes, I agree that it's probably a very good idea to go over our
>> other SRFs and fix them all. I can take a look at that eventually, but
>> for now I think we stick it on the TODO?
>
> Added to TODO:
>
> * Fix system views like pg_stat_all_tables to use set-returning
> functions, rather than views of per-column functions

Thanks, and while I approve of that TODO, that's not actually the one I
was talking about in the email. The one I was talking about was "change
builtin set-returning functions to use OUT parameters so you can query
them without knowing the result format" or something like that.

So, please keep the one you added, but add this one as well.

//Magnus

Magnus Hagander wrote:
> Bruce Momjian wrote:
> > Magnus Hagander wrote:
> >>> Sure, see pg_timezone_abbrevs(), pg_timezone_names().
> >>>
> >>> (This only started to work recently, which is why we have so many
> >>> record-returning functions that don't do it that way. It might be
> >>> an idea to fix them all sooner or later.)
> >> Wow, that was easy. Not sure where I got the impression it was hard
> >> from - probably last looked at it before it was made easy, and didn't
> >> re-check :-)
> >>
> >> Updated the pg_stat_get_activity() function to use this.
> >>
> >> And yes, I agree that it's probably a very good idea to go over our
> >> other SRFs and fix them all. I can take a look at that eventually, but
> >> for now I think we stick it on the TODO?
> >
> > Added to TODO:
> >
> > * Fix system views like pg_stat_all_tables to use set-returning
> > functions, rather than views of per-column functions
>
> Thanks, and while I approve of that TODO, that's not actually the one I
> was talking about in the email. The one I was talking about was "change
> builtin set-returning functions to use OUT parameters so you can query
> them without knowing the result format" or something like that.
>
> So, please keep the one you added, but add this one as well.

Uh, I need more details on this. Can you give an example?

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +

Bruce Momjian <bruce(at)momjian(dot)us> writes:
> Magnus Hagander wrote:
>> Bruce Momjian wrote:
>>> * Fix system views like pg_stat_all_tables to use set-returning
>>> functions, rather than views of per-column functions
>>
>> Thanks, and while I approve of that TODO, that's not actually the one I
>> was talking about in the email. The one I was talking about was "change
>> builtin set-returning functions to use OUT parameters so you can query
>> them without knowing the result format" or something like that.
>>
>> So, please keep the one you added, but add this one as well.

> Uh, I need more details on this. Can you give an example?

Good:

regression=# select * from pg_get_keywords();
word | catcode | catdesc
-------------------+---------+-----------------------
abort | U | Unreserved
absolute | U | Unreserved
access | U | Unreserved
...

Not so good:

regression=# select * from pg_show_all_settings();
ERROR: a column definition list is required for functions returning "record"

There's no longer any very good reason for built-in SRFs to not define
their own output record type.

regards, tom lane

On Sat, Aug 16, 2008 at 12:03 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>
> regression=# select * from pg_show_all_settings();
> ERROR: a column definition list is required for functions returning "record"
>
> There's no longer any very good reason for built-in SRFs to not define
> their own output record type.
>

is there any one doing this? if not i want to give it a try... seems
easy enough, even for me :)

--
regards,
Jaime Casanova
Soporte y capacitación de PostgreSQL
Asesoría y desarrollo de sistemas
Guayaquil - Ecuador
Cel. (593) 87171157

Jaime Casanova wrote:
> On Sat, Aug 16, 2008 at 12:03 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> regression=# select * from pg_show_all_settings();
>> ERROR: a column definition list is required for functions returning "record"
>>
>> There's no longer any very good reason for built-in SRFs to not define
>> their own output record type.
>>
>
> is there any one doing this? if not i want to give it a try... seems
> easy enough, even for me :)

It's on my list, but I haven't actually started it yet. So - take it away!

//Magnus

Tom Lane wrote:
> >> Thanks, and while I approve of that TODO, that's not actually the one I
> >> was talking about in the email. The one I was talking about was "change
> >> builtin set-returning functions to use OUT parameters so you can query
> >> them without knowing the result format" or something like that.
> >>
> >> So, please keep the one you added, but add this one as well.
>
> > Uh, I need more details on this. Can you give an example?
>
> Good:
>
> regression=# select * from pg_get_keywords();
> word | catcode | catdesc
> -------------------+---------+-----------------------
> abort | U | Unreserved
> absolute | U | Unreserved
> access | U | Unreserved
> ...
>
> Not so good:
>
> regression=# select * from pg_show_all_settings();
> ERROR: a column definition list is required for functions returning "record"
>
> There's no longer any very good reason for built-in SRFs to not define
> their own output record type.

TODO updated:

* Fix all set-returning system functions so they support a wildcard
target list

SELECT * FROM pg_get_keywords() works but SELECT * FROM
pg_show_all_settings() does not.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +

On Monday 18 August 2008 10:53:51 Bruce Momjian wrote:
> Tom Lane wrote:
> > >> Thanks, and while I approve of that TODO, that's not actually the one
> > >> I was talking about in the email. The one I was talking about was
> > >> "change builtin set-returning functions to use OUT parameters so you
> > >> can query them without knowing the result format" or something like
> > >> that.
> > >>
> > >> So, please keep the one you added, but add this one as well.
> > >
> > > Uh, I need more details on this. Can you give an example?
> >
> > Good:
> >
> > regression=# select * from pg_get_keywords();
> > word | catcode | catdesc
> > -------------------+---------+-----------------------
> > abort | U | Unreserved
> > absolute | U | Unreserved
> > access | U | Unreserved
> > ...
> >
> > Not so good:
> >
> > regression=# select * from pg_show_all_settings();
> > ERROR: a column definition list is required for functions returning
> > "record"
> >
> > There's no longer any very good reason for built-in SRFs to not define
> > their own output record type.
>
> TODO updated:
>
> * Fix all set-returning system functions so they support a wildcard
> target list
>
> SELECT * FROM pg_get_keywords() works but SELECT * FROM
> pg_show_all_settings() does not.
>

If this isn't critical, and no one is working on it yet, I can see about
whittling away at it for 8.4.

--
Robert Treat
Build A Brighter LAMP :: Linux Apache {middleware} PostgreSQL

Robert Treat <xzilla(at)users(dot)sourceforge(dot)net> writes:
> On Monday 18 August 2008 10:53:51 Bruce Momjian wrote:
>> TODO updated:
>> * Fix all set-returning system functions so they support a wildcard
>> target list

> If this isn't critical, and no one is working on it yet, I can see about
> whittling away at it for 8.4.

I think someone did already volunteer.

regards, tom lane

On Tue, Aug 19, 2008 at 10:03:04PM -0400, Robert Treat wrote:
> On Monday 18 August 2008 10:53:51 Bruce Momjian wrote:
> > Tom Lane wrote:
> > > >> Thanks, and while I approve of that TODO, that's not actually the one
> > > >> I was talking about in the email. The one I was talking about was
> > > >> "change builtin set-returning functions to use OUT parameters so you
> > > >> can query them without knowing the result format" or something like
> > > >> that.
> > > >>
> > > >> So, please keep the one you added, but add this one as well.
> > > >
> > > > Uh, I need more details on this. Can you give an example?
> > >
> > > Good:
> > >
> > > regression=# select * from pg_get_keywords();
> > > word | catcode | catdesc
> > > -------------------+---------+-----------------------
> > > abort | U | Unreserved
> > > absolute | U | Unreserved
> > > access | U | Unreserved
> > > ...
> > >
> > > Not so good:
> > >
> > > regression=# select * from pg_show_all_settings();
> > > ERROR: a column definition list is required for functions returning
> > > "record"
> > >
> > > There's no longer any very good reason for built-in SRFs to not define
> > > their own output record type.
> >
> > TODO updated:
> >
> > * Fix all set-returning system functions so they support a wildcard
> > target list
> >
> > SELECT * FROM pg_get_keywords() works but SELECT * FROM
> > pg_show_all_settings() does not.
> >
>
> If this isn't critical, and no one is working on it yet, I can see about
> whittling away at it for 8.4.

Looks like there are just 5 of these:

SELECT n.nspname as "Schema",
p.proname as "Name",
pg_catalog.pg_get_function_result(p.oid) as "Result data type",
pg_catalog.pg_get_function_arguments(p.oid) as "Argument data types"
FROM pg_catalog.pg_proc p
LEFT JOIN pg_catalog.pg_namespace n ON n.oid = p.pronamespace
WHERE p.prorettype <> 'pg_catalog.cstring'::pg_catalog.regtype
AND p.proargtypes[0] IS DISTINCT FROM 'pg_catalog.cstring'::pg_catalog.regtype
AND NOT p.proisagg
AND pg_catalog.pg_get_function_result(p.oid) = 'SETOF record'
AND pg_catalog.pg_get_function_arguments(p.oid) !~ 'OUT'

Schema | Name | Result data type | Argument data types
------------+-----------------------+------------------+---------------------
pg_catalog | pg_cursor | SETOF record |
pg_catalog | pg_lock_status | SETOF record |
pg_catalog | pg_prepared_statement | SETOF record |
pg_catalog | pg_prepared_xact | SETOF record |
pg_catalog | pg_show_all_settings | SETOF record |
(5 rows)

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

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

On Tue, Aug 19, 2008 at 10:56 PM, David Fetter <david(at)fetter(dot)org> wrote:
>>
>> If this isn't critical, and no one is working on it yet, I can see about
>> whittling away at it for 8.4.
>

i'm doing it...

> Looks like there are just 5 of these:
>
> pg_catalog | pg_cursor | SETOF record |
> pg_catalog | pg_lock_status | SETOF record |
> pg_catalog | pg_prepared_statement | SETOF record |
> pg_catalog | pg_prepared_xact | SETOF record |
> pg_catalog | pg_show_all_settings | SETOF record |

i have done pg_lock_status and pg_show_all_settings the other three
will do tomorrow

--
regards,
Jaime Casanova
Soporte y capacitación de PostgreSQL
Asesoría y desarrollo de sistemas
Guayaquil - Ecuador
Cel. (593) 87171157