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

Hello

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

bash-4.1$ ./psql postgres -E
psql (9.3devel)
Type "help" for help.

postgres=# \sf+ foo
********* QUERY **********
SELECT pg_catalog.pg_get_functiondef(16385)
**************************

CREATE OR REPLACE FUNCTION public.foo(a integer)
RETURNS integer
LANGUAGE plpgsql
1 AS $function$
2 begin
3 return 10/a;
4 end;
5 $function$

Regards

Pavel Stehule

Pavel Stehule <pavel(dot)stehule(at)gmail(dot)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

2013/1/14 Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>:
> Pavel Stehule <pavel(dot)stehule(at)gmail(dot)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

On Mon, Jan 14, 2013 at 11:56 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)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

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> On Mon, Jan 14, 2013 at 11:56 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)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

2013/1/14 Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>:
> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>> On Mon, Jan 14, 2013 at 11:56 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)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

>
> regards, tom lane

On Wed, Jan 23, 2013 at 12:06 PM, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com> wrote:
> 2013/1/14 Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)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

2013/2/20 Josh Kupershmidt <schmiddy(at)gmail(dot)com>:
> On Wed, Jan 23, 2013 at 12:06 PM, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com> wrote:
>> 2013/1/14 Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)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

Pavel,

* Pavel Stehule (pavel(dot)stehule(at)gmail(dot)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(at)snowman(dot)net>:
> Pavel,
>
> * Pavel Stehule (pavel(dot)stehule(at)gmail(dot)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

Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com> writes:
> 2013/2/23 Stephen Frost <sfrost(at)snowman(dot)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

2013/2/23 Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>:
> Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com> writes:
>> 2013/2/23 Stephen Frost <sfrost(at)snowman(dot)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

Pavel,

* Pavel Stehule (pavel(dot)stehule(at)gmail(dot)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(at)snowman(dot)net>:
> Pavel,
>
> * Pavel Stehule (pavel(dot)stehule(at)gmail(dot)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

* Pavel Stehule (pavel(dot)stehule(at)gmail(dot)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(at)snowman(dot)net>:
> * Pavel Stehule (pavel(dot)stehule(at)gmail(dot)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

Stephen Frost <sfrost(at)snowman(dot)net> writes:
> * Pavel Stehule (pavel(dot)stehule(at)gmail(dot)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

* Tom Lane (tgl(at)sss(dot)pgh(dot)pa(dot)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(at)snowman(dot)net> writes:
>> * Pavel Stehule (pavel(dot)stehule(at)gmail(dot)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

* Andrew Dunstan (andrew(at)dunslane(dot)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

Stephen Frost <sfrost(at)snowman(dot)net> writes:
> * Andrew Dunstan (andrew(at)dunslane(dot)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.

Dunno, I think that's going to result in a very large chunk of mostly
duplicative code in psql. regprocedurein() is fairly short because it
can rely on a ton of code from the parser, but psql won't have that
luxury.

regards, tom lane

* Tom Lane (tgl(at)sss(dot)pgh(dot)pa(dot)us) wrote:
> Dunno, I think that's going to result in a very large chunk of mostly
> duplicative code in psql. regprocedurein() is fairly short because it
> can rely on a ton of code from the parser, but psql won't have that
> luxury.

Parsing/tokenizing a CSV string inside parens doesn't strike me as all
that difficult, even when handling the space-delimininated varname from
the type.

The hard part would, I think, be the normalization of the
type names into what \df returns, but do we even want to try and tackle
that..?. How much do we care about supporting every textual
representation of the 'integer' type? That's not going to be an issue
for people doing tab-completion or using \df's output. We could also
have it fall-back to trying w/o any arguments for a unique function name
match if the initial attempt w/ the function arguments included doesn't
return any results.

Thanks,

Stephen

2013/2/26 Stephen Frost <sfrost(at)snowman(dot)net>:
> * Tom Lane (tgl(at)sss(dot)pgh(dot)pa(dot)us) wrote:
>> Dunno, I think that's going to result in a very large chunk of mostly
>> duplicative code in psql. regprocedurein() is fairly short because it
>> can rely on a ton of code from the parser, but psql won't have that
>> luxury.
>
> Parsing/tokenizing a CSV string inside parens doesn't strike me as all
> that difficult, even when handling the space-delimininated varname from
> the type.

this is not hard task, hard task is correct identification related function

see FuncnameGetCandidates() function

I am sure, so we don't would to duplicate this function on client side.

probably we can simplify searching to search only exact same signature
- but still there are problem with type synonyms. And solving this
code on client side means code duplication.

I prefer some smart searching function on server side - it can be
useful for other app too - pgAdmin, plpgsql debugger, ... And in psql
we can do switch - for older versions use current code, and for newer
"smart" server side function.

???

Regards

Pavel

>
> The hard part would, I think, be the normalization of the
> type names into what \df returns, but do we even want to try and tackle
> that..?. How much do we care about supporting every textual
> representation of the 'integer' type? That's not going to be an issue
> for people doing tab-completion or using \df's output. We could also
> have it fall-back to trying w/o any arguments for a unique function name
> match if the initial attempt w/ the function arguments included doesn't
> return any results.
>
> Thanks,
>
> Stephen

Pavel,

* Pavel Stehule (pavel(dot)stehule(at)gmail(dot)com) wrote:
> this is not hard task, hard task is correct identification related function
>
> see FuncnameGetCandidates() function

We're not limited to writing C code here though and I think we've
already solved it, though I admit it wasn't where I originally thought.

> I am sure, so we don't would to duplicate this function on client side.

It took me a bit to go figure out where it is, but I knew we had it.
Look at COMPLETE_WITH_FUNCTION_ARG and Query_for_list_of_arguments in
src/bin/psql/tab-complete.c. We can already fully tab-complete a
function and its arguments, down to knowing that the function+args is
unique. I have no idea why \df and friends aren't already supporting
this (is there a real reason or just unintentional omission? would love
to know..), but it works just fine for DROP FUNCTION and ALTER FUNCTION.
Would be really nice to have that work for \df, \ef, \sf, CREATE OR
REPLACE FUNCTION, and anywhere else that makes sense.

With support for what tab-complete returns (arg types w/o arg names) and
\df's function list result set (arg names + arg types), I think we can
happily close this out. I don't think we need to stress about people
complaining that \ef myfunc(int) doesn't find a matching function when
they can do \ef myfunc(int<tab> and have it tab-complete the rest.

Thanks,

Stephen

2013/2/27 Stephen Frost <sfrost(at)snowman(dot)net>:
> Pavel,
>
> * Pavel Stehule (pavel(dot)stehule(at)gmail(dot)com) wrote:
>> this is not hard task, hard task is correct identification related function
>>
>> see FuncnameGetCandidates() function
>
> We're not limited to writing C code here though and I think we've
> already solved it, though I admit it wasn't where I originally thought.
>
>> I am sure, so we don't would to duplicate this function on client side.
>
> It took me a bit to go figure out where it is, but I knew we had it.
> Look at COMPLETE_WITH_FUNCTION_ARG and Query_for_list_of_arguments in
> src/bin/psql/tab-complete.c. We can already fully tab-complete a
> function and its arguments, down to knowing that the function+args is
> unique. I have no idea why \df and friends aren't already supporting
> this (is there a real reason or just unintentional omission? would love
> to know..), but it works just fine for DROP FUNCTION and ALTER FUNCTION.
> Would be really nice to have that work for \df, \ef, \sf, CREATE OR
> REPLACE FUNCTION, and anywhere else that makes sense.
>
> With support for what tab-complete returns (arg types w/o arg names) and
> \df's function list result set (arg names + arg types), I think we can
> happily close this out. I don't think we need to stress about people
> complaining that \ef myfunc(int) doesn't find a matching function when
> they can do \ef myfunc(int<tab> and have it tab-complete the rest.
>

this autocomplete routine doesn't know type synonyms

so you cannot use int, varchar, ... :(

Pavel

> Thanks,
>
> Stephen

Pavel,

* Pavel Stehule (pavel(dot)stehule(at)gmail(dot)com) wrote:
> this autocomplete routine doesn't know type synonyms
>
> so you cannot use int, varchar, ... :(

Yes, I covered that and it's perfectly fine, imv. Results from
tab-completion and from \df output should work just fine.

'\df myfunc(int)' doesn't work currently either. In fact,
'\df myfunc(integer)' doesn't work.

Thanks,

Stephen

2013/2/27 Stephen Frost <sfrost(at)snowman(dot)net>:
> Pavel,
>
> * Pavel Stehule (pavel(dot)stehule(at)gmail(dot)com) wrote:
>> this autocomplete routine doesn't know type synonyms
>>
>> so you cannot use int, varchar, ... :(
>
> Yes, I covered that and it's perfectly fine, imv. Results from
> tab-completion and from \df output should work just fine.
>
> '\df myfunc(int)' doesn't work currently either. In fact,
> '\df myfunc(integer)' doesn't work.

I though about it more, and this is bad example

we cannot use autocomplete or if we use, then more precious code is on
server side still - everywhere where function autocomplete is used,
then function signature is reparesed again on server side.

So this is not a win

Regards

Pavel

>
> Thanks,
>
> Stephen

* Pavel Stehule (pavel(dot)stehule(at)gmail(dot)com) wrote:
> we cannot use autocomplete or if we use, then more precious code is on
> server side still - everywhere where function autocomplete is used,
> then function signature is reparesed again on server side.

This doesn't make any sense to me.

We should rip out the auto-complete that we already have for functions
because the tab-complete queries the database and then the whole string
is sent to the server and parsed by it?

Thanks,

Stephen

2013/2/27 Stephen Frost <sfrost(at)snowman(dot)net>:
> * Pavel Stehule (pavel(dot)stehule(at)gmail(dot)com) wrote:
>> we cannot use autocomplete or if we use, then more precious code is on
>> server side still - everywhere where function autocomplete is used,
>> then function signature is reparesed again on server side.
>
> This doesn't make any sense to me.
>
> We should rip out the auto-complete that we already have for functions
> because the tab-complete queries the database and then the whole string
> is sent to the server and parsed by it?

autocomplete send a SQL query in every iteration to server - so it is
not any new overhead. And if we should to write some smarted routine,
then I prefer server side due better reusability and better exception
processing than psql environment - and a possibility of access to
system caches and auxiliary functions for arguments processing. This
routine can be smart enough to support autocomplete and unique
identification without needless exceptions - and then psql behave can
be more smooth and consistent.

Regards

Pavel

>
> Thanks,
>
> Stephen

* Pavel Stehule (pavel(dot)stehule(at)gmail(dot)com) wrote:
> autocomplete send a SQL query in every iteration to server - so it is
> not any new overhead. And if we should to write some smarted routine,
> then I prefer server side due better reusability and better exception
> processing than psql environment - and a possibility of access to
> system caches and auxiliary functions for arguments processing. This
> routine can be smart enough to support autocomplete and unique
> identification without needless exceptions - and then psql behave can
> be more smooth and consistent.

We already have it and it works quite well from my POV. I don't think
we need to over-engineer this.

Thanks,

Stephen

2013/2/27 Stephen Frost <sfrost(at)snowman(dot)net>:
> * Pavel Stehule (pavel(dot)stehule(at)gmail(dot)com) wrote:
>> autocomplete send a SQL query in every iteration to server - so it is
>> not any new overhead. And if we should to write some smarted routine,
>> then I prefer server side due better reusability and better exception
>> processing than psql environment - and a possibility of access to
>> system caches and auxiliary functions for arguments processing. This
>> routine can be smart enough to support autocomplete and unique
>> identification without needless exceptions - and then psql behave can
>> be more smooth and consistent.
>
> We already have it and it works quite well from my POV. I don't think
> we need to over-engineer this.

I don't agree so it works well - you cannot use short type names is
significant issue

Pavel

>
> Thanks,
>
> Stephen

* Pavel Stehule (pavel(dot)stehule(at)gmail(dot)com) wrote:
> I don't agree so it works well - you cannot use short type names is
> significant issue

This is for psql. In what use-case do you see that being a serious
limitation?

I might support having psql be able to fall-back to checking if the
function name is unique (or perhaps doing that first before going on to
look at the function arguments) but I don't think this should all be
punted to the backend where only 9.3+ would have any real support for a
capability which already exists in other places and should be trivially
added to these.

Thanks,

Stephen

2013/2/27 Stephen Frost <sfrost(at)snowman(dot)net>:
> * Pavel Stehule (pavel(dot)stehule(at)gmail(dot)com) wrote:
>> I don't agree so it works well - you cannot use short type names is
>> significant issue
>
> This is for psql. In what use-case do you see that being a serious
> limitation?
>
> I might support having psql be able to fall-back to checking if the
> function name is unique (or perhaps doing that first before going on to
> look at the function arguments) but I don't think this should all be
> punted to the backend where only 9.3+ would have any real support for a
> capability which already exists in other places and should be trivially
> added to these.

a functionality can be same - only error message will be little bit different

>
> Thanks,
>
> Stephen

On Wed, Feb 27, 2013 at 12:09 PM, Stephen Frost <sfrost(at)snowman(dot)net> wrote:
> * Pavel Stehule (pavel(dot)stehule(at)gmail(dot)com) wrote:
>> I don't agree so it works well - you cannot use short type names is
>> significant issue
>
> This is for psql. In what use-case do you see that being a serious
> limitation?
>
> I might support having psql be able to fall-back to checking if the
> function name is unique (or perhaps doing that first before going on to
> look at the function arguments) but I don't think this should all be
> punted to the backend where only 9.3+ would have any real support for a
> capability which already exists in other places and should be trivially
> added to these.

Since time is running short for discussion of 9.3:

I still think this patch is an improvement over the status quo, and is
committable as-is. Yes, the patch doesn't address the existing
ugliness with minimal_error_message() and sidestepping PSQLexec(), but
at least it fixes the --echo-hidden behavior, and the various other
issues may be handled separately.

Josh

Josh,

* Josh Kupershmidt (schmiddy(at)gmail(dot)com) wrote:
> I still think this patch is an improvement over the status quo, and is
> committable as-is. Yes, the patch doesn't address the existing
> ugliness with minimal_error_message() and sidestepping PSQLexec(), but
> at least it fixes the --echo-hidden behavior, and the various other
> issues may be handled separately.

Which patch, exactly, are you referring to wrt this..? I'm less than
convinced that it's in a committable state, but I'd like to make sure
that we're talking about the same thing here...

Thanks,

Stephen

On Mon, Mar 4, 2013 at 11:39 AM, Stephen Frost <sfrost(at)snowman(dot)net> wrote:
> Josh,
>
> * Josh Kupershmidt (schmiddy(at)gmail(dot)com) wrote:
>> I still think this patch is an improvement over the status quo, and is
>> committable as-is. Yes, the patch doesn't address the existing
>> ugliness with minimal_error_message() and sidestepping PSQLexec(), but
>> at least it fixes the --echo-hidden behavior, and the various other
>> issues may be handled separately.
>
> Which patch, exactly, are you referring to wrt this..? I'm less than
> convinced that it's in a committable state, but I'd like to make sure
> that we're talking about the same thing here...

Sorry, this second version posted by Pavel:
http://www.postgresql.org/message-id/CAFj8pRB3-Tov5s2dCGshp+Vedyk9s97d7hn7rDMmw9Ztrj-7tg@mail.gmail.com

Josh

* Josh Kupershmidt (schmiddy(at)gmail(dot)com) wrote:
> Sorry, this second version posted by Pavel:
> http://www.postgresql.org/message-id/CAFj8pRB3-Tov5s2dCGshp+Vedyk9s97d7hn7rDMmw9Ztrj-7tg@mail.gmail.com

Yeah, no, I don't think we should go in this direction. The whole
TraceQuery thing is entirely redundant to what's already there and which
should have been used from the beginning. This would be adding on to
that mistake instead of fixing it.

If we're going to fix it, let's fix it correctly.

Thanks,

Stephen

On Mon, Mar 4, 2013 at 11:54 AM, Stephen Frost <sfrost(at)snowman(dot)net> wrote:
> Yeah, no, I don't think we should go in this direction. The whole
> TraceQuery thing is entirely redundant to what's already there and which
> should have been used from the beginning. This would be adding on to
> that mistake instead of fixing it.
>
> If we're going to fix it, let's fix it correctly.

Fair enough. I thought the little extra ugliness was stomachable, but
I'm willing to call this patch Returned with Feedback for now.

Josh