-----BEGIN PGP SIGNED MESSAGE-----
Hash: RIPEMD160 Improve psql \df to choose functions by their arguments == OVERVIEW Having to scroll through same-named functions with different argument types when you know exactly which one you want is annoying at best, error causing at worst. This patch enables a quick narrowing of functions with the same name but different arguments. For example, to see the full details of a function names "myfunc" with a TEXT argument, but not showing the version of "myfunc" with a BIGINT argument, one can now do: psql=# \df myfunc text For this, we are fairly liberal in what we accept, and try to be as intuitive as possible. Features: * Type names are case insensitive. Whitespace is optional, but quoting is respected: greg=# \df myfunc text "character varying" INTEGER * Abbreviations of common types is permitted (because who really likes to type out "character varying"?), so the above could also be written as: greg=# \df myfunc text varchar int * The matching is greedy, so you can see everything matching a subset: greg=# \df myfunc timestamptz List of functions Schema | Name | Result data type | Argument data types | Type - --------+--------+------------------+-------------------------------------------+------ public | myfunc | void | timestamp with time zone | func public | myfunc | void | timestamp with time zone, bigint | func public | myfunc | void | timestamp with time zone, bigint, boolean | func public | myfunc | void | timestamp with time zone, integer | func public | myfunc | void | timestamp with time zone, text, cidr | func (5 rows) * The appearance of a closing paren indicates we do not want the greediness: greg=# \df myfunc (timestamptz, bigint) List of functions Schema | Name | Result data type | Argument data types | Type - --------+--------+------------------+----------------------------------+------ public | myfunc | void | timestamp with time zone, bigint | func (1 row) == TAB COMPLETION: I'm not entirely happy with this, but I figure piggybacking onto COMPLETE_WITH_FUNCTION_ARG is better than nothing at all. Ideally we'd walk prev*_wd to refine the returned list, but that's an awful lot of complexity for very little gain, and I think the current behavior of showing the complete list of args each time should suffice. == DOCUMENTATION: The new feature is briefly mentioned: wordsmithing help in the sgml section is appreciated. I'm not sure how many of the above features need to be documented in detail. Regarding psql/help.c, I don't think this really warrants a change there. As it is, we've gone through great lengths to keep this overloaded backslash command left justified with the rest! == TESTS: I put this into psql.c, seems the best place. Mostly testing out basic functionality, quoting, and the various abbreviations. Not much else to test, near as I can tell, as this is a pure convienence addition and shouldn't affect anything else. Any extra words after a function name for \df was previously treated as an error. == IMPLEMENTATION: Rather than messing with psqlscanslash, we simply slurp in the entire rest of the line via psql_scan_slash_option (all of which was previously ignored). This is passed to describeFunction, which then uses strtokx to break it into tokens. We look for a match by comparing the current proargtypes entry, casted to text, against the lowercase version of the token found by strtokx. Along the way, we convert things like "timestamptz" to the official version (i.e. "timestamp with time zone"). If any of the tokens start with a closing paren, we immediately stop parsing and set pronargs to the current number of valid tokens, thereby forcing a match to one (or zero) functions. 6ab7a45d541f2c31c5631b811f14081bf7b22271 v1-psql-df-pick-function-by-type.patch - -- Greg Sabino Mullane PGP Key: 0x14964AC8 202010151316 http://biglumber.com/x/web?pk=2529DF6AB8F79407E94445B4BC9B906714964AC8 -----BEGIN PGP SIGNATURE----- iF0EAREDAB0WIQQlKd9quPeUB+lERbS8m5BnFJZKyAUCX4iENQAKCRC8m5BnFJZK yIUKAKDiv1E9KgXuSO7lE9p+ttFdk02O2ACg44lu9VdKt3IggIrPiXBPKR8C85M= =QPSd -----END PGP SIGNATURE----- |
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: 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 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 Good suggestion, it is done. + pg_strcasecmp(functoken, "bool") == 0 ? "'boolean'" Hmm, like a custom struct we loop through? I will look into implementing that and submit a new patch. Cheers, Greg |
In reply to this post by Justin Pryzby
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 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: 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 |
In reply to this post by David Christensen-2
On Sun, Nov 1, 2020 at 12:05 PM David Christensen <[hidden email]> wrote:
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 |
In reply to this post by Greg Sabino Mullane-3
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 <[hidden email]> 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 <[hidden email]> wrote: ... Thanks for pointing that out. I tweaked the function definitions to hopefully sidestep the ordering issue - attached is v4. Cheers, Greg |
In reply to this post by Greg Sabino Mullane-3
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 <[hidden email]>: > > 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. 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 |
Free forum by Nabble | Edit this page |