Re: [HACKERS] Tab completion for ALTER COLUMN SET STATISTICS
On Mon, Dec 21, 2015 at 3:34 PM, Jeff Janes wrote: > On Tue, Dec 1, 2015 at 10:37 PM, Michael Paquier > 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Tab completion for ALTER COLUMN SET STATISTICS
On Tue, Dec 1, 2015 at 10:37 PM, Michael Paquier 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Tab completion for ALTER COLUMN SET STATISTICS
On Wed, Nov 18, 2015 at 2:12 AM, Jeff Janes wrote:
> On Mon, Sep 28, 2015 at 8:48 AM, Robert Haas wrote:
>> On Sat, Sep 26, 2015 at 7:24 AM, Michael Paquier
>> wrote:
>>> On Sat, Sep 26, 2015 at 7:18 AM, Jeff Janes 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", "" 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 TO
> ALTER DATABASE SET TO
> ALTER ROLE SET TO
> ALTER USER SET TO
> ALTER FUNCTION ( )
> SET 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 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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Tab completion for ALTER COLUMN SET STATISTICS
On Mon, Sep 28, 2015 at 8:48 AM, Robert Haas wrote:
> On Sat, Sep 26, 2015 at 7:24 AM, Michael Paquier
> wrote:
>> On Sat, Sep 26, 2015 at 7:18 AM, Jeff Janes 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", "" 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 TO
ALTER DATABASE SET TO
ALTER ROLE SET TO
ALTER USER SET TO
ALTER FUNCTION ( )
SET 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 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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Tab completion for ALTER COLUMN SET STATISTICS
On Sat, Sep 26, 2015 at 7:24 AM, Michael Paquier
wrote:
> On Sat, Sep 26, 2015 at 7:18 AM, Jeff Janes 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", "" 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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Tab completion for ALTER COLUMN SET STATISTICS
On Sat, Sep 26, 2015 at 7:18 AM, Jeff Janes 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", "" 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] 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", "" 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. alter_column_tabcomplete_v1.patch Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
