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

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