Thread: [PATCH] Tab completion for ALTER DATABASE … SETTABLESPACE
[PATCH] Tab completion for ALTER DATABASE … SETTABLESPACE
From
ilmari@ilmari.org (Dagfinn Ilmari Mannsåker)
Date:
Hi hackers, I just noticed that psql's tab completion for ALTER TABLE … SET TABLESPACE was treating it as any other configuration parameter and completing with FROM DEFAULT or TO after it, instead of a list of tablespaces. PFA a patch that fixes this. - ilmari -- "A disappointingly low fraction of the human race is, at any given time, on fire." - Stig Sandbeck Mathisen From a72d9dc1f38606a0bca8ff29b561915b8cbe9d5c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Dagfinn=20Ilmari=20Manns=C3=A5ker?= <ilmari@ilmari.org> Date: Thu, 30 Aug 2018 17:36:16 +0100 Subject: [PATCH] =?UTF-8?q?Fix=20tab=20completion=20for=20ALTER=20TABLE=20?= =?UTF-8?q?=E2=80=A6=20SET=20TABLESPACE?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit It was being treated as any other configuration parameter. --- src/bin/psql/tab-complete.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/bin/psql/tab-complete.c b/src/bin/psql/tab-complete.c index bb696f8ee9..f107ffb027 100644 --- a/src/bin/psql/tab-complete.c +++ b/src/bin/psql/tab-complete.c @@ -1783,6 +1783,10 @@ psql_completion(const char *text, int start, int end) "IS_TEMPLATE", "ALLOW_CONNECTIONS", "CONNECTION LIMIT"); + /* ALTER DATABASE <name> SET TABLESPACE */ + else if (Matches5("ALTER", "DATABASE", MatchAny, "SET", "TABLESPACE")) + COMPLETE_WITH_QUERY(Query_for_list_of_tablespaces); + /* ALTER EVENT TRIGGER */ else if (Matches3("ALTER", "EVENT", "TRIGGER")) COMPLETE_WITH_QUERY(Query_for_list_of_event_triggers); -- 2.18.0
Re: [PATCH] Tab completion for ALTER DATABASE …SET TABLESPACE
From
ilmari@ilmari.org (Dagfinn Ilmari Mannsåker)
Date:
ilmari@ilmari.org (Dagfinn Ilmari Mannsåker) writes: > Hi hackers, > > I just noticed that psql's tab completion for ALTER TABLE … SET > TABLESPACE was treating it as any other configuration parameter and > completing with FROM DEFAULT or TO after it, instead of a list of > tablespaces. And just after hitting send, I noticed I'd typed ALTER TABLE instead of ALTER DATABASE, including in the commit message :( Fixed patch attached. - ilmari -- "I use RMS as a guide in the same way that a boat captain would use a lighthouse. It's good to know where it is, but you generally don't want to find yourself in the same spot." - Tollef Fog Heen From e5ff67c3284dd456fe80ed8a8927e12665d68fea Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Dagfinn=20Ilmari=20Manns=C3=A5ker?= <ilmari@ilmari.org> Date: Thu, 30 Aug 2018 17:36:16 +0100 Subject: [PATCH] =?UTF-8?q?Fix=20tab=20completion=20for=20ALTER=20DATABASE?= =?UTF-8?q?=20=E2=80=A6=20SET=20TABLESPACE?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit It was being treated as any other configuration parameter. --- src/bin/psql/tab-complete.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/bin/psql/tab-complete.c b/src/bin/psql/tab-complete.c index bb696f8ee9..f107ffb027 100644 --- a/src/bin/psql/tab-complete.c +++ b/src/bin/psql/tab-complete.c @@ -1783,6 +1783,10 @@ psql_completion(const char *text, int start, int end) "IS_TEMPLATE", "ALLOW_CONNECTIONS", "CONNECTION LIMIT"); + /* ALTER DATABASE <name> SET TABLESPACE */ + else if (Matches5("ALTER", "DATABASE", MatchAny, "SET", "TABLESPACE")) + COMPLETE_WITH_QUERY(Query_for_list_of_tablespaces); + /* ALTER EVENT TRIGGER */ else if (Matches3("ALTER", "EVENT", "TRIGGER")) COMPLETE_WITH_QUERY(Query_for_list_of_event_triggers); -- 2.18.0
Hello, On Thu, Aug 30, 2018 at 05:54:23PM +0100, Dagfinn Ilmari Mannsåker wrote: > ilmari@ilmari.org (Dagfinn Ilmari Mannsåker) writes: > > Hi hackers, > > > > I just noticed that psql's tab completion for ALTER TABLE … SET > > TABLESPACE was treating it as any other configuration parameter and > > completing with FROM DEFAULT or TO after it, instead of a list of > > tablespaces. > > And just after hitting send, I noticed I'd typed ALTER TABLE instead of > ALTER DATABASE, including in the commit message :( > > Fixed patch attached. +1. Attached patch works nicely. It applies using "patch". -- Arthur Zakirov Postgres Professional: http://www.postgrespro.com Russian Postgres Company
On Thu, Aug 30, 2018 at 05:54:23PM +0100, Dagfinn Ilmari Mannsåker wrote: > ilmari@ilmari.org (Dagfinn Ilmari Mannsåker) writes: > > Hi hackers, > > > > I just noticed that psql's tab completion for ALTER TABLE … SET > > TABLESPACE was treating it as any other configuration parameter and > > completing with FROM DEFAULT or TO after it, instead of a list of > > tablespaces. > > And just after hitting send, I noticed I'd typed ALTER TABLE instead of > ALTER DATABASE, including in the commit message :( > > Fixed patch attached. The patch seems reasonable. It fixes the lack of tab completion for ALTER DATABASE ... SET TABLESPACE ... . There is no need to patch the documentation and regression tests. Marked as Ready for Commiter. -- Arthur Zakirov Postgres Professional: http://www.postgrespro.com Russian Postgres Company
Arthur Zakirov <a.zakirov@postgrespro.ru> writes: > On Thu, Aug 30, 2018 at 05:54:23PM +0100, Dagfinn Ilmari Mannsåker wrote: >> And just after hitting send, I noticed I'd typed ALTER TABLE instead of >> ALTER DATABASE, including in the commit message :( >> Fixed patch attached. > The patch seems reasonable. It fixes the lack of tab completion for > ALTER DATABASE ... SET TABLESPACE ... . LGTM too, pushed. regards, tom lane
On Fri, Sep 21, 2018 at 9:22 AM Tom Lane <tgl@sss.pgh.pa.us> wrote: > Arthur Zakirov <a.zakirov@postgrespro.ru> writes: > > On Thu, Aug 30, 2018 at 05:54:23PM +0100, Dagfinn Ilmari Mannsåker wrote: > >> And just after hitting send, I noticed I'd typed ALTER TABLE instead of > >> ALTER DATABASE, including in the commit message :( > >> Fixed patch attached. > > > The patch seems reasonable. It fixes the lack of tab completion for > > ALTER DATABASE ... SET TABLESPACE ... . > > LGTM too, pushed. + else if (Matches5("ALTER", "DATABASE", MatchAny, "SET", "TABLESPACE")) . o O ( hmm, we now have variadic macros ) -- Thomas Munro http://www.enterprisedb.com
Thomas Munro <thomas.munro@enterprisedb.com> writes: > . o O ( hmm, we now have variadic macros ) hmmm ... but even with variadic, C's macro facility is so weak that I'm not sure we can reimplement these with it. What would the expansion look like? (It constantly annoys me that C's so weak here. In the language I used for my first for-pay programming work, Bliss/11, the macro facility could have done that easily. That was 45 years ago.) regards, tom lane
On 2018-09-20 18:38:36 -0400, Tom Lane wrote: > Thomas Munro <thomas.munro@enterprisedb.com> writes: > > . o O ( hmm, we now have variadic macros ) > > hmmm ... but even with variadic, C's macro facility is so weak that > I'm not sure we can reimplement these with it. What would the > expansion look like? There's a dirty hack to count arguments in vararg macros: https://groups.google.com/d/msg/comp.std.c/d-6Mj5Lko_s/5fW1bP6T3RIJ Afaict that ought to be enough to implement something like the current macros? Whether that's too ugly or not, I don't know. > It constantly annoys me that C's so weak here. Yea. Weak and fragile, both. Greetings, Andres Freund
Andres Freund <andres@anarazel.de> writes: > On 2018-09-20 18:38:36 -0400, Tom Lane wrote: >> hmmm ... but even with variadic, C's macro facility is so weak that >> I'm not sure we can reimplement these with it. What would the >> expansion look like? > There's a dirty hack to count arguments in vararg macros: > https://groups.google.com/d/msg/comp.std.c/d-6Mj5Lko_s/5fW1bP6T3RIJ Doesn't seem to help for this case. What we really want is to expand a given pattern once for each variadic argument, and I don't see how to get there from here. Although maybe I'm thinking too much inside-the-box. The expansion doesn't necessarily have to be identical to the code the macros generate today. In fact, that code is kinda bulky. I wonder if we could go over to something involving a variadic function, or maybe an array of string-pointer constants? regards, tom lane
On 2018-09-20 19:03:16 -0400, Tom Lane wrote: > Andres Freund <andres@anarazel.de> writes: > > On 2018-09-20 18:38:36 -0400, Tom Lane wrote: > >> hmmm ... but even with variadic, C's macro facility is so weak that > >> I'm not sure we can reimplement these with it. What would the > >> expansion look like? > > > There's a dirty hack to count arguments in vararg macros: > > https://groups.google.com/d/msg/comp.std.c/d-6Mj5Lko_s/5fW1bP6T3RIJ > > Doesn't seem to help for this case. What we really want is to expand > a given pattern once for each variadic argument, and I don't see how > to get there from here. Depends on whether your goal is to simplify *using* the macro, or the infrastructure for the macro. Afaict it should be relatively straightforward to use a, possibly simplified, macro like referenced above to have TailMatches(...), TailMatchesCS(...), Matches(...) that then expand to the current *N macros. > Although maybe I'm thinking too much inside-the-box. The expansion > doesn't necessarily have to be identical to the code the macros > generate today. In fact, that code is kinda bulky. I wonder if > we could go over to something involving a variadic function, or > maybe an array of string-pointer constants? Yea, that might be a way to simplify both the macros and the use of the macros. Assuming we have something like PP_NARG, ISTM it should be relatively straightforward to define something like #define Matches(...) CheckMatchesFor(PP_NARG(__VA_ARGS__), __VA_ARGS__) and then have a CheckMatchesFor() first check previous_words_count, and then just have a simple for loop through previous_words[] - afaict the number of arguments ought to suffice to make that possible? Greetings, Andres Freund
On 2018-09-20 16:19:26 -0700, Andres Freund wrote: > On 2018-09-20 19:03:16 -0400, Tom Lane wrote: > > Andres Freund <andres@anarazel.de> writes: > > > On 2018-09-20 18:38:36 -0400, Tom Lane wrote: > > >> hmmm ... but even with variadic, C's macro facility is so weak that > > >> I'm not sure we can reimplement these with it. What would the > > >> expansion look like? > > > > > There's a dirty hack to count arguments in vararg macros: > > > https://groups.google.com/d/msg/comp.std.c/d-6Mj5Lko_s/5fW1bP6T3RIJ > > > > Doesn't seem to help for this case. What we really want is to expand > > a given pattern once for each variadic argument, and I don't see how > > to get there from here. > > Depends on whether your goal is to simplify *using* the macro, or the > infrastructure for the macro. Afaict it should be relatively > straightforward to use a, possibly simplified, macro like referenced > above to have TailMatches(...), TailMatchesCS(...), Matches(...) that > then expand to the current *N macros. > > > > Although maybe I'm thinking too much inside-the-box. The expansion > > doesn't necessarily have to be identical to the code the macros > > generate today. In fact, that code is kinda bulky. I wonder if > > we could go over to something involving a variadic function, or > > maybe an array of string-pointer constants? > > Yea, that might be a way to simplify both the macros and the use of the > macros. Assuming we have something like PP_NARG, ISTM it should be > relatively straightforward to define something like > > #define Matches(...) CheckMatchesFor(PP_NARG(__VA_ARGS__), __VA_ARGS__) > > and then have a CheckMatchesFor() first check previous_words_count, and > then just have a simple for loop through previous_words[] - afaict the > number of arguments ought to suffice to make that possible? Here's a very quick-and-dirty implementation of this approach. Some very very brief testing seems to indicate it works, although I'm sure not perfectly. The current duplication of the new functions doing the actual checking (like CheckMatchesFor(), CheckTailMatchesFor()) would probably need to be reduced. But I don't want to invest actual time before we decide that this could be something we'd actually want to pursue. Greetings, Andres Freund
Attachment
On Fri, Sep 21, 2018 at 5:52 PM Andres Freund <andres@anarazel.de> wrote: > Here's a very quick-and-dirty implementation of this approach. Some very > very brief testing seems to indicate it works, although I'm sure not > perfectly. > > The current duplication of the new functions doing the actual checking > (like CheckMatchesFor(), CheckTailMatchesFor()) would probably need to > be reduced. But I don't want to invest actual time before we decide that > this could be something we'd actually want to pursue. +1 And here is a quick-and-dirty variadic COMPLETE_WITH(...). Together: else if (Matches("PREPARE", MatchAny, "AS")) COMPLETE_WITH("SELECT", "UPDATE", "INSERT", "DELETE FROM"); + * Making C pretty by making it ugly again. Heh. -- Thomas Munro http://www.enterprisedb.com
Attachment
Thomas Munro <thomas.munro@enterprisedb.com> writes: > On Fri, Sep 21, 2018 at 5:52 PM Andres Freund <andres@anarazel.de> wrote: >> Here's a very quick-and-dirty implementation of this approach. Some very >> very brief testing seems to indicate it works, although I'm sure not >> perfectly. > And here is a quick-and-dirty variadic COMPLETE_WITH(...). Together: I'm a bit inclined to get rid of the need for PP_NARG() by instead making the macros add a trailing NULL argument, viz #define TailMatches(...) \ CheckTailMatchesFor(previous_words_count, previous_words, __VA_ARGS__, NULL) This'd require (some of) the implementation functions to iterate over their variadic arguments twice, the first time merely counting how many there are. But we aren't exactly concerned about max runtime performance here, and the PP_NARG() thing seems like a crock that could easily blow out compilation time on some compilers. If people are OK with that design decision, I'm happy to assemble these pieces and push it through. I just had to add two more versions of HeadMatchesN :-( so I'm feeling motivated to make something happen. regards, tom lane
Hi, On 2018-09-21 16:20:42 -0400, Tom Lane wrote: > Thomas Munro <thomas.munro@enterprisedb.com> writes: > > On Fri, Sep 21, 2018 at 5:52 PM Andres Freund <andres@anarazel.de> wrote: > >> Here's a very quick-and-dirty implementation of this approach. Some very > >> very brief testing seems to indicate it works, although I'm sure not > >> perfectly. > > > And here is a quick-and-dirty variadic COMPLETE_WITH(...). Together: > > I'm a bit inclined to get rid of the need for PP_NARG() by instead making > the macros add a trailing NULL argument, viz > > #define TailMatches(...) \ > CheckTailMatchesFor(previous_words_count, previous_words, __VA_ARGS__, NULL) I don't think that'd *quite* work right now - MatchAny is also NULL. We probably could make it work by redefining either MatchAny or the last argument to CheckTailMatchesFor() to some other value however. > This'd require (some of) the implementation functions to iterate over > their variadic arguments twice, the first time merely counting how many > there are. Yea, leaving the above problem aside, I've a hard time to get excited about that overhead. > But we aren't exactly concerned about max runtime performance > here, and the PP_NARG() thing seems like a crock that could easily blow > out compilation time on some compilers. It's not actually that complicated an expansion, and we've not encountered many problems with expansions that are similarly complex, e.g. ereport et al. It's pretty likely at least as cheap as the current approach, where we sometimes have 9 deep recursion. So I'm not too concerned about the compile-time performance. FWIW, I tested it with gcc -E yesterday, and I couldn't measure any difference. I think there's some argument to be made about the "mental" complexity of the macros - if we went for them, we'd certainly need to add some docs about how they work. One argument for having PP_NARGS (renamed) is that it doesn't seem useful just here, but in a few other cases as well. Greetings, Andres Freund
On Sat, Sep 22, 2018 at 8:51 AM Andres Freund <andres@anarazel.de> wrote: > I think there's some argument to be made about the "mental" complexity > of the macros - if we went for them, we'd certainly need to add some > docs about how they work. One argument for having PP_NARGS (renamed) is > that it doesn't seem useful just here, but in a few other cases as well. It's a nice general facility to have in the tree. It seems to compile OK on clang, gcc, MSVC (I added this thread as CF entry 20/1798 as a lazy way to see if AppVeyor would build it OK, and it worked fine until conflicting commits landed). I wonder if xlc, icc, aCC and Sun Studio can grok it. -- Thomas Munro http://www.enterprisedb.com
Hi, On 2018-09-22 09:15:27 +1200, Thomas Munro wrote: > On Sat, Sep 22, 2018 at 8:51 AM Andres Freund <andres@anarazel.de> wrote: > > I think there's some argument to be made about the "mental" complexity > > of the macros - if we went for them, we'd certainly need to add some > > docs about how they work. One argument for having PP_NARGS (renamed) is > > that it doesn't seem useful just here, but in a few other cases as well. > > It's a nice general facility to have in the tree. It seems to compile > OK on clang, gcc, MSVC (I added this thread as CF entry 20/1798 as a > lazy way to see if AppVeyor would build it OK, and it worked fine > until conflicting commits landed). I wonder if xlc, icc, aCC and Sun > Studio can grok it. I think unless $compiler doesn't correctly implement vararg macros, it really should just work. There's nothing but pretty smart use of actually pretty plain vararg macros. If any of the other compilers have troubles with that, they'd really not implement vararg macros... Greetings, Andres Freund
On Sat, Sep 22, 2018 at 9:46 AM Andres Freund <andres@anarazel.de> wrote: > On 2018-09-22 09:15:27 +1200, Thomas Munro wrote: > > On Sat, Sep 22, 2018 at 8:51 AM Andres Freund <andres@anarazel.de> wrote: > > > I think there's some argument to be made about the "mental" complexity > > > of the macros - if we went for them, we'd certainly need to add some > > > docs about how they work. One argument for having PP_NARGS (renamed) is > > > that it doesn't seem useful just here, but in a few other cases as well. > > > > It's a nice general facility to have in the tree. It seems to compile > > OK on clang, gcc, MSVC (I added this thread as CF entry 20/1798 as a > > lazy way to see if AppVeyor would build it OK, and it worked fine > > until conflicting commits landed). I wonder if xlc, icc, aCC and Sun > > Studio can grok it. > > I think unless $compiler doesn't correctly implement vararg macros, it > really should just work. There's nothing but pretty smart use of > actually pretty plain vararg macros. If any of the other compilers have > troubles with that, they'd really not implement vararg macros... I vote for doing it this way then. It may turn out to be useful for efficient SearchSysCache(...), DirectFunctionCall(...) and other things like that. -- Thomas Munro http://www.enterprisedb.com
Andres Freund <andres@anarazel.de> writes: > On 2018-09-22 09:15:27 +1200, Thomas Munro wrote: >> On Sat, Sep 22, 2018 at 8:51 AM Andres Freund <andres@anarazel.de> wrote: >>> I think there's some argument to be made about the "mental" complexity >>> of the macros - if we went for them, we'd certainly need to add some >>> docs about how they work. One argument for having PP_NARGS (renamed) is >>> that it doesn't seem useful just here, but in a few other cases as well. If you want to rename it, then to what? VA_ARGS_NARGS, perhaps? >> It's a nice general facility to have in the tree. Yeah, that's a fair point. >> It seems to compile >> OK on clang, gcc, MSVC (I added this thread as CF entry 20/1798 as a >> lazy way to see if AppVeyor would build it OK, and it worked fine >> until conflicting commits landed). I wonder if xlc, icc, aCC and Sun >> Studio can grok it. > I think unless $compiler doesn't correctly implement vararg macros, it > really should just work. Well, we'd find out pretty quickly if we try to use it here. regards, tom lane
On 2018-09-21 18:00:35 -0400, Tom Lane wrote: > Andres Freund <andres@anarazel.de> writes: > > On 2018-09-22 09:15:27 +1200, Thomas Munro wrote: > >> On Sat, Sep 22, 2018 at 8:51 AM Andres Freund <andres@anarazel.de> wrote: > >>> I think there's some argument to be made about the "mental" complexity > >>> of the macros - if we went for them, we'd certainly need to add some > >>> docs about how they work. One argument for having PP_NARGS (renamed) is > >>> that it doesn't seem useful just here, but in a few other cases as well. > > If you want to rename it, then to what? VA_ARGS_NARGS, perhaps? I like your suggestion. I mainly didn't like the PP_ prefix. > >> It's a nice general facility to have in the tree. > > Yeah, that's a fair point. > > >> It seems to compile > >> OK on clang, gcc, MSVC (I added this thread as CF entry 20/1798 as a > >> lazy way to see if AppVeyor would build it OK, and it worked fine > >> until conflicting commits landed). I wonder if xlc, icc, aCC and Sun > >> Studio can grok it. > > > I think unless $compiler doesn't correctly implement vararg macros, it > > really should just work. > > Well, we'd find out pretty quickly if we try to use it here. You earlier were talking about tackling this - do you still want to? I can otherwise, but it'll not be today, but likely tomorrow. Greetings, Andres Freund
Andres Freund <andres@anarazel.de> writes: > On 2018-09-21 18:00:35 -0400, Tom Lane wrote: >> If you want to rename it, then to what? VA_ARGS_NARGS, perhaps? > I like your suggestion. I mainly didn't like the PP_ prefix. Sold. The original author overcomplicated it anyway; I now have /* * VA_ARGS_NARGS * Returns the number of macro arguments it is passed. * * An empty argument still counts as an argument, so effectively, this is * "one more than the number of commas in the argument list". * * This works for up to 63 arguments. Internally, VA_ARGS_NARGS_() is passed * 64+N arguments, and the C99 standard only requires macros to allow up to * 127 arguments, so we can't portably go higher. The implementation is * pretty trivial: VA_ARGS_NARGS_() returns its 64th argument, and we set up * the call so that that is the appropriate one of the list of constants. */ #define VA_ARGS_NARGS(...) \ VA_ARGS_NARGS_(__VA_ARGS__, \ 63,62,61,60, \ 59,58,57,56,55,54,53,52,51,50, \ 49,48,47,46,45,44,43,42,41,40, \ 39,38,37,36,35,34,33,32,31,30, \ 29,28,27,26,25,24,23,22,21,20, \ 19,18,17,16,15,14,13,12,11,10, \ 9, 8, 7, 6, 5, 4, 3, 2, 1, 0) #define VA_ARGS_NARGS_( \ _01,_02,_03,_04,_05,_06,_07,_08,_09,_10, \ _11,_12,_13,_14,_15,_16,_17,_18,_19,_20, \ _21,_22,_23,_24,_25,_26,_27,_28,_29,_30, \ _31,_32,_33,_34,_35,_36,_37,_38,_39,_40, \ _41,_42,_43,_44,_45,_46,_47,_48,_49,_50, \ _51,_52,_53,_54,_55,_56,_57,_58,_59,_60, \ _61,_62,_63, N, ...) \ (N) from which it's pretty obvious that this really is a simple macro call. I'd first thought it involved macro recursion, but it doesn't. > You earlier were talking about tackling this - do you still want to? I > can otherwise, but it'll not be today, but likely tomorrow. On it now. regards, tom lane
Hi, On 2018-09-21 19:27:48 -0400, Tom Lane wrote: > > You earlier were talking about tackling this - do you still want to? I > > can otherwise, but it'll not be today, but likely tomorrow. > > On it now. Thanks, looks good. msvc and icc are, as expected, ok too. Greetings, Andres Freund
Hi, On 2018-09-22 09:56:00 +1200, Thomas Munro wrote: > I vote for doing it this way then. It may turn out to be useful for > efficient SearchSysCache(...), DirectFunctionCall(...) and other > things like that. Yea, especially the *FunctionCall* stuff is awfully verbose. I also wonder if it could make http://archives.postgresql.org/message-id/20180605172952.x34m5uz6ju6enaem%40alap3.anarazel.de a bit less painful. Greetings, Andres Freund