Thread: psql tab completion bug for ALL IN TABLESPACE
Hi all,
I just bumped into the following thing while looking again at Thomas'
patch for psql tab completion:
--- a/src/bin/psql/tab-complete.c
+++ b/src/bin/psql/tab-complete.c
@@ -1040,7 +1040,7 @@ psql_completion(const char *text, int start, int end)
pg_strcasecmp(prev5_wd, "IN") == 0 &&
pg_strcasecmp(prev4_wd, "TABLESPACE") == 0 &&
pg_strcasecmp(prev2_wd, "OWNED") == 0 &&
- pg_strcasecmp(prev4_wd, "BY") == 0)
+ pg_strcasecmp(prev_wd, "BY") == 0)
{
COMPLETE_WITH_QUERY(Query_for_list_of_roles);
This should be backpatched, attached is the needed patch.
Regards,
--
Michael
Attachment
On 2015-12-12 21:04:31 +0900, Michael Paquier wrote:
> Hi all,
>
> I just bumped into the following thing while looking again at Thomas'
> patch for psql tab completion:
> --- a/src/bin/psql/tab-complete.c
> +++ b/src/bin/psql/tab-complete.c
> @@ -1040,7 +1040,7 @@ psql_completion(const char *text, int start, int end)
> pg_strcasecmp(prev5_wd, "IN") == 0 &&
> pg_strcasecmp(prev4_wd, "TABLESPACE") == 0 &&
> pg_strcasecmp(prev2_wd, "OWNED") == 0 &&
> - pg_strcasecmp(prev4_wd, "BY") == 0)
> + pg_strcasecmp(prev_wd, "BY") == 0)
> {
> COMPLETE_WITH_QUERY(Query_for_list_of_roles);
> This should be backpatched, attached is the needed patch.
Hm, this seems to need slightly more expansive surgery.
Trivially the comments for ALL IN TABLESPACE seem broken/badly copy pasted: /* ALTER TABLE,INDEX,MATERIALIZED
VIEWxxx ALL IN TABLESPACE xxx */
the first xxx doesnt make sense.
Secondly the OWNED BY completion then breaks the SET TABLESPACE
completion. That's maybe not an outright bug, but seems odd nonetheless.
Fujii, Stephen?
On Mon, Dec 14, 2015 at 8:18 PM, Andres Freund <andres@anarazel.de> wrote:
> On 2015-12-12 21:04:31 +0900, Michael Paquier wrote:
>> Hi all,
>>
>> I just bumped into the following thing while looking again at Thomas'
>> patch for psql tab completion:
>> --- a/src/bin/psql/tab-complete.c
>> +++ b/src/bin/psql/tab-complete.c
>> @@ -1040,7 +1040,7 @@ psql_completion(const char *text, int start, int end)
>> pg_strcasecmp(prev5_wd, "IN") == 0 &&
>> pg_strcasecmp(prev4_wd, "TABLESPACE") == 0 &&
>> pg_strcasecmp(prev2_wd, "OWNED") == 0 &&
>> - pg_strcasecmp(prev4_wd, "BY") == 0)
>> + pg_strcasecmp(prev_wd, "BY") == 0)
>> {
>> COMPLETE_WITH_QUERY(Query_for_list_of_roles);
>> This should be backpatched, attached is the needed patch.
>
> Hm, this seems to need slightly more expansive surgery.
>
> Trivially the comments for ALL IN TABLESPACE seem broken/badly copy pasted:
> /* ALTER TABLE,INDEX,MATERIALIZED VIEW xxx ALL IN TABLESPACE xxx */
> the first xxx doesnt make sense.
Indeed.
> Secondly the OWNED BY completion then breaks the SET TABLESPACE
> completion. That's maybe not an outright bug, but seems odd nonetheless.
You mean that this code should do the completion of SET TABLESPACE
after "OWNED BY xxx" has been typed? Are you sure it's worth going
this far?
--
Michael
On 2015-12-14 12:18:27 +0100, Andres Freund wrote:
> On 2015-12-12 21:04:31 +0900, Michael Paquier wrote:
> > Hi all,
> >
> > I just bumped into the following thing while looking again at Thomas'
> > patch for psql tab completion:
> > --- a/src/bin/psql/tab-complete.c
> > +++ b/src/bin/psql/tab-complete.c
> > @@ -1040,7 +1040,7 @@ psql_completion(const char *text, int start, int end)
> > pg_strcasecmp(prev5_wd, "IN") == 0 &&
> > pg_strcasecmp(prev4_wd, "TABLESPACE") == 0 &&
> > pg_strcasecmp(prev2_wd, "OWNED") == 0 &&
> > - pg_strcasecmp(prev4_wd, "BY") == 0)
> > + pg_strcasecmp(prev_wd, "BY") == 0)
> > {
> > COMPLETE_WITH_QUERY(Query_for_list_of_roles);
> > This should be backpatched, attached is the needed patch.
>
> Hm, this seems to need slightly more expansive surgery.
>
> Trivially the comments for ALL IN TABLESPACE seem broken/badly copy pasted:
> /* ALTER TABLE,INDEX,MATERIALIZED VIEW xxx ALL IN TABLESPACE xxx */
> the first xxx doesnt make sense.
>
> Secondly the OWNED BY completion then breaks the SET TABLESPACE
> completion. That's maybe not an outright bug, but seems odd nonetheless.
>
> Fujii, Stephen?
I'm off to do something else for a while, but attached is where I
stopped at.
Attachment
On Mon, Dec 14, 2015 at 8:36 PM, Andres Freund <andres@anarazel.de> wrote:
> On 2015-12-14 12:18:27 +0100, Andres Freund wrote:
>> On 2015-12-12 21:04:31 +0900, Michael Paquier wrote:
>> > Hi all,
>> >
>> > I just bumped into the following thing while looking again at Thomas'
>> > patch for psql tab completion:
>> > --- a/src/bin/psql/tab-complete.c
>> > +++ b/src/bin/psql/tab-complete.c
>> > @@ -1040,7 +1040,7 @@ psql_completion(const char *text, int start, int end)
>> > pg_strcasecmp(prev5_wd, "IN") == 0 &&
>> > pg_strcasecmp(prev4_wd, "TABLESPACE") == 0 &&
>> > pg_strcasecmp(prev2_wd, "OWNED") == 0 &&
>> > - pg_strcasecmp(prev4_wd, "BY") == 0)
>> > + pg_strcasecmp(prev_wd, "BY") == 0)
>> > {
>> > COMPLETE_WITH_QUERY(Query_for_list_of_roles);
>> > This should be backpatched, attached is the needed patch.
>>
>> Hm, this seems to need slightly more expansive surgery.
>>
>> Trivially the comments for ALL IN TABLESPACE seem broken/badly copy pasted:
>> /* ALTER TABLE,INDEX,MATERIALIZED VIEW xxx ALL IN TABLESPACE xxx */
>> the first xxx doesnt make sense.
>>
>> Secondly the OWNED BY completion then breaks the SET TABLESPACE
>> completion. That's maybe not an outright bug, but seems odd nonetheless.
>>
>> Fujii, Stephen?
>
> I'm off to do something else for a while, but attached is where I
> stopped at.
Just a bit more and you can get the whole set...
--
Michael
Attachment
On 2015-12-14 20:44:20 +0900, Michael Paquier wrote:
> + /*
> + * ALTER TABLE,INDEX,MATERIALIZED VIEW ALL IN TABLESPACE xxx OWNED BY xxx
> + * SET TABLESPACE.
> + */
> + else if (pg_strcasecmp(prev9_wd, "ALL") == 0 &&
> + pg_strcasecmp(prev8_wd, "IN") == 0 &&
> + pg_strcasecmp(prev7_wd, "TABLESPACE") == 0 &&
> + pg_strcasecmp(prev5_wd, "OWNED") == 0 &&
> + pg_strcasecmp(prev4_wd, "BY") == 0 &&
> + pg_strcasecmp(prev2_wd, "SET") == 0 &&
> + pg_strcasecmp(prev_wd, "TABLESPACE") == 0)
> + {
> + COMPLETE_WITH_QUERY(Query_for_list_of_tablespaces);
> + }
Isn't that already handled by the normal SET TABLESPACE case?
On Mon, Dec 14, 2015 at 8:49 PM, Andres Freund <andres@anarazel.de> wrote:
> On 2015-12-14 20:44:20 +0900, Michael Paquier wrote:
>> + /*
>> + * ALTER TABLE,INDEX,MATERIALIZED VIEW ALL IN TABLESPACE xxx OWNED BY xxx
>> + * SET TABLESPACE.
>> + */
>> + else if (pg_strcasecmp(prev9_wd, "ALL") == 0 &&
>> + pg_strcasecmp(prev8_wd, "IN") == 0 &&
>> + pg_strcasecmp(prev7_wd, "TABLESPACE") == 0 &&
>> + pg_strcasecmp(prev5_wd, "OWNED") == 0 &&
>> + pg_strcasecmp(prev4_wd, "BY") == 0 &&
>> + pg_strcasecmp(prev2_wd, "SET") == 0 &&
>> + pg_strcasecmp(prev_wd, "TABLESPACE") == 0)
>> + {
>> + COMPLETE_WITH_QUERY(Query_for_list_of_tablespaces);
>> + }
>
> Isn't that already handled by the normal SET TABLESPACE case?
No, There is no SET TABLESPACE case, there is a TABLE SET TABLESPACE
though. Just removing the TABLE seems to be fine..
--
Michael
On 2015-12-14 20:58:21 +0900, Michael Paquier wrote:
> On Mon, Dec 14, 2015 at 8:49 PM, Andres Freund <andres@anarazel.de> wrote:
> > On 2015-12-14 20:44:20 +0900, Michael Paquier wrote:
> >> + /*
> >> + * ALTER TABLE,INDEX,MATERIALIZED VIEW ALL IN TABLESPACE xxx OWNED BY xxx
> >> + * SET TABLESPACE.
> >> + */
> >> + else if (pg_strcasecmp(prev9_wd, "ALL") == 0 &&
> >> + pg_strcasecmp(prev8_wd, "IN") == 0 &&
> >> + pg_strcasecmp(prev7_wd, "TABLESPACE") == 0 &&
> >> + pg_strcasecmp(prev5_wd, "OWNED") == 0 &&
> >> + pg_strcasecmp(prev4_wd, "BY") == 0 &&
> >> + pg_strcasecmp(prev2_wd, "SET") == 0 &&
> >> + pg_strcasecmp(prev_wd, "TABLESPACE") == 0)
> >> + {
> >> + COMPLETE_WITH_QUERY(Query_for_list_of_tablespaces);
> >> + }
> >
> > Isn't that already handled by the normal SET TABLESPACE case?
>
> No, There is no SET TABLESPACE case, there is a TABLE SET TABLESPACE
> though. Just removing the TABLE seems to be fine..
ALTER TABLE ALL IN TABLESPACE pg_default OWNED BY andres SET TABLESPACE <tab>
works, because of
/* * Finally, we look through the list of "things", such as TABLE, INDEX and * check if that was the previous word. If
so,execute the query to get a * list of them. */else{ int i;
for (i = 0; words_after_create[i].name; i++) { if (pg_strcasecmp(prev_wd, words_after_create[i].name) ==
0) { if (words_after_create[i].query) COMPLETE_WITH_QUERY(words_after_create[i].query);
else if (words_after_create[i].squery)
COMPLETE_WITH_SCHEMA_QUERY(*words_after_create[i].squery, NULL);
break; } }}
On Mon, Dec 14, 2015 at 9:08 PM, Andres Freund <andres@anarazel.de> wrote: > ALTER TABLE ALL IN TABLESPACE pg_default OWNED BY andres SET TABLESPACE <tab> > works, because of Missed that. - /* If we have TABLE <sth> SET TABLESPACE provide a list of tablespaces */ - else if (pg_strcasecmp(prev4_wd, "TABLE") == 0 && - pg_strcasecmp(prev2_wd, "SET") == 0 && - pg_strcasecmp(prev_wd, "TABLESPACE") == 0) - COMPLETE_WITH_QUERY(Query_for_list_of_tablespaces); So you could get rid of that as well. -- Michael