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
Attachments:
sf_hidden_query.patchapplication/octet-stream; name=sf_hidden_query.patchDownload
*** a/src/bin/psql/command.c
--- b/src/bin/psql/command.c
***************
*** 2554,2579 **** lookup_function_oid(PGconn *conn, const char *desc, Oid *foid)
static bool
get_create_function_cmd(PGconn *conn, Oid oid, PQExpBuffer buf)
{
! bool result = true;
PQExpBuffer query;
PGresult *res;
query = createPQExpBuffer();
printfPQExpBuffer(query, "SELECT pg_catalog.pg_get_functiondef(%u)", oid);
! res = PQexec(conn, query->data);
! if (PQresultStatus(res) == PGRES_TUPLES_OK && PQntuples(res) == 1)
! {
! resetPQExpBuffer(buf);
! appendPQExpBufferStr(buf, PQgetvalue(res, 0, 0));
! }
! else
{
! minimal_error_message(res);
! result = false;
}
- PQclear(res);
destroyPQExpBuffer(query);
return result;
--- 2554,2579 ----
static bool
get_create_function_cmd(PGconn *conn, Oid oid, PQExpBuffer buf)
{
! bool result = false;
PQExpBuffer query;
PGresult *res;
query = createPQExpBuffer();
printfPQExpBuffer(query, "SELECT pg_catalog.pg_get_functiondef(%u)", oid);
! res = PSQLexec(query->data, false);
! if (res)
{
! if (PQresultStatus(res) == PGRES_TUPLES_OK && PQntuples(res) == 1)
! {
! resetPQExpBuffer(buf);
! appendPQExpBufferStr(buf, PQgetvalue(res, 0, 0));
! result = true;
! }
!
! PQclear(res);
}
destroyPQExpBuffer(query);
return result;
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
Show quoted text
regards, tom lane
Attachments:
psql_sf_hidden_queries.patchapplication/octet-stream; name=psql_sf_hidden_queries.patchDownload
*** a/src/bin/psql/command.c
--- b/src/bin/psql/command.c
***************
*** 2537,2552 **** lookup_function_oid(PGconn *conn, const char *desc, Oid *foid)
appendPQExpBuffer(query, "::pg_catalog.%s::pg_catalog.oid",
strchr(desc, '(') ? "regprocedure" : "regproc");
! res = PQexec(conn, query->data);
! if (PQresultStatus(res) == PGRES_TUPLES_OK && PQntuples(res) == 1)
! *foid = atooid(PQgetvalue(res, 0, 0));
! else
{
! minimal_error_message(res);
! result = false;
}
- PQclear(res);
destroyPQExpBuffer(query);
return result;
--- 2537,2558 ----
appendPQExpBuffer(query, "::pg_catalog.%s::pg_catalog.oid",
strchr(desc, '(') ? "regprocedure" : "regproc");
! if (TraceQuery(query->data))
{
! res = PQexec(conn, query->data);
! if (PQresultStatus(res) == PGRES_TUPLES_OK && PQntuples(res) == 1)
! *foid = atooid(PQgetvalue(res, 0, 0));
! else
! {
! minimal_error_message(res);
! result = false;
! }
!
! PQclear(res);
}
+ else
+ result = false;
destroyPQExpBuffer(query);
return result;
***************
*** 2566,2584 **** get_create_function_cmd(PGconn *conn, Oid oid, PQExpBuffer buf)
query = createPQExpBuffer();
printfPQExpBuffer(query, "SELECT pg_catalog.pg_get_functiondef(%u)", oid);
! res = PQexec(conn, query->data);
! if (PQresultStatus(res) == PGRES_TUPLES_OK && PQntuples(res) == 1)
{
! resetPQExpBuffer(buf);
! appendPQExpBufferStr(buf, PQgetvalue(res, 0, 0));
}
else
- {
- minimal_error_message(res);
result = false;
- }
- PQclear(res);
destroyPQExpBuffer(query);
return result;
--- 2572,2596 ----
query = createPQExpBuffer();
printfPQExpBuffer(query, "SELECT pg_catalog.pg_get_functiondef(%u)", oid);
! if (TraceQuery(query->data))
{
! res = PQexec(conn, query->data);
! if (PQresultStatus(res) == PGRES_TUPLES_OK && PQntuples(res) == 1)
! {
! resetPQExpBuffer(buf);
! appendPQExpBufferStr(buf, PQgetvalue(res, 0, 0));
! }
! else
! {
! minimal_error_message(res);
! result = false;
! }
!
! PQclear(res);
}
else
result = false;
destroyPQExpBuffer(query);
return result;
*** a/src/bin/psql/common.c
--- b/src/bin/psql/common.c
***************
*** 471,476 **** AcceptResult(const PGresult *result)
--- 471,507 ----
}
+ /*
+ * TraceQuery
+ *
+ * Show query or push query to log. Returns false, when query should
+ * not be executed.
+ */
+ bool
+ TraceQuery(const char *query)
+ {
+ if (pset.echo_hidden != PSQL_ECHO_HIDDEN_OFF)
+ {
+ printf(_("********* QUERY **********\n"
+ "%s\n"
+ "**************************\n\n"), query);
+ fflush(stdout);
+ if (pset.logfile)
+ {
+ fprintf(pset.logfile,
+ _("********* QUERY **********\n"
+ "%s\n"
+ "**************************\n\n"), query);
+ fflush(pset.logfile);
+ }
+
+ if (pset.echo_hidden == PSQL_ECHO_HIDDEN_NOEXEC)
+ return false;
+ }
+
+ return true;
+ }
+
/*
* PSQLexec
***************
*** 499,522 **** PSQLexec(const char *query, bool start_xact)
return NULL;
}
! if (pset.echo_hidden != PSQL_ECHO_HIDDEN_OFF)
! {
! printf(_("********* QUERY **********\n"
! "%s\n"
! "**************************\n\n"), query);
! fflush(stdout);
! if (pset.logfile)
! {
! fprintf(pset.logfile,
! _("********* QUERY **********\n"
! "%s\n"
! "**************************\n\n"), query);
! fflush(pset.logfile);
! }
!
! if (pset.echo_hidden == PSQL_ECHO_HIDDEN_NOEXEC)
! return NULL;
! }
SetCancelConn();
--- 530,537 ----
return NULL;
}
! if (!TraceQuery(query))
! return NULL;
SetCancelConn();
*** a/src/bin/psql/common.h
--- b/src/bin/psql/common.h
***************
*** 46,51 **** extern void SetCancelConn(void);
--- 46,52 ----
extern void ResetCancelConn(void);
extern PGresult *PSQLexec(const char *query, bool start_xact);
+ extern bool TraceQuery(const char *query);
extern bool SendQuery(const char *query);
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 invasiveSorry 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.oidTom 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
Stephen Frost <sfrost@snowman.net> writes:
* 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.
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
--
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:
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@snowman.net>:
* Tom Lane (tgl@sss.pgh.pa.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
--
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:
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@snowman.net>:
Pavel,
* Pavel Stehule (pavel.stehule@gmail.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
--
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:
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@snowman.net>:
Pavel,
* Pavel Stehule (pavel.stehule@gmail.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
--
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:
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@snowman.net>:
* Pavel Stehule (pavel.stehule@gmail.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
--
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:
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@snowman.net>:
* Pavel Stehule (pavel.stehule@gmail.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
--
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:
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@snowman.net>:
* Pavel Stehule (pavel.stehule@gmail.com) wrote:
I don't agree so it works well - you cannot use short type names is
significant issueThis 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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Wed, Feb 27, 2013 at 12:09 PM, Stephen Frost <sfrost@snowman.net> wrote:
* Pavel Stehule (pavel.stehule@gmail.com) wrote:
I don't agree so it works well - you cannot use short type names is
significant issueThis 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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Josh,
* Josh Kupershmidt (schmiddy@gmail.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@snowman.net> wrote:
Josh,
* Josh Kupershmidt (schmiddy@gmail.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:
/messages/by-id/CAFj8pRB3-Tov5s2dCGshp+Vedyk9s97d7hn7rDMmw9Ztrj-7tg@mail.gmail.com
Josh
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
* Josh Kupershmidt (schmiddy@gmail.com) wrote:
Sorry, this second version posted by Pavel:
/messages/by-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@snowman.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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers