Thread
Commits
-
psql: Add tab completion for view options.
- cd342474890f 17.0 landed
-
Fix typos in code and comments
- 5f6401f81cb2 16.0 cited
-
[PATCH] psql: Add tab-complete for optional view parameters
Christoph Heiss <christoph@c8h4.io> — 2022-12-07T19:44:27Z
Hi all! This adds tab-complete for optional parameters (as can be specified using WITH) for views, similarly to how it works for storage parameters of tables. Thanks, Christoph Heiss
-
Re: [PATCH] psql: Add tab-complete for optional view parameters
Melih Mutlu <m.melihmutlu@gmail.com> — 2022-12-08T11:19:25Z
Hi Christoph, I just took a quick look at your patch. Some suggestions: + else if (Matches("ALTER", "VIEW", MatchAny, "SET", "(")) > + COMPLETE_WITH_LIST(view_optional_parameters); > + /* ALTER VIEW xxx RESET ( yyy , ... ) */ > + else if (Matches("ALTER", "VIEW", MatchAny, "RESET", "(")) > + COMPLETE_WITH_LIST(view_optional_parameters); What about combining these two cases into one like Matches("ALTER", "VIEW", MatchAny, "SET|RESET", "(") ? /* ALTER VIEW <name> */ > else if (Matches("ALTER", "VIEW", MatchAny)) > COMPLETE_WITH("ALTER COLUMN", "OWNER TO", "RENAME", > "SET SCHEMA"); Also seems like SET and RESET don't get auto-completed for "ALTER VIEW <name>". I think it would be nice to include those missing words. Thanks, -- Melih Mutlu Microsoft -
Re: [PATCH] psql: Add tab-complete for optional view parameters
Christoph Heiss <christoph@c8h4.io> — 2022-12-09T10:31:21Z
Thanks for the review! On 12/8/22 12:19, Melih Mutlu wrote: > Hi Christoph, > > I just took a quick look at your patch. > Some suggestions: > > + else if (Matches("ALTER", "VIEW", MatchAny, "SET", "(")) > + COMPLETE_WITH_LIST(view_optional_parameters); > + /* ALTER VIEW xxx RESET ( yyy , ... ) */ > + else if (Matches("ALTER", "VIEW", MatchAny, "RESET", "(")) > + COMPLETE_WITH_LIST(view_optional_parameters); > > > What about combining these two cases into one like Matches("ALTER", > "VIEW", MatchAny, "SET|RESET", "(") ? Good thinking, incorporated that into v2. While at it, I also added completion for the values of the SET parameters, since that is useful as well. > > /* ALTER VIEW <name> */ > else if (Matches("ALTER", "VIEW", MatchAny)) > COMPLETE_WITH("ALTER COLUMN", "OWNER TO", "RENAME", > "SET SCHEMA"); > > > Also seems like SET and RESET don't get auto-completed for "ALTER VIEW > <name>". > I think it would be nice to include those missing words. That is already contained in the patch: @@ -2139,7 +2146,7 @@ psql_completion(const char *text, int start, int end) /* ALTER VIEW <name> */ else if (Matches("ALTER", "VIEW", MatchAny)) COMPLETE_WITH("ALTER COLUMN", "OWNER TO", "RENAME", - "SET SCHEMA"); + "SET SCHEMA", "SET (", "RESET ("); Thanks, Christoph -
Re: [PATCH] psql: Add tab-complete for optional view parameters
vignesh C <vignesh21@gmail.com> — 2023-01-06T11:51:42Z
On Fri, 9 Dec 2022 at 16:01, Christoph Heiss <christoph@c8h4.io> wrote: > > Thanks for the review! > > On 12/8/22 12:19, Melih Mutlu wrote: > > Hi Christoph, > > > > I just took a quick look at your patch. > > Some suggestions: > > > > + else if (Matches("ALTER", "VIEW", MatchAny, "SET", "(")) > > + COMPLETE_WITH_LIST(view_optional_parameters); > > + /* ALTER VIEW xxx RESET ( yyy , ... ) */ > > + else if (Matches("ALTER", "VIEW", MatchAny, "RESET", "(")) > > + COMPLETE_WITH_LIST(view_optional_parameters); > > > > > > What about combining these two cases into one like Matches("ALTER", > > "VIEW", MatchAny, "SET|RESET", "(") ? > Good thinking, incorporated that into v2. > While at it, I also added completion for the values of the SET > parameters, since that is useful as well. > > > > > /* ALTER VIEW <name> */ > > else if (Matches("ALTER", "VIEW", MatchAny)) > > COMPLETE_WITH("ALTER COLUMN", "OWNER TO", "RENAME", > > "SET SCHEMA"); > > > > > > Also seems like SET and RESET don't get auto-completed for "ALTER VIEW > > <name>". > > I think it would be nice to include those missing words. > That is already contained in the patch: > > @@ -2139,7 +2146,7 @@ psql_completion(const char *text, int start, int end) > /* ALTER VIEW <name> */ > else if (Matches("ALTER", "VIEW", MatchAny)) > COMPLETE_WITH("ALTER COLUMN", "OWNER TO", "RENAME", > - "SET SCHEMA"); > + "SET SCHEMA", "SET (", "RESET ("); One suggestion: Tab completion for "alter view v1 set (check_option =" is handled to complete with CASCADED and LOCAL but the same is not handled for create view: "create view viewname with ( check_option =" + else if (Matches("ALTER", "VIEW", MatchAny, "SET", "(", "check_option", "=")) + COMPLETE_WITH("local", "cascaded"); I felt we should keep the handling consistent for both create view and alter view. Regards, Vignesh -
Re: [PATCH] psql: Add tab-complete for optional view parameters
Dean Rasheed <dean.a.rasheed@gmail.com> — 2023-01-06T12:18:44Z
On Fri, 6 Jan 2023 at 11:52, vignesh C <vignesh21@gmail.com> wrote: > > One suggestion: > Tab completion for "alter view v1 set (check_option =" is handled to > complete with CASCADED and LOCAL but the same is not handled for > create view: "create view viewname with ( check_option =" > + else if (Matches("ALTER", "VIEW", MatchAny, "SET", "(", > "check_option", "=")) > + COMPLETE_WITH("local", "cascaded"); > > I felt we should keep the handling consistent for both create view and > alter view. > Hmm, I don't think we should be offering "check_option" as a tab completion for CREATE VIEW at all, since that would encourage users to use non-SQL-standard syntax, rather than CREATE VIEW ... WITH [CASCADED|LOCAL] CHECK OPTION. Regards, Dean -
Re: [PATCH] psql: Add tab-complete for optional view parameters
Jim Jones <jim.jones@uni-muenster.de> — 2023-01-09T15:32:09Z
Hi Christoph, Thanks for the patch! I just tested it and I could reproduce the expected behaviour in these cases: postgres=# CREATE VIEW w AS WITH ( postgres=# CREATE OR REPLACE VIEW w AS WITH ( postgres=# CREATE VIEW w WITH ( CHECK_OPTION SECURITY_BARRIER SECURITY_INVOKER postgres=# CREATE OR REPLACE VIEW w WITH ( CHECK_OPTION SECURITY_BARRIER SECURITY_INVOKER postgres=# ALTER VIEW w ALTER COLUMN OWNER TO RENAME RESET ( SET ( SET SCHEMA postgres=# ALTER VIEW w RESET ( CHECK_OPTION SECURITY_BARRIER SECURITY_INVOKER postgres=# ALTER VIEW w SET (check_option = CASCADED LOCAL However, an "ALTER TABLE <name> S<tab>" does not complete the open parenthesis "(" from "SET (", as suggested in "ALTER VIEW <name> <tab>". postgres=# ALTER VIEW w SET Display all 187 possibilities? (y or n) Is it intended to behave like this? If so, an "ALTER VIEW <name> RES<tab>" does complete the open parenthesis -> "RESET (". Best, Jim On 09.12.22 11:31, Christoph Heiss wrote: > Thanks for the review! > > On 12/8/22 12:19, Melih Mutlu wrote: >> Hi Christoph, >> >> I just took a quick look at your patch. >> Some suggestions: >> >> + else if (Matches("ALTER", "VIEW", MatchAny, "SET", "(")) >> + COMPLETE_WITH_LIST(view_optional_parameters); >> + /* ALTER VIEW xxx RESET ( yyy , ... ) */ >> + else if (Matches("ALTER", "VIEW", MatchAny, "RESET", "(")) >> + COMPLETE_WITH_LIST(view_optional_parameters); >> >> >> What about combining these two cases into one like Matches("ALTER", >> "VIEW", MatchAny, "SET|RESET", "(") ? > Good thinking, incorporated that into v2. > While at it, I also added completion for the values of the SET > parameters, since that is useful as well. > >> >> /* ALTER VIEW <name> */ >> else if (Matches("ALTER", "VIEW", MatchAny)) >> COMPLETE_WITH("ALTER COLUMN", "OWNER TO", "RENAME", >> "SET SCHEMA"); >> >> >> Also seems like SET and RESET don't get auto-completed for "ALTER >> VIEW <name>". >> I think it would be nice to include those missing words. > That is already contained in the patch: > > @@ -2139,7 +2146,7 @@ psql_completion(const char *text, int start, int > end) > /* ALTER VIEW <name> */ > else if (Matches("ALTER", "VIEW", MatchAny)) > COMPLETE_WITH("ALTER COLUMN", "OWNER TO", "RENAME", > - "SET SCHEMA"); > + "SET SCHEMA", "SET (", "RESET ("); > > Thanks, > Christoph -
Re: [PATCH] psql: Add tab-complete for optional view parameters
vignesh C <vignesh21@gmail.com> — 2023-01-11T16:50:29Z
On Fri, 9 Dec 2022 at 16:01, Christoph Heiss <christoph@c8h4.io> wrote: > > Thanks for the review! > > On 12/8/22 12:19, Melih Mutlu wrote: > > Hi Christoph, > > > > I just took a quick look at your patch. > > Some suggestions: > > > > + else if (Matches("ALTER", "VIEW", MatchAny, "SET", "(")) > > + COMPLETE_WITH_LIST(view_optional_parameters); > > + /* ALTER VIEW xxx RESET ( yyy , ... ) */ > > + else if (Matches("ALTER", "VIEW", MatchAny, "RESET", "(")) > > + COMPLETE_WITH_LIST(view_optional_parameters); > > > > > > What about combining these two cases into one like Matches("ALTER", > > "VIEW", MatchAny, "SET|RESET", "(") ? > Good thinking, incorporated that into v2. > While at it, I also added completion for the values of the SET > parameters, since that is useful as well. > > > > > /* ALTER VIEW <name> */ > > else if (Matches("ALTER", "VIEW", MatchAny)) > > COMPLETE_WITH("ALTER COLUMN", "OWNER TO", "RENAME", > > "SET SCHEMA"); > > > > > > Also seems like SET and RESET don't get auto-completed for "ALTER VIEW > > <name>". > > I think it would be nice to include those missing words. > That is already contained in the patch: > > @@ -2139,7 +2146,7 @@ psql_completion(const char *text, int start, int end) > /* ALTER VIEW <name> */ > else if (Matches("ALTER", "VIEW", MatchAny)) > COMPLETE_WITH("ALTER COLUMN", "OWNER TO", "RENAME", > - "SET SCHEMA"); > + "SET SCHEMA", "SET (", "RESET ("); For some reason CFBot is not able to apply the patch, please have a look and post an updated version if required, also check and handle Dean Rasheed and Jim Jones comment if applicable: === Applying patches on top of PostgreSQL commit ID 5f6401f81cb24bd3930e0dc589fc4aa8b5424cdc === === applying patch ./v2-0001-psql-Add-tab-complete-for-optional-view-parameter.patch gpatch: **** Only garbage was found in the patch input. [1] - http://cfbot.cputube.org/patch_41_4053.log Regards, Vignesh -
Re: [PATCH] psql: Add tab-complete for optional view parameters
Thomas Munro <thomas.munro@gmail.com> — 2023-01-11T22:29:11Z
On Thu, Jan 12, 2023 at 5:50 AM vignesh C <vignesh21@gmail.com> wrote: > For some reason CFBot is not able to apply the patch, please have a > look and post an updated version if required, also check and handle > Dean Rasheed and Jim Jones comment if applicable: > === Applying patches on top of PostgreSQL commit ID > 5f6401f81cb24bd3930e0dc589fc4aa8b5424cdc === > === applying patch > ./v2-0001-psql-Add-tab-complete-for-optional-view-parameter.patch > gpatch: **** Only garbage was found in the patch input. Melanie pointed me at this issue. This particular entry is now fixed, and I think I know what happened: cfbot wasn't checking the HTTP status when downloading patches from the web archives, because I had incorrectly assumed Python's requests.get() would raise an exception if the web server sent an error status, but it turns out you have to ask for that. I've now fixed that. So I think it was probably trying to apply one of those "guru meditation" error message the web archives occasionally spit out.
-
Re: [PATCH] psql: Add tab-complete for optional view parameters
Mikhail Gribkov <youzhick@gmail.com> — 2023-01-29T10:19:12Z
The following review has been posted through the commitfest application: make installcheck-world: tested, passed Implements feature: tested, passed Spec compliant: tested, failed Documentation: tested, passed Hi Christoph, The patch have a potential, although I have to agree with Jim Jones, it obviously have a problem with "alter view <name> set<tab>" handling. First of all user can notice, that SET and RESET alternatives are proposed in an absolutely equivalent way: postgres=# alter view VVV <tab> ALTER COLUMN OWNER TO RENAME RESET ( SET ( SET SCHEMA But completion of a parentheses differs fore these alternatives: postgres=# alter view VVV reset<tab> -> completes with "<space>(" -> <tab> CHECK_OPTION SECURITY_BARRIER SECURITY_INVOKER postgres=# alter view VVV set<tab> -> completes with a single spase -> <tab> Display all 188 possibilities? (y or n) (and these 188 possibilities do not contain "(") The probmen is obviously in the newly added second line of the following clause: COMPLETE_WITH("ALTER COLUMN", "OWNER TO", "RENAME", "SET SCHEMA", "SET (", "RESET ("); "set schema" and "set (" alternatives are competing, while completion of the common part "set<space>" leads to a string composition which does not have the check branch (Matches("ALTER", "VIEW", MatchAny, "SET")). I think it may worth looking at "alter materialized view" completion tree and making "alter view" the same way. The new status of this patch is: Waiting on Author -
Re: [PATCH] psql: Add tab-complete for optional view parameters
Gregory Stark (as CFM) <stark.cfm@gmail.com> — 2023-03-20T18:40:43Z
On Sun, 29 Jan 2023 at 05:20, Mikhail Gribkov <youzhick@gmail.com> wrote: > > The problem is obviously in the newly added second line of the following clause: > COMPLETE_WITH("ALTER COLUMN", "OWNER TO", "RENAME", > "SET SCHEMA", "SET (", "RESET ("); > > "set schema" and "set (" alternatives are competing, while completion of the common part "set<space>" leads to a string composition which does not have the check branch (Matches("ALTER", "VIEW", MatchAny, "SET")). > > I think it may worth looking at "alter materialized view" completion tree and making "alter view" the same way. > > The new status of this patch is: Waiting on Author I think this patch received real feedback and it looks like it's clear where there's more work needed. I'll move this to the next commitfest. If you plan to work on it this month we can always move it back. -- Gregory Stark As Commitfest Manager -
Re: [PATCH] psql: Add tab-complete for optional view parameters
Daniel Gustafsson <daniel@yesql.se> — 2023-07-06T13:28:33Z
> On 29 Jan 2023, at 11:19, Mikhail Gribkov <youzhick@gmail.com> wrote: > The new status of this patch is: Waiting on Author This patch has been Waiting on Author since January with the thread being stale, so I am marking this as Returned with Feedback for now. Please feel free to resubmit to a future CF when there is renewed interest in working on this. -- Daniel Gustafsson
-
Re: [PATCH] psql: Add tab-complete for optional view parameters
Christoph Heiss <christoph@c8h4.io> — 2023-08-07T18:49:04Z
Hi all, sorry for the long delay. On Mon, Jan 09, 2023 at 04:32:09PM +0100, Jim Jones wrote: > However, an "ALTER TABLE <name> S<tab>" does not complete the open > parenthesis "(" from "SET (", as suggested in "ALTER VIEW <name> <tab>". > > postgres=# ALTER VIEW w SET > Display all 187 possibilities? (y or n) > > Is it intended to behave like this? If so, an "ALTER VIEW <name> > RES<tab>" does complete the open parenthesis -> "RESET (". On Sun, Jan 29, 2023 at 10:19:12AM +0000, Mikhail Gribkov wrote: > The patch have a potential, although I have to agree with Jim Jones, > it obviously have a problem with "alter view <name> set<tab>" > handling. > [..] > I think it may worth looking at "alter materialized view" completion > tree and making "alter view" the same way. Thank you both for reviewing/testing and the suggestions. Yeah, definitively, sounds very sensible. I've attached a new revision, rebased and addressing the above by aligning it with how "ALTER MATERIALIZED VIEW" works, such that "SET (" and "SET SCHEMA" won't compete anymore. So that should now work more like expected. postgres=# ALTER MATERIALIZED VIEW m ALTER COLUMN CLUSTER ON DEPENDS ON EXTENSION NO DEPENDS ON EXTENSION OWNER TO RENAME RESET ( SET postgres=# ALTER MATERIALIZED VIEW m SET ( ACCESS METHOD SCHEMA TABLESPACE WITHOUT CLUSTER postgres=# ALTER VIEW v ALTER COLUMN OWNER TO RENAME RESET ( SET postgres=# ALTER VIEW v SET ( SCHEMA postgres=# ALTER VIEW v SET ( CHECK_OPTION SECURITY_BARRIER SECURITY_INVOKER On Fri, Jan 06, 2023 at 12:18:44PM +0000, Dean Rasheed wrote: > Hmm, I don't think we should be offering "check_option" as a tab > completion for CREATE VIEW at all, since that would encourage users to > use non-SQL-standard syntax, rather than CREATE VIEW ... WITH > [CASCADED|LOCAL] CHECK OPTION. Left that part in for now. I would argue that it is a well-documented combination and as such users would expect it to turn up in the tab-complete as well. OTOH not against removing it either, if there are others voicing the same opinion .. Thanks, Christoph -
Re: [PATCH] psql: Add tab-complete for optional view parameters
Dean Rasheed <dean.a.rasheed@gmail.com> — 2023-08-08T08:17:51Z
On Mon, 7 Aug 2023 at 19:49, Christoph Heiss <christoph@c8h4.io> wrote: > > On Fri, Jan 06, 2023 at 12:18:44PM +0000, Dean Rasheed wrote: > > Hmm, I don't think we should be offering "check_option" as a tab > > completion for CREATE VIEW at all, since that would encourage users to > > use non-SQL-standard syntax, rather than CREATE VIEW ... WITH > > [CASCADED|LOCAL] CHECK OPTION. > > Left that part in for now. I would argue that it is a well-documented > combination and as such users would expect it to turn up in the > tab-complete as well. OTOH not against removing it either, if there are > others voicing the same opinion .. > On reflection, I think that's probably OK. I mean, I still don't like the fact that it's offering to complete with non-SQL-standard syntax, but that seems less bad than using an incomplete list of options, and I don't really have any other better ideas. Regards, Dean
-
Re: [PATCH] psql: Add tab-complete for optional view parameters
Christoph Heiss <christoph@c8h4.io> — 2023-08-08T10:21:08Z
On Tue, Aug 08, 2023 at 09:17:51AM +0100, Dean Rasheed wrote: > > On Mon, 7 Aug 2023 at 19:49, Christoph Heiss <christoph@c8h4.io> wrote: > > > > On Fri, Jan 06, 2023 at 12:18:44PM +0000, Dean Rasheed wrote: > > > Hmm, I don't think we should be offering "check_option" as a tab > > > completion for CREATE VIEW at all, since that would encourage users to > > > use non-SQL-standard syntax, rather than CREATE VIEW ... WITH > > > [CASCADED|LOCAL] CHECK OPTION. > > > > Left that part in for now. I would argue that it is a well-documented > > combination and as such users would expect it to turn up in the > > tab-complete as well. OTOH not against removing it either, if there are > > others voicing the same opinion .. > > > > On reflection, I think that's probably OK. I mean, I still don't like > the fact that it's offering to complete with non-SQL-standard syntax, > but that seems less bad than using an incomplete list of options, and > I don't really have any other better ideas. My thought pretty much as well. While obviously far from ideal as you say, it's the less bad case of these both. I would also guess that it is not the only instance of non-SQL-standard syntax completion in psql .. Thanks for weighing in once again. Cheers, Christoph
-
Re: [PATCH] psql: Add tab-complete for optional view parameters
David Zhang <david.zhang@highgo.ca> — 2023-08-11T19:48:17Z
Applied v3 patch to master and verified it with below commands, #Alter view postgres=# alter view v <tab> ALTER COLUMN OWNER TO RENAME RESET ( SET postgres=# alter view v set <tab> ( SCHEMA postgres=# alter view v set ( <tab> CHECK_OPTION SECURITY_BARRIER SECURITY_INVOKER postgres=# alter view v reset ( <tab> CHECK_OPTION SECURITY_BARRIER SECURITY_INVOKER postgres=# alter view v set ( check_option = <tab> CASCADED LOCAL postgres=# alter view v set ( security_barrier = <tab> FALSE TRUE postgres=# alter view v set ( security_invoker = <tab> FALSE TRUE #Create view postgres=# create view v AS WITH ( postgres=# create or replace view v AS WITH ( postgres=# create view v with ( CHECK_OPTION SECURITY_BARRIER SECURITY_INVOKER postgres=# create or replace view v with ( CHECK_OPTION SECURITY_BARRIER SECURITY_INVOKER postgres=# create view v with (*)<tab>AS postgres=# create or replace view v with (*)<tab>AS postgres=# create view v as <tab>SELECT postgres=# create or replace view v as <tab>SELECT For below changes, else if (TailMatches("CREATE", "VIEW", MatchAny, "AS") || - TailMatches("CREATE", "OR", "REPLACE", "VIEW", MatchAny, "AS")) + TailMatches("CREATE", "VIEW", MatchAny, "WITH", "(*)", "AS") || + TailMatches("CREATE", "OR", "REPLACE", "VIEW", MatchAny, "AS") || + TailMatches("CREATE", "OR", "REPLACE", "VIEW", MatchAny, "WITH", "(*)", "AS")) it would be great to switch the order of the 3rd and the 4th line to make a better match for "CREATE" and "CREATE OR REPLACE" . Since it supports <tab> in the middle for below case, postgres=# alter view v set ( security_<tab> security_barrier security_invoker and during view reset it can also provide all the options list, postgres=# alter view v reset ( CHECK_OPTION SECURITY_BARRIER SECURITY_INVOKER but not sure if it is a good idea or possible to autocomplete the reset options after seeing one of the options showing up with "," for example, postgres=# alter view v reset ( CHECK_OPTION, <tab> SECURITY_BARRIER SECURITY_INVOKER Thank you, David -
Re: [PATCH] psql: Add tab-complete for optional view parameters
Christoph Heiss <christoph@c8h4.io> — 2023-08-12T18:09:52Z
On Fri, Aug 11, 2023 at 12:48:17PM -0700, David Zhang wrote: > > Applied v3 patch to master and verified it with below commands, Thanks for testing! > [..] > > For below changes, > > else if (TailMatches("CREATE", "VIEW", MatchAny, "AS") || > - TailMatches("CREATE", "OR", "REPLACE", "VIEW", MatchAny, > "AS")) > + TailMatches("CREATE", "VIEW", MatchAny, "WITH", "(*)", "AS") > || > + TailMatches("CREATE", "OR", "REPLACE", "VIEW", MatchAny, "AS") > || > + TailMatches("CREATE", "OR", "REPLACE", "VIEW", MatchAny, > "WITH", "(*)", "AS")) > > it would be great to switch the order of the 3rd and the 4th line to make a > better match for "CREATE" and "CREATE OR REPLACE" . I don't see how it would effect matching in any way - or am I overlooking something here? postgres=# CREATE VIEW v <tab> AS WITH ( postgres=# CREATE VIEW v AS <tab> .. autocompletes with "SELECT" postgres=# CREATE VIEW v WITH ( security_invoker = true ) <tab> .. autocompletes with "AS" and so on And the same for CREATE OR REPLACE VIEW. Can you provide an example case that would benefit from that? > > Since it supports <tab> in the middle for below case, > postgres=# alter view v set ( security_<tab> > security_barrier security_invoker > > and during view reset it can also provide all the options list, > postgres=# alter view v reset ( > CHECK_OPTION SECURITY_BARRIER SECURITY_INVOKER > > but not sure if it is a good idea or possible to autocomplete the reset > options after seeing one of the options showing up with "," for example, > postgres=# alter view v reset ( CHECK_OPTION, <tab> > SECURITY_BARRIER SECURITY_INVOKER I'd rather not add this and leave it as-is. It doesn't add any real value - how often does this case really come up, especially with ALTER VIEW only having three options? Thanks, Christoph -
Re: [PATCH] psql: Add tab-complete for optional view parameters
David Zhang <david.zhang@highgo.ca> — 2023-08-14T17:34:11Z
>> [..] >> >> For below changes, >> >> else if (TailMatches("CREATE", "VIEW", MatchAny, "AS") || >> - TailMatches("CREATE", "OR", "REPLACE", "VIEW", MatchAny, >> "AS")) >> + TailMatches("CREATE", "VIEW", MatchAny, "WITH", "(*)", "AS") >> || >> + TailMatches("CREATE", "OR", "REPLACE", "VIEW", MatchAny, "AS") >> || >> + TailMatches("CREATE", "OR", "REPLACE", "VIEW", MatchAny, >> "WITH", "(*)", "AS")) >> >> it would be great to switch the order of the 3rd and the 4th line to make a >> better match for "CREATE" and "CREATE OR REPLACE" . > I don't see how it would effect matching in any way - or am I > overlooking something here? It won't affect the SQL matching. What I was trying to say is that using 'CREATE OR REPLACE ...' after 'CREATE ...' can enhance code structure, making it more readable. For instance, /* Complete CREATE [ OR REPLACE ] VIEW <name> WITH ( ... ) with "AS" */ else if (TailMatches("CREATE", "VIEW", MatchAny, "WITH", "(*)") || TailMatches("CREATE", "OR", "REPLACE", "VIEW", MatchAny, "WITH", "(*)")) COMPLETE_WITH("AS"); "CREATE", "OR", "REPLACE", "VIEW", MatchAny, "WITH", "(*)" follows "CREATE", "VIEW", MatchAny, "WITH", "(*)") immediately. best regards, David -
Re: [PATCH] psql: Add tab-complete for optional view parameters
Peter Eisentraut <peter@eisentraut.org> — 2023-09-06T06:52:14Z
I noticed that on this commitfest entry (https://commitfest.postgresql.org/44/4491/), the reviewers were assigned by the patch author (presumably because they had previously contributed to this thread). Unless these individuals know about that, this is unlikely to work out. It's better to remove the reviewer entries and let people sign up on their own. (You can nudge potential candidates, of course.) On 07.08.23 20:49, Christoph Heiss wrote: > > Hi all, > sorry for the long delay. > > On Mon, Jan 09, 2023 at 04:32:09PM +0100, Jim Jones wrote: >> However, an "ALTER TABLE <name> S<tab>" does not complete the open >> parenthesis "(" from "SET (", as suggested in "ALTER VIEW <name> <tab>". >> >> postgres=# ALTER VIEW w SET >> Display all 187 possibilities? (y or n) >> >> Is it intended to behave like this? If so, an "ALTER VIEW <name> >> RES<tab>" does complete the open parenthesis -> "RESET (". > > On Sun, Jan 29, 2023 at 10:19:12AM +0000, Mikhail Gribkov wrote: >> The patch have a potential, although I have to agree with Jim Jones, >> it obviously have a problem with "alter view <name> set<tab>" >> handling. >> [..] >> I think it may worth looking at "alter materialized view" completion >> tree and making "alter view" the same way. > > Thank you both for reviewing/testing and the suggestions. Yeah, > definitively, sounds very sensible. > > I've attached a new revision, rebased and addressing the above by > aligning it with how "ALTER MATERIALIZED VIEW" works, such that "SET (" > and "SET SCHEMA" won't compete anymore. So that should now work more > like expected. > > postgres=# ALTER MATERIALIZED VIEW m > ALTER COLUMN CLUSTER ON DEPENDS ON EXTENSION > NO DEPENDS ON EXTENSION OWNER TO RENAME > RESET ( SET > > postgres=# ALTER MATERIALIZED VIEW m SET > ( ACCESS METHOD SCHEMA TABLESPACE > WITHOUT CLUSTER > > postgres=# ALTER VIEW v > ALTER COLUMN OWNER TO RENAME RESET ( SET > > postgres=# ALTER VIEW v SET > ( SCHEMA > > postgres=# ALTER VIEW v SET ( > CHECK_OPTION SECURITY_BARRIER SECURITY_INVOKER > > On Fri, Jan 06, 2023 at 12:18:44PM +0000, Dean Rasheed wrote: >> Hmm, I don't think we should be offering "check_option" as a tab >> completion for CREATE VIEW at all, since that would encourage users to >> use non-SQL-standard syntax, rather than CREATE VIEW ... WITH >> [CASCADED|LOCAL] CHECK OPTION. > > Left that part in for now. I would argue that it is a well-documented > combination and as such users would expect it to turn up in the > tab-complete as well. OTOH not against removing it either, if there are > others voicing the same opinion .. > > Thanks, > Christoph -
Re: [PATCH] psql: Add tab-complete for optional view parameters
Dean Rasheed <dean.a.rasheed@gmail.com> — 2023-11-23T11:07:33Z
On Mon, 14 Aug 2023 at 18:34, David Zhang <david.zhang@highgo.ca> wrote: > > it would be great to switch the order of the 3rd and the 4th line to make a > better match for "CREATE" and "CREATE OR REPLACE" . > I took a look at this, and I think it's probably neater to keep the "AS SELECT" completion for CREATE [OR REPLACE] VIEW xxx WITH (*) separate from the already existing support for "AS SELECT" without WITH. A couple of other points: 1. It looks slightly neater, and works better, to complete one word at a time -- e.g., "WITH" then "(", instead of "WITH (", since the latter doesn't work if the user has already typed "WITH". 2. It should also complete with "=" after the option, where appropriate. 3. CREATE VIEW should offer "local" and "cascaded" after "check_option" (though there's no point in doing likewise for the boolean options, since they default to true, if present, and false otherwise). Attached is an updated patch, incorporating those comments. Barring any further comments, I think this is ready for commit. Regards, Dean -
Re: [PATCH] psql: Add tab-complete for optional view parameters
Shubham Khanna <khannashubham1197@gmail.com> — 2023-11-28T03:42:34Z
On Thu, Nov 23, 2023 at 4:37 PM Dean Rasheed <dean.a.rasheed@gmail.com> wrote: > > On Mon, 14 Aug 2023 at 18:34, David Zhang <david.zhang@highgo.ca> wrote: > > > > it would be great to switch the order of the 3rd and the 4th line to make a > > better match for "CREATE" and "CREATE OR REPLACE" . > > > > I took a look at this, and I think it's probably neater to keep the > "AS SELECT" completion for CREATE [OR REPLACE] VIEW xxx WITH (*) > separate from the already existing support for "AS SELECT" without > WITH. > > A couple of other points: > > 1. It looks slightly neater, and works better, to complete one word at > a time -- e.g., "WITH" then "(", instead of "WITH (", since the latter > doesn't work if the user has already typed "WITH". > > 2. It should also complete with "=" after the option, where appropriate. > > 3. CREATE VIEW should offer "local" and "cascaded" after > "check_option" (though there's no point in doing likewise for the > boolean options, since they default to true, if present, and false > otherwise). > > Attached is an updated patch, incorporating those comments. > > Barring any further comments, I think this is ready for commit. I reviewed the given Patch and it is working fine. Thanks and Regards, Shubham Khanna. -
Re: [PATCH] psql: Add tab-complete for optional view parameters
Dean Rasheed <dean.a.rasheed@gmail.com> — 2023-11-28T10:00:44Z
On Tue, 28 Nov 2023 at 03:42, Shubham Khanna <khannashubham1197@gmail.com> wrote: > > I reviewed the given Patch and it is working fine. > Thanks for checking. Patch pushed. Regards, Dean