Thread: [PATCH] psql: Add tab-complete for optional view parameters
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
Attachment
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
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
Attachment
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
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
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
Attachment
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
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.
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
compositionwhich 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
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
stringcomposition 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
> 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
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
Attachment
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
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
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
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
>> [..]
>>
>> 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
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
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
Attachment
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.
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