bugfix: --echo-hidden is not supported by \sf statements

Pavel Stehule <pavel.stehule@gmail.com> writes:

this is very simple patch - it enables hidden_queries for commands
\sf and \ef to be consistent with other describing commands.

So far as I can tell, get_create_function_cmd (and lookup_function_oid
too) were intentionally designed to not show their queries, and for that
matter they go out of their way to produce terse error output if they
fail. I'm not sure why we should revisit that choice. In any case
it seems silly to change one and not the other.

A purely stylistic gripe is that you have get_create_function_cmd taking
a PGconn parameter that now has nothing to do with its behavior.

regards, tom lane

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

2013/1/14 Tom Lane <tgl@sss.pgh.pa.us>:

Pavel Stehule <pavel.stehule@gmail.com> writes:

this is very simple patch - it enables hidden_queries for commands
\sf and \ef to be consistent with other describing commands.

So far as I can tell, get_create_function_cmd (and lookup_function_oid
too) were intentionally designed to not show their queries, and for that
matter they go out of their way to produce terse error output if they
fail. I'm not sure why we should revisit that choice. In any case
it seems silly to change one and not the other.

Motivation for this patch is consistency with other backslash
statements. Some people uses --echo-hidden option like "tutorial" or
"navigation" over system tables or builtin functionality. With this
patch these people should be navigated to function pg_get_functiondef.
There are no other motivation - jut it should to help to users that
has no necessary knowledges look to psql source code.

I am not sure about --echo-hidden option - if it is limited just to
system tables or complete functionality. Probably both designs are
valid. So first we should to decide if this behave is bug or not. I am
co-author \sf and I didn't calculate with --echo-hidden options. Now
I am inclined so it is bug. This bug is not significant - it is just
one detail - but for some who will work with psql this can be pleasant
feature if psql will be consistent in all.

After decision I can recheck this patch and enhance it for all statements.

Regards

Pavel

A purely stylistic gripe is that you have get_create_function_cmd taking
a PGconn parameter that now has nothing to do with its behavior.

regards, tom lane

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

On Mon, Jan 14, 2013 at 11:56 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

So far as I can tell, get_create_function_cmd (and lookup_function_oid
too) were intentionally designed to not show their queries, and for that
matter they go out of their way to produce terse error output if they
fail. I'm not sure why we should revisit that choice. In any case
it seems silly to change one and not the other.

Agreed on the second point, but I think I worked on that patch, and I
don't think that was intentional on my part. You worked on it, too,
IIRC, so I guess you'll have to comment on your intentions.

Personally I think -E is one of psql's finer features, so +1 from me
for making it apply across the board.

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

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

Robert Haas <robertmhaas@gmail.com> writes:

On Mon, Jan 14, 2013 at 11:56 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

So far as I can tell, get_create_function_cmd (and lookup_function_oid
too) were intentionally designed to not show their queries, and for that
matter they go out of their way to produce terse error output if they
fail. I'm not sure why we should revisit that choice. In any case
it seems silly to change one and not the other.

Agreed on the second point, but I think I worked on that patch, and I
don't think that was intentional on my part. You worked on it, too,
IIRC, so I guess you'll have to comment on your intentions.

Personally I think -E is one of psql's finer features, so +1 from me
for making it apply across the board.

Well, fine, but then it should fix both of them and remove
minimal_error_message altogether. I would however suggest eyeballing
what happens when you try "\ef nosuchfunction" (with or without -E).
I'm pretty sure that the reason for the special error handling in these
functions is that the default error reporting was unpleasant for this
use-case.

regards, tom lane

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

2013/1/14 Tom Lane <tgl@sss.pgh.pa.us>:

Robert Haas <robertmhaas@gmail.com> writes:

On Mon, Jan 14, 2013 at 11:56 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

So far as I can tell, get_create_function_cmd (and lookup_function_oid
too) were intentionally designed to not show their queries, and for that
matter they go out of their way to produce terse error output if they
fail. I'm not sure why we should revisit that choice. In any case
it seems silly to change one and not the other.

Agreed on the second point, but I think I worked on that patch, and I
don't think that was intentional on my part. You worked on it, too,
IIRC, so I guess you'll have to comment on your intentions.

Personally I think -E is one of psql's finer features, so +1 from me
for making it apply across the board.

Well, fine, but then it should fix both of them and remove
minimal_error_message altogether. I would however suggest eyeballing
what happens when you try "\ef nosuchfunction" (with or without -E).
I'm pretty sure that the reason for the special error handling in these
functions is that the default error reporting was unpleasant for this
use-case.

so I rewrote patch

still is simple

On the end I didn't use PSQLexec - just write hidden queries directly
from related functions - it is less invasive

Regards

Pavel

On Wed, Jan 23, 2013 at 12:06 PM, Pavel Stehule <pavel.stehule@gmail.com> wrote:

2013/1/14 Tom Lane <tgl@sss.pgh.pa.us>:

Well, fine, but then it should fix both of them and remove
minimal_error_message altogether. I would however suggest eyeballing
what happens when you try "\ef nosuchfunction" (with or without -E).
I'm pretty sure that the reason for the special error handling in these
functions is that the default error reporting was unpleasant for this
use-case.

so I rewrote patch

still is simple

On the end I didn't use PSQLexec - just write hidden queries directly
from related functions - it is less invasive

Sorry for the delay, but I finally had a chance to review this patch.
I think the patch does a good job of bringing the behavior of \sf and
\ef in-line with the other meta-commands as far as --echo-hidden is
concerned.

About the code changes:

The bulk of the code change comes from factoring TraceQuery() out of
PSQLexec(), so that \ef and \sf may make use of this query-printing
without going through PSQLexec(). And not using PSQLexec() lets us
avoid a somewhat uglier error message like:

ERROR: function "nonexistent" does not exist
LINE 1: SELECT 'nonexistent'::pg_catalog.regproc::pg_catalog.oid

Tom suggested removing minimal_error_message() entirely, which would
be nice if possible. It is a shame that "\sf nonexistent" produces a
scary-looking "ERROR: ...." message (and would cause any in-progress
transaction to need a rollback) while the other informational
metacommands do not. Actually, the other metacommands are not entirely
consistent with each other; compare "\dt nonexistent" and "\df
nonexistent".

It would be nice if the case of a matching function not found by \sf
or \ef could be handled in the same fashion as:

# \d nonexistent
Did not find any relation named "nonexistent".

though I realize that's not trivial due to the implementation of
lookup_function_oid(). At any rate, this nitpicking about the error
handling in the case that the function is not found is quibbling about
behavior unchanged by the patch. I don't have any complaints about the
patch itself, so I think it can be applied as-is, and we can attack
the error handling issue separately.

Josh

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

2013/2/20 Josh Kupershmidt <schmiddy@gmail.com>:

On Wed, Jan 23, 2013 at 12:06 PM, Pavel Stehule <pavel.stehule@gmail.com> wrote:

2013/1/14 Tom Lane <tgl@sss.pgh.pa.us>:

Well, fine, but then it should fix both of them and remove
minimal_error_message altogether. I would however suggest eyeballing
what happens when you try "\ef nosuchfunction" (with or without -E).
I'm pretty sure that the reason for the special error handling in these
functions is that the default error reporting was unpleasant for this
use-case.

so I rewrote patch

still is simple

On the end I didn't use PSQLexec - just write hidden queries directly
from related functions - it is less invasive

Sorry for the delay, but I finally had a chance to review this patch.
I think the patch does a good job of bringing the behavior of \sf and
\ef in-line with the other meta-commands as far as --echo-hidden is
concerned.

About the code changes:

The bulk of the code change comes from factoring TraceQuery() out of
PSQLexec(), so that \ef and \sf may make use of this query-printing
without going through PSQLexec(). And not using PSQLexec() lets us
avoid a somewhat uglier error message like:

ERROR: function "nonexistent" does not exist
LINE 1: SELECT 'nonexistent'::pg_catalog.regproc::pg_catalog.oid

Tom suggested removing minimal_error_message() entirely, which would
be nice if possible. It is a shame that "\sf nonexistent" produces a
scary-looking "ERROR: ...." message (and would cause any in-progress
transaction to need a rollback) while the other informational
metacommands do not. Actually, the other metacommands are not entirely
consistent with each other; compare "\dt nonexistent" and "\df
nonexistent".

It would be nice if the case of a matching function not found by \sf
or \ef could be handled in the same fashion as:

# \d nonexistent
Did not find any relation named "nonexistent".

though I realize that's not trivial due to the implementation of
lookup_function_oid(). At any rate, this nitpicking about the error
handling in the case that the function is not found is quibbling about
behavior unchanged by the patch. I don't have any complaints about the
patch itself, so I think it can be applied as-is, and we can attack
the error handling issue separately.

I looked there - and mentioned issue needs little bit different
strategy - fault tolerant function searching based on function
signature. This code can be very simply, although I am not sure if we
would it. We cannot to remove minimal_error_message() because there
are >>two<< SQL queries and if we do fault tolerant oid lookup, then
still pg_get_functiondef can raise exception. We can significantly
reduce showing this error only. It is not hard task, but it needs new
builtin SQL function and then patch can be more times larger than
current patch.

Opinion, notes?

Regards

Pavel

Josh

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

Pavel,

* Pavel Stehule (pavel.stehule@gmail.com) wrote:

We cannot to remove minimal_error_message() because there
are >>two<< SQL queries and if we do fault tolerant oid lookup, then
still pg_get_functiondef can raise exception.

Why is that? lookup_function_oid() only collects the oid to pass to
get_create_function_cmd(), why not just issue one query to the backend?
And use PSQLexec() to boot and get --echo-hidden, etc, for free? And
eliminate the one-off error handling for just this case?

Thanks,

Stephen

2013/2/23 Stephen Frost <sfrost@snowman.net>:

Pavel,

* Pavel Stehule (pavel.stehule@gmail.com) wrote:

We cannot to remove minimal_error_message() because there
are >>two<< SQL queries and if we do fault tolerant oid lookup, then
still pg_get_functiondef can raise exception.

Why is that? lookup_function_oid() only collects the oid to pass to
get_create_function_cmd(), why not just issue one query to the backend?
And use PSQLexec() to boot and get --echo-hidden, etc, for free? And
eliminate the one-off error handling for just this case?

yes, we can do it. There is only one issue

routines for parsing function signature in regproc and regprocedure
should be updated - and I would to get some agreement than I start to
do modify core.

Regards

Pavel

Thanks,

Stephen

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

Pavel Stehule <pavel.stehule@gmail.com> writes:

2013/2/23 Stephen Frost <sfrost@snowman.net>:

Why is that? lookup_function_oid() only collects the oid to pass to
get_create_function_cmd(), why not just issue one query to the backend?
And use PSQLexec() to boot and get --echo-hidden, etc, for free? And
eliminate the one-off error handling for just this case?

yes, we can do it. There is only one issue

routines for parsing function signature in regproc and regprocedure
should be updated - and I would to get some agreement than I start to
do modify core.

Uh ... you seem to be asking for a blank check to modify regprocedure,
which is unlikely to be forthcoming. Why do you think these things need
to be changed, and to what?

regards, tom lane

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

2013/2/23 Tom Lane <tgl@sss.pgh.pa.us>:

Pavel Stehule <pavel.stehule@gmail.com> writes:

2013/2/23 Stephen Frost <sfrost@snowman.net>:

Why is that? lookup_function_oid() only collects the oid to pass to
get_create_function_cmd(), why not just issue one query to the backend?
And use PSQLexec() to boot and get --echo-hidden, etc, for free? And
eliminate the one-off error handling for just this case?

yes, we can do it. There is only one issue

routines for parsing function signature in regproc and regprocedure
should be updated - and I would to get some agreement than I start to
do modify core.

Uh ... you seem to be asking for a blank check to modify regprocedure,
which is unlikely to be forthcoming. Why do you think these things need
to be changed, and to what?

If I understand well to request to remove "minimal_error_message" we
need to have fault tolerant regprocedure.

I am looking on this code now, and it is not easy as I though - there
are two possible errors: not found or found more - so returning
InvalidOid is not enough - and then some "new lookup" function is not
simple or is ugly - and I am not sure, so cost is less than benefit.
in this case.

Regards

Pavel

regards, tom lane

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

Pavel,

* Pavel Stehule (pavel.stehule@gmail.com) wrote:

I am looking on this code now, and it is not easy as I though - there
are two possible errors: not found or found more - so returning
InvalidOid is not enough - and then some "new lookup" function is not
simple or is ugly - and I am not sure, so cost is less than benefit.
in this case.

The problem here is that this code is trying to cheat and use a cast
call to regproc or regprocedure to look for the function. This has
already been solved in \df, why not refactor that code to be used for
this case as well?

Thanks,

Stephen

2013/2/24 Stephen Frost <sfrost@snowman.net>:

Pavel,

* Pavel Stehule (pavel.stehule@gmail.com) wrote:

I am looking on this code now, and it is not easy as I though - there
are two possible errors: not found or found more - so returning
InvalidOid is not enough - and then some "new lookup" function is not
simple or is ugly - and I am not sure, so cost is less than benefit.
in this case.

The problem here is that this code is trying to cheat and use a cast
call to regproc or regprocedure to look for the function. This has
already been solved in \df, why not refactor that code to be used for
this case as well?

it is not possible - both fragments has different purpose. Code in \ef
or \sf should to select exactly one function based on complete
function signature, \df try to show list of functions filtered by
name.

Minimally \ef needs exact specification - you cannot to edit more
functions in same time. So we have to be able identify if there are no
selected function or if there are more functions. We can write a
auxiliary function that returns list of function oids for specified
signature - but it is relative much more code and it is hard to
implement for older versions - but we can use regproc and regprocedure
there. It is not hard problem - just a some about 100 lines - but it
is going out of original proposal, so I am asking if we want this and
more code will be accepted.

Pavel

Thanks,

Stephen

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

* Pavel Stehule (pavel.stehule@gmail.com) wrote:

it is not possible - both fragments has different purpose. Code in \ef
or \sf should to select exactly one function based on complete
function signature, \df try to show list of functions filtered by
name.

I don't buy that argument. You could use the same code and simply
complain if multiple functions are returned from the query. That's the
point- when you actually query the catalog instead of just trying to use
the cast functions, you can do things like detect how many records are
returned and do something sensible.

Minimally \ef needs exact specification - you cannot to edit more
functions in same time. So we have to be able identify if there are no
selected function or if there are more functions. We can write a
auxiliary function that returns list of function oids for specified
signature - but it is relative much more code and it is hard to
implement for older versions - but we can use regproc and regprocedure
there.

Using regproc and regprocedure is the wrong approach here and will be a
pain to maintain as well as a backwards incompatible change to how they
behave. We have solved this problem already and what \df does is exactly
the right answer.

It is not hard problem - just a some about 100 lines - but it
is going out of original proposal, so I am asking if we want this and
more code will be accepted.

I don't see any reason nor need to change regproc or regprocedure.

Thanks,

Stephen

2013/2/24 Stephen Frost <sfrost@snowman.net>:

* Pavel Stehule (pavel.stehule@gmail.com) wrote:

it is not possible - both fragments has different purpose. Code in \ef
or \sf should to select exactly one function based on complete
function signature, \df try to show list of functions filtered by
name.

I don't buy that argument. You could use the same code and simply
complain if multiple functions are returned from the query. That's the
point- when you actually query the catalog instead of just trying to use
the cast functions, you can do things like detect how many records are
returned and do something sensible.

ok, please, send me some SQL that identify some overloaded function -
that will be compatible with current behave and enough robust and not
too much ugly.

Minimally \ef needs exact specification - you cannot to edit more
functions in same time. So we have to be able identify if there are no
selected function or if there are more functions. We can write a
auxiliary function that returns list of function oids for specified
signature - but it is relative much more code and it is hard to
implement for older versions - but we can use regproc and regprocedure
there.

Using regproc and regprocedure is the wrong approach here and will be a
pain to maintain as well as a backwards incompatible change to how they
behave. We have solved this problem already and what \df does is exactly
the right answer.

It is not hard problem - just a some about 100 lines - but it
is going out of original proposal, so I am asking if we want this and
more code will be accepted.

I don't see any reason nor need to change regproc or regprocedure.

no, I talked about new SRF function, that should to help with function
identification. Probably there are not too much shared lines with
regproc8 routines

Regards

Pavel

Thanks,

Stephen

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

Stephen Frost <sfrost@snowman.net> writes:

* Pavel Stehule (pavel.stehule@gmail.com) wrote:

Minimally \ef needs exact specification - you cannot to edit more
functions in same time. So we have to be able identify if there are no
selected function or if there are more functions. We can write a
auxiliary function that returns list of function oids for specified
signature - but it is relative much more code and it is hard to
implement for older versions - but we can use regproc and regprocedure
there.

Using regproc and regprocedure is the wrong approach here and will be a
pain to maintain as well as a backwards incompatible change to how they
behave. We have solved this problem already and what \df does is exactly
the right answer.

Well, actually I think Pavel's got a point. What about overloaded
functions? In \df we don't try to solve that problem, we just print
them all:

regression=# \df abs
List of functions
Schema | Name | Result data type | Argument data types | Type
------------+------+------------------+---------------------+--------
pg_catalog | abs | bigint | bigint | normal
pg_catalog | abs | double precision | double precision | normal
pg_catalog | abs | integer | integer | normal
pg_catalog | abs | numeric | numeric | normal
pg_catalog | abs | real | real | normal
pg_catalog | abs | smallint | smallint | normal
(6 rows)

In \ef you need to select just one of these functions, but \df doesn't
have any ability to do that:

regression=# \df abs(int)
List of functions
Schema | Name | Result data type | Argument data types | Type
--------+------+------------------+---------------------+------
(0 rows)

Now, maybe we *should* teach \df about handling parameter types and
then \ef can piggyback on it, but that code isn't there now.

regards, tom lane

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

* Tom Lane (tgl@sss.pgh.pa.us) wrote:

Well, actually I think Pavel's got a point. What about overloaded
functions? In \df we don't try to solve that problem, we just print
them all:

To be honest, I was reading through that code the other night and could
have sworn that I saw us doing some kind of magic on the arguments under
\df, but of course I don't see it now.

Now, maybe we *should* teach \df about handling parameter types and
then \ef can piggyback on it, but that code isn't there now.

That's definitely the right approach, imv. It should also work if only
a function name is provided and it's not overloaded, of course.

Thanks,

Stephen

On 02/26/2013 02:12 PM, Tom Lane wrote:

Stephen Frost <sfrost@snowman.net> writes:

* Pavel Stehule (pavel.stehule@gmail.com) wrote:

Minimally \ef needs exact specification - you cannot to edit more
functions in same time. So we have to be able identify if there are no
selected function or if there are more functions. We can write a
auxiliary function that returns list of function oids for specified
signature - but it is relative much more code and it is hard to
implement for older versions - but we can use regproc and regprocedure
there.

Using regproc and regprocedure is the wrong approach here and will be a
pain to maintain as well as a backwards incompatible change to how they
behave. We have solved this problem already and what \df does is exactly
the right answer.

Well, actually I think Pavel's got a point. What about overloaded
functions? In \df we don't try to solve that problem, we just print
them all:

regression=# \df abs
List of functions
Schema | Name | Result data type | Argument data types | Type
------------+------+------------------+---------------------+--------
pg_catalog | abs | bigint | bigint | normal
pg_catalog | abs | double precision | double precision | normal
pg_catalog | abs | integer | integer | normal
pg_catalog | abs | numeric | numeric | normal
pg_catalog | abs | real | real | normal
pg_catalog | abs | smallint | smallint | normal
(6 rows)

In \ef you need to select just one of these functions, but \df doesn't
have any ability to do that:

regression=# \df abs(int)
List of functions
Schema | Name | Result data type | Argument data types | Type
--------+------+------------------+---------------------+------
(0 rows)

Now, maybe we *should* teach \df about handling parameter types and
then \ef can piggyback on it, but that code isn't there now.

If we're going to mess with this area can I put in a plea to get \ef and
\sf to handle full parameter specs? I want to be able to c&p from the
\df output to see the function. But here's what happens:

andrew=# \df json_get
List of functions
Schema | Name | Result data type | Argument
data types | Type
------------+----------+------------------+-----------------------------------------------+--------
pg_catalog | json_get | json | from_json json, element
integer | normal
pg_catalog | json_get | json | from_json json,
VARIADIC path_elements text[] | normal
(2 rows)

andrew=# \sf json_get (from_json json, element integer )
ERROR: invalid type name "from_json json"

cheers

andrew

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

* Andrew Dunstan (andrew@dunslane.net) wrote:

If we're going to mess with this area can I put in a plea to get \ef
and \sf to handle full parameter specs? I want to be able to c&p
from the \df output to see the function. But here's what happens:

I was thinking along the same lines. This will involve a bit more code
in psql, but I think that's better than trying to get the backend to do
this work for us, for one thing, we want psql to support multiple major
versions and that could be done by adding code to psql, but couldn't be
done for released versions if we depend on the backend to solve this
matching for us.

Thanks,

Stephen