Re: Tab completion for ALTER COLUMN SET STATISTICS - Mailing list pgsql-hackers

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