Re: Tab completion for ALTER COLUMN SET STATISTICS

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