Thread: Tab completion for ALTER COLUMN SET STATISTICS
If I have "alter table foo alter COLUMN bar SET STATISTICS" in the line buffer,
it tab completes to add " TO", which is not legal.
The attached patch makes it not tab complete anything at all, which is at least not actively misleading.
I thought of having it complete "-1", "<integer>" so that it gives a clue about what is needed, but I didn't see any precedence for non-literal clue-giving and I did not want to try to create new precedence.
Attachment
On Sat, Sep 26, 2015 at 7:18 AM, Jeff Janes <jeff.janes@gmail.com> wrote: > If I have "alter table foo alter COLUMN bar SET STATISTICS" in the line > buffer, > it tab completes to add " TO", which is not legal. > > The attached patch makes it not tab complete anything at all, which is at > least not actively misleading. > I thought of having it complete "-1", "<integer>" so that it gives a clue > about what is needed, but I didn't see any precedence for non-literal > clue-giving and I did not want to try to create new precedence. +1 for the way you are doing it in your patch. -- Michael
On Sat, Sep 26, 2015 at 7:24 AM, Michael Paquier
<michael.paquier@gmail.com> wrote:
> On Sat, Sep 26, 2015 at 7:18 AM, Jeff Janes <jeff.janes@gmail.com> wrote:
>> If I have "alter table foo alter COLUMN bar SET STATISTICS" in the line
>> buffer,
>> it tab completes to add " TO", which is not legal.
>>
>> The attached patch makes it not tab complete anything at all, which is at
>> least not actively misleading.
>> I thought of having it complete "-1", "<integer>" so that it gives a clue
>> about what is needed, but I didn't see any precedence for non-literal
>> clue-giving and I did not want to try to create new precedence.
>
> +1 for the way you are doing it in your patch.
Before we take that approach, can I back up and ask whether we
shouldn't instead narrow the rule that's inserting TO? I think that
completion is coming from here:
else if (pg_strcasecmp(prev2_wd, "SET") == 0 && pg_strcasecmp(prev4_wd, "UPDATE") != 0 &&
pg_strcasecmp(prev_wd,"TABLESPACE") != 0 && pg_strcasecmp(prev_wd, "SCHEMA") != 0 &&
prev_wd[strlen(prev_wd)- 1] != ')' && prev_wd[strlen(prev_wd) - 1] != '=' &&
pg_strcasecmp(prev4_wd,"DOMAIN") != 0) COMPLETE_WITH_CONST("TO");
Now, that is basically an incredibly broad production: every time the
second-most recent word is SET, complete with TO. It looks to me like
this has already been patched around multiple times to make it
slightly narrower. Those exceptions were added by three different
commits, in 2004, 2010, and 2012.
Maybe it's time to back up and make the whole thing a lot narrower.
Like, if second-most-recent word of the command is also the FIRST word
of the command, this is probably right. And there may be a few other
situations where it's right. But in general, SET is used in lots of
places and the fact that we've seen one recently isn't enough to
decide in TO.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
On Mon, Sep 28, 2015 at 8:48 AM, Robert Haas <robertmhaas@gmail.com> wrote:
> On Sat, Sep 26, 2015 at 7:24 AM, Michael Paquier
> <michael.paquier@gmail.com> wrote:
>> On Sat, Sep 26, 2015 at 7:18 AM, Jeff Janes <jeff.janes@gmail.com> wrote:
>>> If I have "alter table foo alter COLUMN bar SET STATISTICS" in the line
>>> buffer,
>>> it tab completes to add " TO", which is not legal.
>>>
>>> The attached patch makes it not tab complete anything at all, which is at
>>> least not actively misleading.
>>> I thought of having it complete "-1", "<integer>" so that it gives a clue
>>> about what is needed, but I didn't see any precedence for non-literal
>>> clue-giving and I did not want to try to create new precedence.
>>
>> +1 for the way you are doing it in your patch.
>
> Before we take that approach, can I back up and ask whether we
> shouldn't instead narrow the rule that's inserting TO? I think that
> completion is coming from here:
>
> else if (pg_strcasecmp(prev2_wd, "SET") == 0 &&
> pg_strcasecmp(prev4_wd, "UPDATE") != 0 &&
> pg_strcasecmp(prev_wd, "TABLESPACE") != 0 &&
> pg_strcasecmp(prev_wd, "SCHEMA") != 0 &&
> prev_wd[strlen(prev_wd) - 1] != ')' &&
> prev_wd[strlen(prev_wd) - 1] != '=' &&
> pg_strcasecmp(prev4_wd, "DOMAIN") != 0)
> COMPLETE_WITH_CONST("TO");
>
> Now, that is basically an incredibly broad production: every time the
> second-most recent word is SET, complete with TO. It looks to me like
> this has already been patched around multiple times to make it
> slightly narrower. Those exceptions were added by three different
> commits, in 2004, 2010, and 2012.
>
> Maybe it's time to back up and make the whole thing a lot narrower.
> Like, if second-most-recent word of the command is also the FIRST word
> of the command, this is probably right. And there may be a few other
> situations where it's right. But in general, SET is used in lots of
> places and the fact that we've seen one recently isn't enough to
> decide in TO.
There are quite a few places where TO is legitimately the 2nd word
after SET. I don't know how to do an exhaustive survey of them, but
here are the ones I know about:
SET <word> TO
ALTER DATABASE <word> SET <word> TO
ALTER ROLE <word> SET <word> TO
ALTER USER <word> SET <word> TO
ALTER FUNCTION <word> ( <any number of words with commas between> )
SET <word other than 'schema'> TO
I don't know if coding the non-TO cases as exceptions is easier or
harder than coding the TO cases more tightly.
In the case of "SET SCHEMA", it seems like we might be able to just
move that case higher in the giant list of 'else if' so that it
triggers before getting to the generic SET <word> code. But I can't
figure out where in the file that code is, to see if it might be
moved. And I'm not sure that intentionally relying on order more than
already is a better strategy, it seems kind of fragile.
In any case, I'd rather not try re-factor this part of the code while
there is another large refactoring pending. But I think an
incremental improvement would be acceptable.
Cheers,
Jeff
On Wed, Nov 18, 2015 at 2:12 AM, Jeff Janes <jeff.janes@gmail.com> wrote:
> On Mon, Sep 28, 2015 at 8:48 AM, Robert Haas <robertmhaas@gmail.com> wrote:
>> On Sat, Sep 26, 2015 at 7:24 AM, Michael Paquier
>> <michael.paquier@gmail.com> wrote:
>>> On Sat, Sep 26, 2015 at 7:18 AM, Jeff Janes <jeff.janes@gmail.com> wrote:
>>>> If I have "alter table foo alter COLUMN bar SET STATISTICS" in the line
>>>> buffer,
>>>> it tab completes to add " TO", which is not legal.
>>>>
>>>> The attached patch makes it not tab complete anything at all, which is at
>>>> least not actively misleading.
>>>> I thought of having it complete "-1", "<integer>" so that it gives a clue
>>>> about what is needed, but I didn't see any precedence for non-literal
>>>> clue-giving and I did not want to try to create new precedence.
>>>
>>> +1 for the way you are doing it in your patch.
>>
>> Before we take that approach, can I back up and ask whether we
>> shouldn't instead narrow the rule that's inserting TO? I think that
>> completion is coming from here:
>>
>> else if (pg_strcasecmp(prev2_wd, "SET") == 0 &&
>> pg_strcasecmp(prev4_wd, "UPDATE") != 0 &&
>> pg_strcasecmp(prev_wd, "TABLESPACE") != 0 &&
>> pg_strcasecmp(prev_wd, "SCHEMA") != 0 &&
>> prev_wd[strlen(prev_wd) - 1] != ')' &&
>> prev_wd[strlen(prev_wd) - 1] != '=' &&
>> pg_strcasecmp(prev4_wd, "DOMAIN") != 0)
>> COMPLETE_WITH_CONST("TO");
>>
>> Now, that is basically an incredibly broad production: every time the
>> second-most recent word is SET, complete with TO. It looks to me like
>> this has already been patched around multiple times to make it
>> slightly narrower. Those exceptions were added by three different
>> commits, in 2004, 2010, and 2012.
>>
>> Maybe it's time to back up and make the whole thing a lot narrower.
>> Like, if second-most-recent word of the command is also the FIRST word
>> of the command, this is probably right. And there may be a few other
>> situations where it's right. But in general, SET is used in lots of
>> places and the fact that we've seen one recently isn't enough to
>> decide in TO.
>
> There are quite a few places where TO is legitimately the 2nd word
> after SET. I don't know how to do an exhaustive survey of them, but
> here are the ones I know about:
>
> SET <word> TO
> ALTER DATABASE <word> SET <word> TO
> ALTER ROLE <word> SET <word> TO
> ALTER USER <word> SET <word> TO
> ALTER FUNCTION <word> ( <any number of words with commas between> )
> SET <word other than 'schema'> TO
>
> I don't know if coding the non-TO cases as exceptions is easier or
> harder than coding the TO cases more tightly.
>
> In the case of "SET SCHEMA", it seems like we might be able to just
> move that case higher in the giant list of 'else if' so that it
> triggers before getting to the generic SET <word> code. But I can't
> figure out where in the file that code is, to see if it might be
> moved. And I'm not sure that intentionally relying on order more than
> already is a better strategy, it seems kind of fragile.
>
> In any case, I'd rather not try re-factor this part of the code while
> there is another large refactoring pending. But I think an
> incremental improvement would be acceptable.
With the refactoring of tab-complete.c that is moving on, perhaps this
entry should be moved to next CF. Thoughts?
--
Michael
On Tue, Dec 1, 2015 at 10:37 PM, Michael Paquier <michael.paquier@gmail.com> wrote: > > With the refactoring of tab-complete.c that is moving on, perhaps this > entry should be moved to next CF. Thoughts? The now-committed tab-complete.c refactoring renders this unnecessary, so I've marked this patch as rejected. Thanks, Jeff
On Mon, Dec 21, 2015 at 3:34 PM, Jeff Janes <jeff.janes@gmail.com> wrote: > On Tue, Dec 1, 2015 at 10:37 PM, Michael Paquier > <michael.paquier@gmail.com> wrote: >> >> With the refactoring of tab-complete.c that is moving on, perhaps this >> entry should be moved to next CF. Thoughts? > > The now-committed tab-complete.c refactoring renders this unnecessary, > so I've marked this patch as rejected. Thanks for the follow-up. -- Michael