Improved \df(+) in psql + backward-compatibility
Folks,
I've noticed that \df doesn't do quite what it might when a function
is created with named input parameters. Please find enclosed a patch
against CVS TIP that does this better.
On a slightly related note, I've noticed that psql isn't backward
compatible. I know that it's *very* late to be introducing things,
but I consider this a bug, and would like to send in more fixes.
What do you all think?
Cheers,
D
--
David Fetter david@fetter.org http://fetter.org/
phone: +1 510 893 6100 mobile: +1 415 235 3778
Remember to vote!
Attachments:
psql.difftext/plain; charset=us-asciiDownload
Index: src/bin/psql/describe.c
===================================================================
RCS file: /projects/cvsroot/pgsql/src/bin/psql/describe.c,v
retrieving revision 1.124
diff -c -r1.124 describe.c
*** src/bin/psql/describe.c 14 Aug 2005 19:20:45 -0000 1.124
--- src/bin/psql/describe.c 29 Aug 2005 02:57:14 -0000
***************
*** 170,186 ****
"SELECT n.nspname as \"%s\",\n"
" p.proname as \"%s\",\n"
" CASE WHEN p.proretset THEN 'setof ' ELSE '' END ||\n"
! " pg_catalog.format_type(p.prorettype, NULL) as \"%s\",\n"
! " pg_catalog.oidvectortypes(p.proargtypes) as \"%s\"",
! _("Schema"), _("Name"), _("Result data type"),
! _("Argument data types"));
if (verbose)
appendPQExpBuffer(&buf,
",\n r.rolname as \"%s\",\n"
" l.lanname as \"%s\",\n"
" p.prosrc as \"%s\",\n"
! " pg_catalog.obj_description(p.oid, 'pg_proc') as \"%s\"",
_("Owner"), _("Language"),
_("Source code"), _("Description"));
--- 170,203 ----
"SELECT n.nspname as \"%s\",\n"
" p.proname as \"%s\",\n"
" CASE WHEN p.proretset THEN 'setof ' ELSE '' END ||\n"
! " pg_catalog.format_type(p.prorettype, NULL) as \"%s\",\n",
! _("Schema"), _("Name"), _("Result data type"));
+ if (pset.sversion < 80000)
+ {
+ appendPQExpBuffer(&buf,
+ " pg_catalog.oidvectortypes(p.proargtypes) as \"%s\"",
+ _("Argument data types"));
+ }
+ else
+ {
+ appendPQExpBuffer(&buf,
+ " CASE WHEN array_upper(p.proargnames, 1)>0 THEN\n"
+ " array_to_string(ARRAY(\n"
+ " SELECT (p.proargnames)[s.a] || ' ' || (string_to_array(pg_catalog.oidvectortypes(p.proargtypes), ', ' ))[s.a]\n"
+ " FROM generate_series(1,p.pronargs) AS s(a)\n"
+ " ), ', ')\n"
+ " ELSE\n"
+ " pg_catalog.oidvectortypes(p.proargtypes)\n"
+ " END as \"%s\"",
+ _("Argument data types"));
+ }
if (verbose)
appendPQExpBuffer(&buf,
",\n r.rolname as \"%s\",\n"
" l.lanname as \"%s\",\n"
" p.prosrc as \"%s\",\n"
! " pg_catalog.obj_description(p.oid, 'pg_proc') as \"%s\"",
_("Owner"), _("Language"),
_("Source code"), _("Description"));
***************
*** 192,198 ****
appendPQExpBuffer(&buf,
"\nFROM pg_catalog.pg_proc p"
"\n LEFT JOIN pg_catalog.pg_namespace n ON n.oid = p.pronamespace"
! "\n LEFT JOIN pg_catalog.pg_language l ON l.oid = p.prolang"
"\n LEFT JOIN pg_catalog.pg_roles r ON r.oid = p.proowner\n");
/*
--- 209,215 ----
appendPQExpBuffer(&buf,
"\nFROM pg_catalog.pg_proc p"
"\n LEFT JOIN pg_catalog.pg_namespace n ON n.oid = p.pronamespace"
! "\n LEFT JOIN pg_catalog.pg_language l ON l.oid = p.prolang"
"\n LEFT JOIN pg_catalog.pg_roles r ON r.oid = p.proowner\n");
/*
David Fetter <david@fetter.org> writes:
On a slightly related note, I've noticed that psql isn't backward
compatible.
We have never expected psql's \d commands to work against older server
versions, and two months after feature freeze isn't the time to start
making that happen.
regards, tom lane
On Monday 29 August 2005 00:33, Tom Lane wrote:
David Fetter <david@fetter.org> writes:
On a slightly related note, I've noticed that psql isn't backward
compatible.We have never expected psql's \d commands to work against older server
versions, and two months after feature freeze isn't the time to start
making that happen.
That said, number of folks have looked at this problem and agree it would be
nice to do, they just haven't formed a consensus on how to do it. If you
have a plan for how you would want to approach this in 8.2, feel free to post
it.
--
Robert Treat
Build A Brighter Lamp :: Linux Apache {middleware} PostgreSQL
On Mon, Aug 29, 2005 at 08:12:37AM -0400, Robert Treat wrote:
On Monday 29 August 2005 00:33, Tom Lane wrote:
David Fetter <david@fetter.org> writes:
On a slightly related note, I've noticed that psql isn't
backward compatible.We have never expected psql's \d commands to work against older
server versions, and two months after feature freeze isn't the
time to start making that happen.
Tom, good point on the timing. I wish I'd come up with this at a
better moment for 8.1. I still contend that this falls squarely in
the realm of bug fixes, and would be happy to get as
backward-compatible a bunch of \ functions as possible, even
back-porting, little as I believe that should be encouraged.
That said, number of folks have looked at this problem and agree it
would be nice to do, they just haven't formed a consensus on how to
do it. If you have a plan for how you would want to approach this
in 8.2, feel free to post it.
I'd be curious as to what's been proposed before, but briefly:
switch (pset.sversion/100) {
case 801:
...
break;
case 800:
...
break;
.
.
.
default:
...
break;
}
for each version-specific chunk of SQL that a backslash command would
issue. I know it makes the code bigger, but stacking things that way
makes it relatively easy to prepend new conditions at the top as
needed. Also TBD, I think it would be a Very Good Idea(TM) to see
about how this (and other front ends) can play nice with the
newsysviews project. It really chaps me to see all these wheels being
reinvented. :/
Cheers,
D
--
David Fetter david@fetter.org http://fetter.org/
phone: +1 510 893 6100 mobile: +1 415 235 3778
Remember to vote!
David Fetter wrote:
On Mon, Aug 29, 2005 at 08:12:37AM -0400, Robert Treat wrote:
On Monday 29 August 2005 00:33, Tom Lane wrote:
David Fetter <david@fetter.org> writes:
On a slightly related note, I've noticed that psql isn't
backward compatible.We have never expected psql's \d commands to work against older
server versions, and two months after feature freeze isn't the
time to start making that happen.Tom, good point on the timing. I wish I'd come up with this at a
better moment for 8.1. I still contend that this falls squarely in
the realm of bug fixes
[ -patches removed ]
I don't see how, if it is not functionality that has been explicitly or
implicitly promised. The fact that it isn't what you expected doesn't
make it a bug.
There's a natural tendency to want to call things bugs at this stage of
the cycle so that they qualify for application, but there's a reason we
have a freeze, and it needs to be adhered to.
If we're going to do backwards compatibility for psql then we need to do
it in a fairly comprehensive way, not bit by bit, because we can
reasonably say either "we support backwards compatibility" or "we don't
support backwards compatibility", but we cannot reasonably say "we
support backwards compatibility just for these commands" - that's way
too confusing. The task is probably non-trivial - just look at pg_dump.
Might be another good starting hackers project.
cheers
andreew
Andrew Dunstan <andrew@dunslane.net> writes:
If we're going to do backwards compatibility for psql then we need to do
it in a fairly comprehensive way, not bit by bit, because we can
reasonably say either "we support backwards compatibility" or "we don't
support backwards compatibility", but we cannot reasonably say "we
support backwards compatibility just for these commands" - that's way
too confusing.
Yeah. It would be good to set some parameters before starting: how far
back is reasonable to support? pg_dump goes back to 7.0 but that's now
mostly for historical reasons, ie, 7.0 was the immediately previous
release when we started making it do backwards-compatible dumps. I'm
not sure it's worth the trouble to make psql go that far back. "Back
to 8.0" would be a nice round figure...
The task is probably non-trivial - just look at pg_dump.
Might be another good starting hackers project.
If you consulted the back branches of psql source code it wouldn't be
too hard I would think, though surely tedious.
As an aside, I would most certainly NOT use the "switch" coding style
suggested upthread, as that is guaranteed to break completely every time
there's a version bump. Do it the way pg_dump does, with a series of
"if (version >= something)" tests. Then you only have to change a given
piece of code when there's a direct reason to change it.
regards, tom lane
Tom Lane wrote:
It would be good to set some parameters before starting: how far
back is reasonable to support? pg_dump goes back to 7.0 but that's now
mostly for historical reasons, ie, 7.0 was the immediately previous
release when we started making it do backwards-compatible dumps. I'm
not sure it's worth the trouble to make psql go that far back. "Back
to 8.0" would be a nice round figure...
Seems fair for an initial target of 8.2. Going forward a policy of "3
years or 3 releases back" seems about right. If people can't bring
themselves to upgrade in that time they can be left on their own.
cheers
andrew
David Fetter <david@fetter.org> writes:
I've noticed that \df doesn't do quite what it might when a function
is created with named input parameters. Please find enclosed a patch
against CVS TIP that does this better.
Meanwhile, getting back to the actual merits of the patch ... this is
not right yet, because it will do the wrong thing when there are OUT
parameters. (The proargnames array includes both IN and OUT params,
and you can't assume that proargnames and proargtypes have corresponding
subscripts.) It would probably be a good idea to discuss what display
we want for a function with OUT parameters, anyway. The strict columnar
representation that \df currently uses doesn't scale very well :-(
regards, tom lane
On Mon, Aug 29, 2005 at 11:13:29AM -0400, Tom Lane wrote:
David Fetter <david@fetter.org> writes:
I've noticed that \df doesn't do quite what it might when a
function is created with named input parameters. Please find
enclosed a patch against CVS TIP that does this better.Meanwhile, getting back to the actual merits of the patch ... this
is not right yet, because it will do the wrong thing when there are
OUT parameters.
Right. I'd tried doing something with INOUT and OUT parameters, but I
wasn't able to figure out how to do with oid[] what I'd do with
oidvector. On the bright side, what I did does do the right thing if
there are named IN parameters, which was part of what I was trying to
fix.
(The proargnames array includes both IN and OUT params, and you
can't assume that proargnames and proargtypes have corresponding
subscripts.) It would probably be a good idea to discuss what
display we want for a function with OUT parameters, anyway. The
strict columnar representation that \df currently uses doesn't scale
very well :-(
Speaking of said psql's columnar representations, what about the
alignment thing proposed earlier where an embedded newline doesn't
mess up the alignment of everything else? Is there some generic way
to handle this?
Cheers,
D
--
David Fetter david@fetter.org http://fetter.org/
phone: +1 510 893 6100 mobile: +1 415 235 3778
Remember to vote!
David Fetter <david@fetter.org> writes:
Speaking of said psql's columnar representations, what about the
alignment thing proposed earlier where an embedded newline doesn't
mess up the alignment of everything else? Is there some generic way
to handle this?
If that's not on TODO already, it certainly should be --- but again,
I don't see that happening for 8.1. We've lived with it for years
so it hardly counts as "must fix for 8.1".
Were you thinking of proposing a multiline output format for \df when
there are named parameters? We could plan on doing that in 8.2,
assuming someone steps up to fix the formatting code by then.
regards, tom lane
Seems this item will have to remain for 8.2. I have added this to TODO:
o Display IN, INOUT, and OUT parameters in \df+
It probably requires psql to output newlines in the proper
column, which is already on the TODO list.
---------------------------------------------------------------------------
Tom Lane wrote:
David Fetter <david@fetter.org> writes:
I've noticed that \df doesn't do quite what it might when a function
is created with named input parameters. Please find enclosed a patch
against CVS TIP that does this better.Meanwhile, getting back to the actual merits of the patch ... this is
not right yet, because it will do the wrong thing when there are OUT
parameters. (The proargnames array includes both IN and OUT params,
and you can't assume that proargnames and proargtypes have corresponding
subscripts.) It would probably be a good idea to discuss what display
we want for a function with OUT parameters, anyway. The strict columnar
representation that \df currently uses doesn't scale very well :-(regards, tom lane
---------------------------(end of broadcast)---------------------------
TIP 4: Have you searched our list archives?
--
Bruce Momjian | http://candle.pha.pa.us
pgman@candle.pha.pa.us | (610) 359-1001
+ If your life is a hard drive, | 13 Roberts Road
+ Christ can be your backup. | Newtown Square, Pennsylvania 19073