psql \df choose functions by their arguments

On Thu, Oct 15, 2020 at 01:21:06PM -0400, Greg Sabino Mullane wrote:

Improve psql \df to choose functions by their arguments

I think this is a good idea.

This isn't working for arrays:

postgres=# \df aa
public | aa | integer | integer, integer | func
public | aa | integer | integer, integer, integer | func
public | aa | integer | integer[], integer, integer | func

postgres=# \df aa aa int[]

I think it should use the same syntax as \sf and \ef, which require parenthesis
and commas, not spaces.

int x = 0
while ((functoken = strtokx(x++ ? NULL : funcargs, " \t\n\r", ".,();", "\"", 0, false, true, pset.encoding)))

I think x is just used as "initial", so I think you should make it boolean and
then set is_initial = false, or similar.

+ pg_strcasecmp(functoken, "bool") == 0 ? "'boolean'"

I think writing this all within a call to appendPQExpBuffer() is excessive.
You can make an array or structure to search through and then append the result
to the buffer.

--
Justin

Thank you for looking this over.

This isn't working for arrays:
...
postgres=# \df aa aa int[]

Arrays should work as expected, I think you have one too many "aa" in there?

I think it should use the same syntax as \sf and \ef, which require
parenthesis
and commas, not spaces.

Hmm, that will not allow partial matches if we require a closing parens.
Right now both commas and parens are accepted, but optional.

I think x is just used as "initial", so I think you should make it boolean
and
then set is_initial = false, or similar.

Good suggestion, it is done.

+
pg_strcasecmp(functoken, "bool") == 0 ? "'boolean'"

I think writing this all within a call to appendPQExpBuffer() is excessive.
You can make an array or structure to search through and then append the
result
to the buffer.

Hmm, like a custom struct we loop through? I will look into implementing
that and submit a new patch.

Cheers,
Greg

Thanks for the feedback, attached is version two of the patch. Major
changes:

* Use booleans not generic "int x"
* Build a quick list of abbreviations at the top of the function
* Add array mapping for all types
* Removed the tab-complete bit, it was too fragile and unhelpful

Cheers,
Greg

* Removed the tab-complete bit, it was too fragile and unhelpful

I can’t speak for the specific patch, but tab completion of proc args for \df, \ef and friends has long been a desired feature of mine, particularly when you are dealing with functions with huge numbers of arguments and the same name which I have (sadly) come across many times in the wild.

Removing this because it was brittle is fine, but would be good to see if we could figure out a way to have this kind of feature in psql IMHO.

Best,

David

Hi

(sorry forget to cc the hacklist)

Improve psql \df to choose functions by their arguments

I think this is useful.

I found some comments in the patch.

1.

* Abbreviations of common types is permitted (because who really likes
to type out "character varying"?), so the above could also be written as:

some Abbreviations of common types are not added to the type_abbreviations[] Such as:

Int8 => bigint
Int2 => smallint
Int4 ,int => integer
Float4 => real
Float8,float,double => double precision
(as same as array type)

Single array seems difficult to handle it, may be we can use double array or use a struct.

2.
And I think It's better to update '/?' info about '\df[+]' in function slashUsage(unsigned short int pager).

Best regards,
houzj

Thanks for looking this over!

some Abbreviations of common types are not added to the
type_abbreviations[] Such as:

Int8 => bigint

I wasn't aiming to provide a canonical list, as I personally have never
seen anyone use int8 instead of bigint when (for example) creating a
function, but I'm not strongly opposed to expanding the list.

Single array seems difficult to handle it, may be we can use double array

or use a struct.

I think the single works out okay, as this is a simple write-once variable
that is not likely to get updated often.

And I think It's better to update '/?' info about '\df[+]' in function
slashUsage(unsigned short int pager).

Suggestions welcome, but it's already pretty tight in there, so I couldn't
think of anything:

fprintf(output, _(" \\dew[+] [PATTERN] list foreign-data
wrappers\n"));
fprintf(output, _(" \\df[anptw][S+] [PATRN] list [only
agg/normal/procedures/trigger/window] functions\n"));
fprintf(output, _(" \\dF[+] [PATTERN] list text search
configurations\n"));

The \df option is already our longest one, even with the silly attempt to
shorten PATTERN :)

Cheers,
Greg

On Sun, Nov 1, 2020 at 12:05 PM David Christensen <david@pgguru.net> wrote:

I can’t speak for the specific patch, but tab completion of proc args for
\df, \ef and friends has long been a desired feature of mine, particularly
when you are dealing with functions with huge numbers of arguments and the
same name which I have (sadly) come across many times in the wild.

If someone can get this working against this current patch, that would be
great, but I suspect it will require some macro-jiggering in tab-complete.c
and possibly more, so yeah, could be something to add to the todo list.

Cheers,
Greg

Attached is the latest patch against HEAD - basically fixes a few typos.

Cheers,
Greg

On Thu, Dec 31, 2020 at 7:01 AM Greg Sabino Mullane <htamfids@gmail.com> wrote:

Attached is the latest patch against HEAD - basically fixes a few typos.

Hi Greg,

It looks like there is a collation dependency here that causes the
test to fail on some systems:

=== ./src/test/regress/regression.diffs ===
diff -U3 /tmp/cirrus-ci-build/src/test/regress/expected/psql.out
/tmp/cirrus-ci-build/src/test/regress/results/psql.out
--- /tmp/cirrus-ci-build/src/test/regress/expected/psql.out 2021-01-01
16:05:25.749692000 +0000
+++ /tmp/cirrus-ci-build/src/test/regress/results/psql.out 2021-01-01
16:11:28.525632000 +0000
@@ -5094,8 +5094,8 @@
public | mtest | integer | double precision, double precision, integer | func
public | mtest | integer | integer | func
public | mtest | integer | integer, text | func
- public | mtest | integer | timestamp without time zone, timestamp
with time zone | func
public | mtest | integer | time without time zone, time with time zone | func
+ public | mtest | integer | timestamp without time zone, timestamp
with time zone | func

On Sat, Jan 2, 2021 at 1:56 AM Thomas Munro <thomas.munro@gmail.com> wrote:

...
It looks like there is a collation dependency here that causes the
test to fail on some systems:

Thanks for pointing that out. I tweaked the function definitions to
hopefully sidestep the ordering issue - attached is v4.

Cheers,
Greg

Hi

I tried this patch out last year but was overrolled by Other Stuff before I got
around to providing any feedback, and was reminded of it just now when I was
trying to execute "\df somefunction text int" or similar, which had me
confused until I remembered it's not a feature yet, so it would
certainly be very
welcome to have this.

2020年11月3日(火) 23:27 Greg Sabino Mullane <htamfids@gmail.com>:

Thanks for looking this over!

some Abbreviations of common types are not added to the type_abbreviations[] Such as:

Int8 => bigint

I wasn't aiming to provide a canonical list, as I personally have never seen
anyone use int8 instead of bigint when (for example) creating a function, but
I'm not strongly opposed to expanding the list.

I have vague memories of working with "int8" a bit (possibly related to an
Informix migration), anyway it seems easy enough to add them for completeness
as someone (possibly migrating from another database) might wonder why
it's not working.

Just a small code readability suggestion - in exec_command_d(), it seems
neater to put the funcargs declaration in a block together with the
code with which uses it (see attached diff).

Regards

Ian Barwick

--
EnterpriseDB: https://www.enterprisedb.com

Thanks for the feedback: new version v5 (attached) has int8, plus the
suggested code formatting.

Cheers,
Greg

2021年1月15日(金) 1:46 Greg Sabino Mullane <htamfids@gmail.com>:

Thanks for the feedback: new version v5 (attached) has int8, plus the
suggested code formatting.

Cheers,
Greg

Thanks for the update.

In my preceding mail I meant we should add int2, int4 and int8 for
completeness
(apologies, I was a bit unclear there), as AFAICS that covers all aliases,
even if these
three are less widely used.

FWIW one place where these do get used in substantial numbers is in the
regression tests themselves:

$ for L in 2 4 8; do git grep int$L src/test/regress/ | wc -l; done
544
2332
1353

Regards

Ian Barwick

--
EnterpriseDB: https://www.enterprisedb.com

Ha ha ha, my bad, I am not sure why I left those out. Here is a new patch
with int2, int4, and int8. Thanks for the email.

Cheers,
Greg

On 1/19/21 11:58 AM, Greg Sabino Mullane wrote:

Ha ha ha, my bad, I am not sure why I left those out. Here is a new
patch with int2, int4, and int8. Thanks for the email.

Ian, does the new patch look good to you?

Also, not sure why the target version for this patch is stable so I have
updated it to PG14.

Regards,
--
-David
david@pgmasters.net

Greg Sabino Mullane <htamfids@gmail.com> writes:

[ v6-psql-df-pick-function-by-type.patch ]

I looked this over. I like the idea a lot, but not much of anything
about the implementation. I think the additional arguments should be
matched to the function types using the same rules as for \dT. That
allows patterns for the argument type names, which is particularly
useful if you want to do something like
\df foo * integer
to find functions whose second argument is integer, without restricting
the first argument.

As a lesser quibble, splitting the arguments with strtokx is a hack;
we should let the normal psql scanner collect the arguments.

So that leads me to the attached, which I think is committable. Since
we're down to the last day of the CF, I'm going to push this shortly if
there aren't squawks soon.

regards, tom lane

I like the wildcard aspect, but I have a few issues with the patch:

* It doesn't respect some common abbreviations that work elsewhere (e.g.
CREATE FUNCTION). So while "int4" works, "int" does not. Nor does "float",
which thus requires the mandatory-double-quoted "double precision"

* Adding commas to the args, as returned by psql itself via \df, provides
no matches.

* There seems to be no way (?) to limit the functions returned if they
share a common root. The previous incantation allowed you to pull out
foo(int) from foo(int, bigint). This was a big motivation for writing this
patch.

* SQL error on \df foo a..b as well as one on \df foo (bigint bigint)

Cheers,
Greg

Greg Sabino Mullane <htamfids@gmail.com> writes:

I like the wildcard aspect, but I have a few issues with the patch:

* It doesn't respect some common abbreviations that work elsewhere (e.g.
CREATE FUNCTION). So while "int4" works, "int" does not. Nor does "float",
which thus requires the mandatory-double-quoted "double precision"

"\dT int" doesn't match anything either. Maybe there's room to improve
on that, but I don't think this patch should deviate from what \dT does.

* Adding commas to the args, as returned by psql itself via \df, provides
no matches.

The docs are fairly clear that the args are to be space-separated, not
comma-separated. This fits with psql's general treatment of backslash
arguments, and I think trying to "improve" on it will just end badly.

* There seems to be no way (?) to limit the functions returned if they
share a common root. The previous incantation allowed you to pull out
foo(int) from foo(int, bigint). This was a big motivation for writing this
patch.

Hmm, are you trying to say that a invocation with N arg patterns should
match only functions with exactly N arguments? We could do that, but
I'm not convinced it's an improvement over what I did here. Default
arguments are a counterexample.

* SQL error on \df foo a..b as well as one on \df foo (bigint bigint)

The first one seems to be a bug, will look. As for the second, I still
don't agree that that should be within the mandated syntax.

regards, tom lane

I wrote:

Greg Sabino Mullane <htamfids@gmail.com> writes:

* SQL error on \df foo a..b as well as one on \df foo (bigint bigint)

The first one seems to be a bug, will look.

Argh, silly typo (and I'd failed to test the schema-qualified-name case).

While I was thinking about use-cases for this, I realized that at least
for me, being able to restrict \do operator searches by input type would
be even more useful than is true for \df. Operator names tend to be
overloaded even more heavily than functions. So here's a v8 that
also fixes \do in the same spirit.

(With respect to the other point: for \do it does seem to make sense
to constrain the match to operators with exactly as many arguments
as specified. I still say that's a bad idea for functions, though.)

regards, tom lane