Thread: Making tab-complete.c easier to maintain

Thomas Munro <thomas.munro@enterprisedb.com> writes:
> See attached a proof-of-concept patch.  It reduces the size of
> tab-complete.c by a bit over a thousand lines.  I realise that changing so
> many lines just to refactor code may may be a difficult sell!  But I think
> this would make it easier to improve the tab completion code in future, and
> although it's large it's a superficial and mechanical change.

I really dislike the magical "<" business.  Maybe you could use NULL
instead of having to reserve a class of real strings as placeholders.
(That would require finding another way to denote the end of the list,
but a separate count argument seems like a possible answer.)
Or perhaps use empty strings as placeholders?
        regards, tom lane

Thomas Munro wrote:

> Thanks, good point.  Here's a version that uses NULL via a macro ANY.
> Aside from a few corrections it also now distinguishes between
> TAIL_MATCHESn (common) and MATCHESn (rarely used for now), for example:

This looks pretty neat -- 100x neater than what we have, at any rate.  I
would use your new MATCHESn() macros a bit more -- for instance the
completion for "ALTER but not ALTER after ALTER TABLE" could be
rephrased as simply MATCHES1("ALTER"), i.e. have it match at start of
command only.  Maybe that's just a matter of going over the new code
after the initial run, so that we can have a first patch that's mostly
mechanical and a second pass in which more semantically relevant changes
are applied.  Seems easier to review ...

I would use "ANY" as a keyword here.  Sounds way too generic to me.
Maybe "CompleteAny" or something like that.

Stylistically, I find there's too much uppercasing here.  Maybe rename the
macros like this instead:

> +       else if (TailMatches4("ALL", "IN", "TABLESPACE", ANY))
> +               CompleteWithList2("SET TABLESPACE", "OWNED BY");

Not totally sure about this part TBH.

-- 
Álvaro Herrera                http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Hi

Here is a new version merging recent changes.

--
Thomas Munro
http://www.enterprisedb.com

On Mon, Oct 19, 2015 at 4:58 PM, Jeff Janes <jeff.janes@gmail.com> wrote:
> On Sun, Oct 18, 2015 at 5:31 PM, Thomas Munro
> <thomas.munro@enterprisedb.com> wrote:
>>
>> Hi
>>
>> Here is a new version merging recent changes.
>
>
> For reasons I do not understand, "SET work_mem " does not complete with
> "TO".
>
> But if I change:
>
> else if (Matches2("SET", MatchAny))
>
> to:
>
> else if (TailMatches2("SET", MatchAny))
>
> Then it does.

Thanks for taking a look at this!  The word count returned by
get_previous_words was incorrect.  Here is a corrected version.

--
Thomas Munro
http://www.enterprisedb.com

Here is a new version merging the recent CREATE EXTENSION ... VERSION
patch from master.

(Apologies for sending so many versions.  tab-complete.c keeps moving
and I want to keep a version that applies on top of master out there,
for anyone interested in looking at this.  As long as no one objects
and there is interest in the patch, I'll keep doing that.)

--
Thomas Munro
http://www.enterprisedb.com

On Wed, Oct 21, 2015 at 8:54 PM, Thomas Munro
<thomas.munro@enterprisedb.com> wrote:
> Here is a new version merging the recent CREATE EXTENSION ... VERSION
> patch from master.
>
> (Apologies for sending so many versions.  tab-complete.c keeps moving
> and I want to keep a version that applies on top of master out there,
> for anyone interested in looking at this.  As long as no one objects
> and there is interest in the patch, I'll keep doing that.)

I don't want to rain on the parade since other people seem to like
this, but I'm sort of unimpressed by this.  Yes, it removes >1000
lines of code, and that's not nothing.  But it's all mechanical code,
so, not to be dismissive, but who really cares?  Is it really worth
replacing the existing notation that we all know with a new one that
we have to learn?  I'm not violently opposed if someone else wants to
commit this, but I'm unexcited about it.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Robert Haas <robertmhaas@gmail.com> writes:
> On Wed, Oct 21, 2015 at 8:54 PM, Thomas Munro
> <thomas.munro@enterprisedb.com> wrote:
>> (Apologies for sending so many versions.  tab-complete.c keeps moving
>> and I want to keep a version that applies on top of master out there,
>> for anyone interested in looking at this.  As long as no one objects
>> and there is interest in the patch, I'll keep doing that.)

> I don't want to rain on the parade since other people seem to like
> this, but I'm sort of unimpressed by this.  Yes, it removes >1000
> lines of code, and that's not nothing.  But it's all mechanical code,
> so, not to be dismissive, but who really cares?  Is it really worth
> replacing the existing notation that we all know with a new one that
> we have to learn?  I'm not violently opposed if someone else wants to
> commit this, but I'm unexcited about it.

What I would like is to find a way to auto-generate basically this entire
file from gram.y.  That would imply going over to something at least
somewhat parser-based, instead of the current way that is more or less
totally ad-hoc.  That would be a very good thing though, because the
current way gives wrong answers not-infrequently, even discounting cases
that it's simply not been taught about.

I have no very good idea how to do that, though.  Bison does have a
notion of which symbols are possible as the next symbol at any given
parse point, but it doesn't really make that accessible.  There's a lack
of cooperation on the readline side too: we'd need to be able to see the
whole query buffer not just the current line.
        regards, tom lane

Tom Lane wrote:

> What I would like is to find a way to auto-generate basically this entire
> file from gram.y.  That would imply going over to something at least
> somewhat parser-based, instead of the current way that is more or less
> totally ad-hoc.  That would be a very good thing though, because the
> current way gives wrong answers not-infrequently, even discounting cases
> that it's simply not been taught about.

I did discuss exactly this topic with Thomas a month ago or so in
private email, and our conclusion was that it would be a really neat
project but a lot more effort than this patch.  And after skimming over
the patch back then, I think this is well worth the reduced maintenance
effort.

> I have no very good idea how to do that, though.  Bison does have a
> notion of which symbols are possible as the next symbol at any given
> parse point, but it doesn't really make that accessible.  There's a lack
> of cooperation on the readline side too: we'd need to be able to see the
> whole query buffer not just the current line.

At the current pace, a project like this might take years to
turn into a real patch.  My own vote is for applying this for the time
being.

-- 
Álvaro Herrera                http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On Thu, Oct 22, 2015 at 02:36:53PM -0700, Tom Lane wrote:
> Robert Haas <robertmhaas@gmail.com> writes:
> > On Wed, Oct 21, 2015 at 8:54 PM, Thomas Munro
> > <thomas.munro@enterprisedb.com> wrote:
> >> (Apologies for sending so many versions.  tab-complete.c keeps moving
> >> and I want to keep a version that applies on top of master out there,
> >> for anyone interested in looking at this.  As long as no one objects
> >> and there is interest in the patch, I'll keep doing that.)
> 
> > I don't want to rain on the parade since other people seem to like
> > this, but I'm sort of unimpressed by this.  Yes, it removes >1000
> > lines of code, and that's not nothing.  But it's all mechanical code,
> > so, not to be dismissive, but who really cares?  Is it really worth
> > replacing the existing notation that we all know with a new one that
> > we have to learn?  I'm not violently opposed if someone else wants to
> > commit this, but I'm unexcited about it.
> 
> What I would like is to find a way to auto-generate basically this entire
> file from gram.y.

I've been hoping we could use a principled approach for years.  My
fondest hope along that line would also involve catalog access, so it
could correctly tab-complete user-defined things, but I have the
impression that the catalog access variant is "much later" even if
autogeneration from gram.y is merely "soon."  I'd love to be wrong
about that.

> That would imply going over to something at least
> somewhat parser-based, instead of the current way that is more or less
> totally ad-hoc.  That would be a very good thing though, because the
> current way gives wrong answers not-infrequently, even discounting cases
> that it's simply not been taught about.

Indeed.

> I have no very good idea how to do that, though.  Bison does have a
> notion of which symbols are possible as the next symbol at any given
> parse point, but it doesn't really make that accessible.  There's a lack
> of cooperation on the readline side too: we'd need to be able to see the
> whole query buffer not just the current line.

This may be on point:

http://stackoverflow.com/questions/161495/is-there-a-nice-way-of-handling-multi-line-input-with-gnu-readline

I suspect we might have to stop pretending to support alternatives to
libreadline if we went that direction, not that that would necessarily
be a bad idea.

Cheers,
David.
-- 
David Fetter <david@fetter.org> http://fetter.org/
Phone: +1 415 235 3778  AIM: dfetter666  Yahoo!: dfetter
Skype: davidfetter      XMPP: david.fetter@gmail.com

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate

David Fetter <david@fetter.org> writes:
> On Thu, Oct 22, 2015 at 02:36:53PM -0700, Tom Lane wrote:
>> I have no very good idea how to do that, though.  Bison does have a
>> notion of which symbols are possible as the next symbol at any given
>> parse point, but it doesn't really make that accessible.  There's a lack
>> of cooperation on the readline side too: we'd need to be able to see the
>> whole query buffer not just the current line.

> This may be on point:

> http://stackoverflow.com/questions/161495/is-there-a-nice-way-of-handling-multi-line-input-with-gnu-readline

> I suspect we might have to stop pretending to support alternatives to
> libreadline if we went that direction, not that that would necessarily
> be a bad idea.

Given the license issues around GNU readline, requiring it seems like
probably a non-starter.

It strikes me though that maybe we don't need readline's cooperation.
I think it's already true that the previous lines of the query buffer
are stashed somewhere that psql knows about, so in principle we could
sew them together with the current line.  That might be a project worth
tackling on its own, since we could make the existing code smarter about
multiline queries, whether or not we ever get to a grammar-based solution.
        regards, tom lane

On Thu, Oct 22, 2015 at 04:05:06PM -0700, Tom Lane wrote:
> David Fetter <david@fetter.org> writes:
> > On Thu, Oct 22, 2015 at 02:36:53PM -0700, Tom Lane wrote:
> >> I have no very good idea how to do that, though.  Bison does have a
> >> notion of which symbols are possible as the next symbol at any given
> >> parse point, but it doesn't really make that accessible.  There's a lack
> >> of cooperation on the readline side too: we'd need to be able to see the
> >> whole query buffer not just the current line.
> 
> > This may be on point:
> 
> > http://stackoverflow.com/questions/161495/is-there-a-nice-way-of-handling-multi-line-input-with-gnu-readline
> 
> > I suspect we might have to stop pretending to support alternatives to
> > libreadline if we went that direction, not that that would necessarily
> > be a bad idea.
> 
> Given the license issues around GNU readline, requiring it seems like
> probably a non-starter.

I should have made this more clear.  I am not an IP attorney and don't
play one on TV, but this is what I've gotten out of conversations with
IP attorneys on this subject:

- If we allow people to disable readline at compile time, the software is not GPL even if only libreadline would add
thecommand line editing capability.
 

- When people do compile with libreadline, the only software affected by libreadine's license is the binary to which it
islinked, namely the psql client. 
 
To be affective negatively by libreadline's viral license, an entity
would need to fork the psql client in proprietary ways that they did
not wish not to make available to end users, at the same time linking
in libreadline.

Maybe I'm missing something big, but I really don't see people out
there shipping a libreadline-enabled psql client, details of whose
source they'd want to keep a deep, dark secret.

If this gets to matter, we can probably get /pro bono/ work from IP
attorneys specializing in just this kind of thing.

> It strikes me though that maybe we don't need readline's
> cooperation.  I think it's already true that the previous lines of
> the query buffer are stashed somewhere that psql knows about, so in
> principle we could sew them together with the current line.  That
> might be a project worth tackling on its own, since we could make
> the existing code smarter about multiline queries, whether or not we
> ever get to a grammar-based solution.

Great!

Cheers,
David.
-- 
David Fetter <david@fetter.org> http://fetter.org/
Phone: +1 415 235 3778  AIM: dfetter666  Yahoo!: dfetter
Skype: davidfetter      XMPP: david.fetter@gmail.com

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate

David Fetter <david@fetter.org> writes:
> On Thu, Oct 22, 2015 at 04:05:06PM -0700, Tom Lane wrote:
>> Given the license issues around GNU readline, requiring it seems like
>> probably a non-starter.

> I should have made this more clear.  I am not an IP attorney and don't
> play one on TV, but this is what I've gotten out of conversations with
> IP attorneys on this subject:

I'm not an IP attorney either, and the ones I've talked to seem to agree
with you.  (Red Hat's corporate attorneys, at least, certainly thought
that when I last asked them.)  But the observed facts are that some
distros refuse to ship psql-linked-to-readline, and substitute libedit
instead, because they got different advice from their attorneys.

If we desupport libedit, the end result is going to be these distros
shipping psql with no command-line-editing support at all.  Our opinion,
or even our lawyers' opinion, is unlikely to prevent that outcome.

While libedit certainly has got its bugs and deficiencies, it's way better
than nothing at all, so I think we'd better keep supporting it.
        regards, tom lane

On 2015-10-22 16:26:10 -0700, David Fetter wrote:
> To be affective negatively by libreadline's viral license, an entity
> would need to fork the psql client in proprietary ways that they did
> not wish not to make available to end users, at the same time linking
> in libreadline.

> Maybe I'm missing something big, but I really don't see people out
> there shipping a libreadline-enabled psql client, details of whose
> source they'd want to keep a deep, dark secret.

Isn't that just about every proprietary fork of postgres? Most have
added backend features and I guess many of those have in turn added
support to psql for those features.  Sure it'd probably in reality be
relatively harmless for them to release these psql modifications, but I
rather doubt their management will generally see it that way.

Greetings,

Andres Freund

On Thu, Oct 22, 2015 at 8:09 PM, Andres Freund <andres@anarazel.de> wrote:
> On 2015-10-22 16:26:10 -0700, David Fetter wrote:
>> To be affective negatively by libreadline's viral license, an entity
>> would need to fork the psql client in proprietary ways that they did
>> not wish not to make available to end users, at the same time linking
>> in libreadline.
>
>> Maybe I'm missing something big, but I really don't see people out
>> there shipping a libreadline-enabled psql client, details of whose
>> source they'd want to keep a deep, dark secret.
>
> Isn't that just about every proprietary fork of postgres? Most have
> added backend features and I guess many of those have in turn added
> support to psql for those features.  Sure it'd probably in reality be
> relatively harmless for them to release these psql modifications, but I
> rather doubt their management will generally see it that way.

Yeah, exactly.  EnterpriseDB have to keep libedit working even if the
PostgreSQL community dropped support, so I hope we don't decide to do
that.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

* Tom Lane (tgl@sss.pgh.pa.us) wrote:
> David Fetter <david@fetter.org> writes:
> > On Thu, Oct 22, 2015 at 04:05:06PM -0700, Tom Lane wrote:
> >> Given the license issues around GNU readline, requiring it seems like
> >> probably a non-starter.
>
> > I should have made this more clear.  I am not an IP attorney and don't
> > play one on TV, but this is what I've gotten out of conversations with
> > IP attorneys on this subject:
>
> I'm not an IP attorney either, and the ones I've talked to seem to agree
> with you.  (Red Hat's corporate attorneys, at least, certainly thought
> that when I last asked them.)  But the observed facts are that some
> distros refuse to ship psql-linked-to-readline, and substitute libedit
> instead, because they got different advice from their attorneys.

The issue for Debian, at least, isn't really about libreadline or
libedit, it's about the GPL and the OpenSSL license.  From the Debian
perspective, if we were able to build with GNUTLS or another SSL library
other than OpenSSL (which I know we've made some progress on, thanks
Heikki...) then Debian wouldn't have any problem shipping PG linked with
libreadline.

> If we desupport libedit, the end result is going to be these distros
> shipping psql with no command-line-editing support at all.  Our opinion,
> or even our lawyers' opinion, is unlikely to prevent that outcome.
>
> While libedit certainly has got its bugs and deficiencies, it's way better
> than nothing at all, so I think we'd better keep supporting it.

I agree that it's probably not a good idea to desupport libedit.
However, I don't believe supporting libedit necessairly prevents us
from providing better support when libreadline is available.
Debian-based distributions won't be able to reap the benefits of that
better support until we remove the OpenSSL dependency, which is
unfortunate, but other distributions would be able to.

Also, if the PG project is agreeable to it, there's no reason why we
couldn't provide full libreadline-enabled builds off of
apt.postgresql.org, regardless of what Debian wants.  That would add a
bit of extra burden on our package maintainers, as there isn't any
difference between packages built for Debian and packages built for
apt.p.o currently, I don't believe, but it seems like it'd be a pretty
minor change that would be well worth it if we get better libreadline
integration.

The one bit of all of this that worries me is that we would take
libreadline for granted and the libedit support would end up broken.
That seems a relatively minor concern though, as it sounds like there is
a fair bit of exercise of the libedit path due to the commercial forks
of PG which make use of it to avoid the GPL.

Thanks!

Stephen

Stephen Frost wrote:

> The issue for Debian, at least, isn't really about libreadline or
> libedit, it's about the GPL and the OpenSSL license.  From the Debian
> perspective, if we were able to build with GNUTLS or another SSL library
> other than OpenSSL (which I know we've made some progress on, thanks
> Heikki...) then Debian wouldn't have any problem shipping PG linked with
> libreadline.

What if OpenSSL were to switch to APL2?
http://www.postgresql.org/message-id/20150801151410.GA28344@awork2.anarazel.de


-- 
Álvaro Herrera                http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

* Alvaro Herrera (alvherre@2ndquadrant.com) wrote:
> Stephen Frost wrote:
>
> > The issue for Debian, at least, isn't really about libreadline or
> > libedit, it's about the GPL and the OpenSSL license.  From the Debian
> > perspective, if we were able to build with GNUTLS or another SSL library
> > other than OpenSSL (which I know we've made some progress on, thanks
> > Heikki...) then Debian wouldn't have any problem shipping PG linked with
> > libreadline.
>
> What if OpenSSL were to switch to APL2?
> http://www.postgresql.org/message-id/20150801151410.GA28344@awork2.anarazel.de

According to that post, it would depend what libreadline is licensed
under.  There may be other complications with GPL3 and other libraries
we link against, though I can't think of any offhand and perhaps there
aren't any.

Thanks!

Stephen

On Fri, Oct 23, 2015 at 02:09:43AM +0200, Andres Freund wrote:
> On 2015-10-22 16:26:10 -0700, David Fetter wrote:
> > To be affective negatively by libreadline's viral license, an entity
> > would need to fork the psql client in proprietary ways that they did
> > not wish not to make available to end users, at the same time linking
> > in libreadline.
> 
> > Maybe I'm missing something big, but I really don't see people out
> > there shipping a libreadline-enabled psql client, details of whose
> > source they'd want to keep a deep, dark secret.
> 
> Isn't that just about every proprietary fork of postgres?

Proprietary versions of the psql client?  I'm not sure I understand.

Proprietary, secret changes to the back end, sure, but the client?
The most recent example I recall of that is Netezza, and I suspect
that they just couldn't be bothered to publish the changes they made.
At that time, the community psql client was not by any means as nice
as it is now, so it's conceivable that they made substantive
improvements, at least for talking to Netezza DBs.

> Most have added backend features and I guess many of those have in
> turn added support to psql for those features.  Sure it'd probably
> in reality be relatively harmless for them to release these psql
> modifications, but I rather doubt their management will generally
> see it that way.

Is it really on us as a community to go long distances out of our way
to assuage the baseless[1] paranoia of people who are by and large not
part of our community?

As to our "support" of libedit, feel free to try working with a psql
client that has libedit instead of libreadline for a few weeks.  It
would be a very long stretch, at least for the work flows I've seen so
far, to call that client functional.  The strongest praise I can come
up with is that it's usually not quite as awful as working with a psql
client entirely devoid of command line editing support.

Of course, we can continue to pretend that there is a viable
alternative to libreadline if that soothes down some feathers, but I
don't see any reason to make substantive sacrifices in service of
keeping that particular box checked.

Cheers,
David.

[1] as far as I know, and I'll take this back in its entirety if
someone shows me a reasonable basis
-- 
David Fetter <david@fetter.org> http://fetter.org/
Phone: +1 415 235 3778  AIM: dfetter666  Yahoo!: dfetter
Skype: davidfetter      XMPP: david.fetter@gmail.com

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate

On Fri, Oct 23, 2015 at 11:16 AM, David Fetter <david@fetter.org> wrote:
> Proprietary, secret changes to the back end, sure, but the client?
> The most recent example I recall of that is Netezza, and I suspect
> that they just couldn't be bothered to publish the changes they made.
> At that time, the community psql client was not by any means as nice
> as it is now, so it's conceivable that they made substantive
> improvements, at least for talking to Netezza DBs.
>
>> Most have added backend features and I guess many of those have in
>> turn added support to psql for those features.  Sure it'd probably
>> in reality be relatively harmless for them to release these psql
>> modifications, but I rather doubt their management will generally
>> see it that way.
>
> Is it really on us as a community to go long distances out of our way
> to assuage the baseless[1] paranoia of people who are by and large not
> part of our community?

I was under the impression that I was part of this community, and I
have already said that my employer has added tab completion support,
and other psql features, related to the server features we have added.

Also, your statement that this is a long distance out of our way does
not seem to be justified by the facts.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Fri, Oct 23, 2015 at 12:15:01PM -0400, Robert Haas wrote:
> On Fri, Oct 23, 2015 at 11:16 AM, David Fetter <david@fetter.org> wrote:
> > Proprietary, secret changes to the back end, sure, but the client?
> > The most recent example I recall of that is Netezza, and I suspect
> > that they just couldn't be bothered to publish the changes they
> > made.  At that time, the community psql client was not by any
> > means as nice as it is now, so it's conceivable that they made
> > substantive improvements, at least for talking to Netezza DBs.
> >
> >> Most have added backend features and I guess many of those have
> >> in turn added support to psql for those features.  Sure it'd
> >> probably in reality be relatively harmless for them to release
> >> these psql modifications, but I rather doubt their management
> >> will generally see it that way.
> >
> > Is it really on us as a community to go long distances out of our
> > way to assuage the baseless[1] paranoia of people who are by and
> > large not part of our community?
> 
> I was under the impression that I was part of this community, and I
> have already said that my employer has added tab completion support,
> and other psql features, related to the server features we have
> added.
> 
> Also, your statement that this is a long distance out of our way
> does not seem to be justified by the facts.

As I wrote in the part you cut, we can continue to pretend libedit is
a viable alternative if it keeps some feathers unruffled.  If libedit
"support" gets to be a real drag, we can and should reconsider with a
strong bias to dropping it.

Cheers,
David.

P.S.  With a little luck, our next license or other perceived legal
conflict will be as easy to sort out as this one.  I have the sinking
feeling that anything we do with crypto or hashing more secure than
MD5 will run afoul of someone's idea of what the law is in their
country, and the impacts will be a lot nastier than this.  We will
need to deal with that separately, as needs arise.
-- 
David Fetter <david@fetter.org> http://fetter.org/
Phone: +1 415 235 3778  AIM: dfetter666  Yahoo!: dfetter
Skype: davidfetter      XMPP: david.fetter@gmail.com

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate

David Fetter <david@fetter.org> writes:
> Is it really on us as a community to go long distances out of our way
> to assuage the baseless[1] paranoia of people who are by and large not
> part of our community?

While I personally don't care that much about the proprietary-psql-variant
scenario, I do care whether people using Debian packages will have a good
experience with psql.

And frankly, sir, you do not have the standing to call their fears
baseless.  You can believe that they are, and you might even be right,
but your opinion on such matters does not matter to them.
        regards, tom lane

Jeff Janes wrote:

> For the bigger picture, I don't think we should not apply this patch simply
> because there is something even better we might theoretically do at some
> point in the future.

Agreed.

> Having used it a little bit, I do agree with Robert
> that it is not a gigantic improvement over the current situation, as the
> code it replaces is largely mechanical boilerplate.  But I think it is
> enough of an improvement that we should go ahead with it.

To me this patch sounds much like 2eafcf68d563df8a1db80a.  You could say
that what was replaced was "largely mechanical", but it was so much
easier to make mistakes with the original coding that it's not funny.

-- 
Álvaro Herrera                http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On Sat, Oct 24, 2015 at 6:19 AM, Jeff Janes <jeff.janes@gmail.com> wrote:
> On Sun, Oct 18, 2015 at 9:12 PM, Thomas Munro
> <thomas.munro@enterprisedb.com> wrote:
>> Thanks for taking a look at this!  The word count returned by
>> get_previous_words was incorrect.  Here is a corrected version.
>
> I haven't looked at v6 yet, but in v5:
>
> "set work_mem TO" completes to "NULL" not to "DEFAULT"
>
> line 2665 of the patched tab complete file,, should be "DEFAULT", not "NULL"
> as the completion string.  Looks like a simple copy and paste error.

Indeed.  Thanks.  Fixed in the attached.

--
Thomas Munro
http://www.enterprisedb.com

On Thu, Oct 22, 2015 at 10:36 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> What I would like is to find a way to auto-generate basically this entire
> file from gram.y.  That would imply going over to something at least
> somewhat parser-based, instead of the current way that is more or less
> totally ad-hoc.  That would be a very good thing though, because the
> current way gives wrong answers not-infrequently, even discounting cases
> that it's simply not been taught about.

I always assumed the reason we didn't use the bison grammar table to
generate completions was because the grammar is way too general and
there would be way too many spurious completions that in practice
nobody would ever be interested in. I assumed it was an intentional
choice that it was more helpful to complete things we know people
usually want rather than every theoretically possible next token.

If that's not true then maybe I'll poke at this sometime. But I agree
with the other part of this thread that that would be totally
experimental and even if we had a working patch it would be a long
time before the user experience was up to the same level as the
current behaviour. I suspect it would involve sending the partial
query to the server for parsing and asking for feedback on completions
using the grammar parser table and the search_path object resolution
rules in effect.


-- 
greg

Hello. How  about regular expressions?

I've been thinking of better mechanism for tab-compltion for
these days since I found some bugs in it.

At Fri, 23 Oct 2015 14:50:58 -0300, Alvaro Herrera <alvherre@2ndquadrant.com> wrote in
<20151023175058.GA3391@alvherre.pgsql>
> Jeff Janes wrote:
> 
> > For the bigger picture, I don't think we should not apply this patch simply
> > because there is something even better we might theoretically do at some
> > point in the future.
> 
> Agreed.

Auto-generating from grammer should be the ultimate solution but
I don't think it will be available. But still I found that the
word-splitting-then-match-word-by-word-for-each-matching is
terriblly unmaintainable and poorly capable. So, how about
regular expressions?

I tried to use pg_regex in frontend and found that it is easily
doable. As a proof of the concept, the two patches attached to
this message does that changes.

1. 0001-Allow-regex-module-to-be-used-outside-server.patch
 This small change makes pg_regex possible to be used in frontend.

2. 0002-Replace-previous-matching-rule-with-regexps.patch
 Simply replaces existing matching rules almost one-by-one with regular expression matches.

I made these patches not to change the behavior except inevitable
ones.

We would have far powerful matching capability using regular
expressions and it makes tab-complete.c look simpler. On the
other hand, regular expressions - which are stashed away into new
file by this patch - is a chunk of complexity and (also) error
prone. For all that I think this is better than the current
situation in terms of maintainability and capability.

This should look stupid because it really be replaced stupidly
and of course this can be more sane/effective/maintainable by
refactoring. But before that issue, I'm not confident at all that
this is really a alternative with *gigantic* improvement.

Any opinions?

> > Having used it a little bit, I do agree with Robert
> > that it is not a gigantic improvement over the current situation, as the
> > code it replaces is largely mechanical boilerplate.  But I think it is
> > enough of an improvement that we should go ahead with it.
> 
> To me this patch sounds much like 2eafcf68d563df8a1db80a.  You could say
> that what was replaced was "largely mechanical", but it was so much
> easier to make mistakes with the original coding that it's not funny.

regards,

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

On Thu, Nov 12, 2015 at 1:16 PM, Kyotaro HORIGUCHI wrote:
> 1. 0001-Allow-regex-module-to-be-used-outside-server.patch
>
>   This small change makes pg_regex possible to be used in
>   frontend.

This is generic enough to live in src/common, then psql would directly
reuse it using lpgcommon.

> 2. 0002-Replace-previous-matching-rule-with-regexps.patch
>
>   Simply replaces existing matching rules almost one-by-one with
>   regular expression matches.

This makes the situation messier. At least with Thomas' patch one can
immediately know the list of words that are being matched for a given
code path while with this patch we need to have a look to the regex
where they are list. And this would get more and more complicated with
new commands added.
-- 
Michael

Kyotaro HORIGUCHI wrote:

> Auto-generating from grammer should be the ultimate solution but
> I don't think it will be available. But still I found that the
> word-splitting-then-match-word-by-word-for-each-matching is
> terriblly unmaintainable and poorly capable. So, how about
> regular expressions?

I don't think this is an improvement.  It seems to me that a lot more
work is required to maintain these regular expressions, which while
straightforward are not entirely trivial (harder to read).

If we could come up with a reasonable format that is pre-processed to a
regexp, that might be better.  I think Thomas' proposed patch is closer
to that than Horiguchi-san's.

-- 
Álvaro Herrera                http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Thomas Munro wrote:
> New version attached, merging recent changes.

I wonder about the TailMatches and Matches macros --- wouldn't it be
better to have a single one, renaming TailMatches to Matches and
replacing the current Matches() with an initial token that corresponds
to anchoring to start of command?  Just wondering, not terribly attached
to the idea.

-- 
Álvaro Herrera                http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Hello, thank you for many valuable opinions.

I am convinced that bare regular expressions cannot be applied
here:)

At Mon, 16 Nov 2015 18:59:06 +1300, Thomas Munro <thomas.munro@enterprisedb.com> wrote in
<CAEepm=26AR3drcAUW+paLvJmXS6WV36kk6e72GWS4rAykOueOg@mail.gmail.com>
> It's an interesting idea to use regular expressions, but it's a shame to
> move the patterns so far away from the actions they trigger.

Yes, I agree. RE is powerful tool but too complicated. Some
intermediate expression would be needed but it also adds
different kind of complexity.

At Mon, 16 Nov 2015 23:09:52 +0900, Michael Paquier <michael.paquier@gmail.com> wrote in
<CAB7nPqR+uQj=QvXDbookA5OmbmCR6So2fJxCLbh6gJ6OvacDLA@mail.gmail.com>
> This makes the situation messier. At least with Thomas' patch one can
> immediately know the list of words that are being matched for a given
> code path while with this patch we need to have a look to the regex
> where they are list. And this would get more and more complicated with
> new commands added.

Hmm, so I named the enum items to reflect its pattern but even
though I also think it is one of the problem of using regular
expressions.

At Mon, 16 Nov 2015 12:16:07 -0300, Alvaro Herrera <alvherre@2ndquadrant.com> wrote in
<20151116151606.GW614468@alvherre.pgsql>
> I don't think this is an improvement.  It seems to me that a lot more
> work is required to maintain these regular expressions, which while
> straightforward are not entirely trivial (harder to read).
> 
> If we could come up with a reasonable format that is pre-processed to a
> regexp, that might be better.  I think Thomas' proposed patch is closer
> to that than Horiguchi-san's.

I aimed that matching mechanism can handle optional elements in
syntexes more robustly. But as all you are saying, bare regular
expression is too complex here.



regares,

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

Hello,

At Mon, 16 Nov 2015 12:19:23 -0300, Alvaro Herrera <alvherre@2ndquadrant.com> wrote in
<20151116151923.GX614468@alvherre.pgsql>
> Thomas Munro wrote:
> > New version attached, merging recent changes.
> 
> I wonder about the TailMatches and Matches macros --- wouldn't it be
> better to have a single one, renaming TailMatches to Matches and
> replacing the current Matches() with an initial token that corresponds
> to anchoring to start of command?  Just wondering, not terribly attached
> to the idea.

Does it looks like this?

> if (Match(BOL, "ALTER", "TABLE", EOL)) ...

It would be doable giving special meaning to
word_matches(BOL/EOL, *_wd). And I give +1 to that than having so
many similar macros.




The following is my delusion..

It could develop to some mini-laguages like the following, which
is a kind of subset of regular expressions, that is powerful than
current mechanism but not meesier than regular expressions by
narrowing its functionarity. Addition to that the custom minilang
could have specilized functionarity for matching SQL statements.

> if (Match("^ALTER TABLE \id$"))...

It would be nice to have tokens to match to optional words.

> if (Match("^ALTER TABLE(IF EXISTS) \id$"))...
> if (Match("CREATE(OR REPLACE)FUNCTION \id (\CSL)$")

Mmm. this might be another kind of complexity?  This is also
accomplished by multiple matching descriptions.

> if (Match("^,ALTER,TABLE,\id,$") ||
>     Match("^,ALTER,TABLE,IF,EXISTS,\id,$"))...

Interpreting this kind of mini-language into regular expressions
could be doable..


regards,

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

Hello, I tried to implement the mini-language, which is a
simplified reglar expression for this specific use.

As a ultra-POC, the attached patch has very ad-hoc preprocess
function and does on-the-fly preprocessing, compilation then
execution of regular expression. And it is applied to only the
first ten or so matchings in psql_completion().

The first attachment is the same with that of previous patchset.

Every matching line looks like the following,

> else if (RM("ALTER {AGGREGATE|FUNCTION} [#id](.."))
>     COMPLETE_WITH_FUNCTION_ARG(CAPTURE(1));

As a temporary desin, "{}" means grouping, "|" means alternatives,
"[]" means capture and '#id' means any identifier.

The largest problem of this would be its computation cost:( This
in turn might be too slow if about three hundred matches run...

I see no straight-forward way to preprocess these strings.. A
possible solution would be extracting these strings then
auto-generate a c-srouce to preprocess them. And when running,
RM() retrieves the compiled regular expression using the string
as the key.


At Tue, 17 Nov 2015 15:35:43 +0900 (Tokyo Standard Time), Kyotaro HORIGUCHI <horiguchi.kyotaro@lab.ntt.co.jp> wrote in
<20151117.153543.183036803.horiguchi.kyotaro@lab.ntt.co.jp>
> At Mon, 16 Nov 2015 12:16:07 -0300, Alvaro Herrera <alvherre@2ndquadrant.com> wrote in
<20151116151606.GW614468@alvherre.pgsql>
> > I don't think this is an improvement.  It seems to me that a lot more
> > work is required to maintain these regular expressions, which while
> > straightforward are not entirely trivial (harder to read).
> > 
> > If we could come up with a reasonable format that is pre-processed to a
> > regexp, that might be better.  I think Thomas' proposed patch is closer
> > to that than Horiguchi-san's.
> 
> I aimed that matching mechanism can handle optional elements in
> syntexes more robustly. But as all you are saying, bare regular
> expression is too complex here.

At Tue, 17 Nov 2015 16:09:25 +0900 (Tokyo Standard Time), Kyotaro HORIGUCHI <horiguchi.kyotaro@lab.ntt.co.jp> wrote in
<20151117.160925.45883793.horiguchi.kyotaro@lab.ntt.co.jp>
> if (Match("^,ALTER,TABLE,\id,$") ||
>     Match("^,ALTER,TABLE,IF,EXISTS,\id,$"))...
> 
> Interpreting this kind of mini-language into regular expressions
> could be doable..

regards,

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

Hello, the attached are a patchset to introduce a special
matching expression which simplifies matching descriptions. The
result looks as if the comments in the previous implement run
as-is.

0001-Allow-regex-module-to-be-used-outside-server.patch
 A small patch to allow pg_regex to be used outside backend. The same with the previous one.

0002-Simplify-the-usages-of-COMPLETE_WITH_QUERY.patch
 This is also the same with the previous one.

0003-Replace-previous-matching-rule-with-regexps-take-2.patch
 Modifies to use this matching minilang. No bare regular expression is seen for every matching. The difference from the
previousone is that sources for other than regcomp.o are no longer symlinked to psql's directory. Compiled regular
expressionsare cached until psql ends. The function patcomp is the central of the messy of this patchset.
 
 This cannot be compiled on Windows, I think.

0004-Remove-less-informative-comments.patch
 Remove comments that no longer has reason to be placed there.

0005-Merge-mergable-completions.patch
 Merge some of the matchings that are simply mergable by using this minilang.

What do you think about this solution?


At Tue, 17 Nov 2015 19:25:24 +0900 (Tokyo Standard Time), Kyotaro HORIGUCHI <horiguchi.kyotaro@lab.ntt.co.jp> wrote in
<20151117.192524.95155716.horiguchi.kyotaro@lab.ntt.co.jp>
> Hello, I tried to implement the mini-language, which is a
> simplified reglar expression for this specific use.
> 
> As a ultra-POC, the attached patch has very ad-hoc preprocess
> function and does on-the-fly preprocessing, compilation then
> execution of regular expression. And it is applied to only the
> first ten or so matchings in psql_completion().
> 
> The first attachment is the same with that of previous patchset.
> 
> Every matching line looks like the following,
> 
> > else if (RM("ALTER {AGGREGATE|FUNCTION} [#id](.."))
> >     COMPLETE_WITH_FUNCTION_ARG(CAPTURE(1));

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

On Thu, Nov 26, 2015 at 2:45 PM, Kyotaro HORIGUCHI
<horiguchi.kyotaro@lab.ntt.co.jp> wrote:
> What do you think about this solution?

<please do not top-post, it breaks the logic of the thread>

This patch fails to compile on OSX:
Undefined symbols for architecture x86_64: "_ExceptionalCondition", referenced from:     _pg_regexec in regexec.o
_cfindin regexec.o     _find in regexec.o     _newdfa in regexec.o     _cfindloop in regexec.o     _shortest in
regexec.o    _cdissect in regexec.o     ...
 
So, to begin with, this may be better if replugged as a standalone
library, aka moving the regexp code into src/common for example or
similar. Also, per the comments on top of rcancelrequested,
rstacktoodeep and rcancelrequested, returning unconditionally 0 is not
a good idea for -DFRONTEND. Callbacks should be defined and made
available for callers.

-       {"EVENT TRIGGER", NULL, NULL},
+       {"EVENT TRIGGER", Query_for_list_of_event_triggers, NULL},       {"EXTENSION", Query_for_list_of_extensions},
-       {"FOREIGN DATA WRAPPER", NULL, NULL},
+       {"FOREIGN DATA WRAPPER", Query_for_list_of_fdws, NULL},       {"FOREIGN TABLE", NULL, NULL},       {"FUNCTION",
NULL,&Query_for_list_of_functions},       {"GROUP", Query_for_list_of_roles},       {"LANGUAGE",
Query_for_list_of_languages},      {"INDEX", NULL, &Query_for_list_of_indexes},
 
-       {"MATERIALIZED VIEW", NULL, NULL},
+       {"MATERIALIZED VIEW", NULL, &Query_for_list_of_matviews},
This has value as a separate patch.

The patch has many whitespaces, and unrelated diffs.
-- 
Michael

Thank you for looking on this and the comment.

At Mon, 7 Dec 2015 15:00:32 +0900, Michael Paquier <michael.paquier@gmail.com> wrote in
<CAB7nPqQ69EPZkOqHAVuZXPxNzb_c2R5hs88tedLYqU9=Zu6xOw@mail.gmail.com>
> On Thu, Nov 26, 2015 at 2:45 PM, Kyotaro HORIGUCHI
> <horiguchi.kyotaro@lab.ntt.co.jp> wrote:
> > What do you think about this solution?
> 
> <please do not top-post, it breaks the logic of the thread>

I believe I haven't ripped off any in CC: list or In-Reply-To and
References in the previous post. (I read "top-post" as a post
without these headers.)  Could you let me know how the message
was broken?

> This patch fails to compile on OSX:
> Undefined symbols for architecture x86_64:
>   "_ExceptionalCondition", referenced from:
>       _pg_regexec in regexec.o
>       _cfind in regexec.o
>       _find in regexec.o
>       _newdfa in regexec.o
>       _cfindloop in regexec.o
>       _shortest in regexec.o
>       _cdissect in regexec.o
>       ...
> So, to begin with, this may be better if replugged as a standalone
> library, aka moving the regexp code into src/common for example or
> similar. 

I agree to that. I'll consider doing so. (But my middle finger
tip injury makes me further slower than usual..)

> Also, per the comments on top of rcancelrequested,
> rstacktoodeep and rcancelrequested, returning unconditionally 0 is not
> a good idea for -DFRONTEND. Callbacks should be defined and made
> available for callers.

cancel_pressed is usable for the purpose and I'll add
cancel_callback feeature to separate it from both frontend and
backend.

> -       {"EVENT TRIGGER", NULL, NULL},
> +       {"EVENT TRIGGER", Query_for_list_of_event_triggers, NULL},
>         {"EXTENSION", Query_for_list_of_extensions},
> -       {"FOREIGN DATA WRAPPER", NULL, NULL},
> +       {"FOREIGN DATA WRAPPER", Query_for_list_of_fdws, NULL},
>         {"FOREIGN TABLE", NULL, NULL},
>         {"FUNCTION", NULL, &Query_for_list_of_functions},
>         {"GROUP", Query_for_list_of_roles},
>         {"LANGUAGE", Query_for_list_of_languages},
>         {"INDEX", NULL, &Query_for_list_of_indexes},
> -       {"MATERIALIZED VIEW", NULL, NULL},
> +       {"MATERIALIZED VIEW", NULL, &Query_for_list_of_matviews},
> This has value as a separate patch.

I carelessly merged it in the fourth (Merge mergable...)
patch. I'll separate it.


> The patch has many whitespaces, and unrelated diffs.

Mmm, thanks for pointing it out. I haven't see such lines differ
only in whitespaces or found unrelated diffs so far but I'll
check it out.

regards,

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

On Tue, Dec 8, 2015 at 6:31 PM, Kyotaro HORIGUCHI
<horiguchi.kyotaro@lab.ntt.co.jp> wrote:
>
> Thank you for looking on this and the comment.
>
> At Mon, 7 Dec 2015 15:00:32 +0900, Michael Paquier <michael.paquier@gmail.com> wrote in
<CAB7nPqQ69EPZkOqHAVuZXPxNzb_c2R5hs88tedLYqU9=Zu6xOw@mail.gmail.com>
> > On Thu, Nov 26, 2015 at 2:45 PM, Kyotaro HORIGUCHI
> > <horiguchi.kyotaro@lab.ntt.co.jp> wrote:
> > > What do you think about this solution?
> >
> > <please do not top-post, it breaks the logic of the thread>
>
> I believe I haven't ripped off any in CC: list or In-Reply-To and
> References in the previous post. (I read "top-post" as a post
> without these headers.)  Could you let me know how the message
> was broken?

20151126.144512.10228250.horiguchi.kyotaro@lab.ntt.co.jp just did that
when you sent this new patch series:
A: Because it reverses the logical flow of conversation.
Q: Why is top posting frowned upon?
https://www.freebsd.org/doc/en/articles/mailing-list-faq/etiquette.html#idp55416272
-- 
Michael

Hello,

At Tue, 8 Dec 2015 20:50:39 +0900, Michael Paquier <michael.paquier@gmail.com> wrote in
<CAB7nPqR7UkYAhiFu=-W6M30Ku-9zSFPQerT1LDLwktyAXzLBQA@mail.gmail.com>
> On Tue, Dec 8, 2015 at 6:31 PM, Kyotaro HORIGUCHI
> <horiguchi.kyotaro@lab.ntt.co.jp> wrote:
> > > <please do not top-post, it breaks the logic of the thread>
> >
> > I believe I haven't ripped off any in CC: list or In-Reply-To and
> > References in the previous post. (I read "top-post" as a post
> > without these headers.)  Could you let me know how the message
> > was broken?
> 
> 20151126.144512.10228250.horiguchi.kyotaro@lab.ntt.co.jp just did that
> when you sent this new patch series:
> A: Because it reverses the logical flow of conversation.
> Q: Why is top posting frowned upon?
> https://www.freebsd.org/doc/en/articles/mailing-list-faq/etiquette.html#idp55416272

Thank you for the reference. I made it because I didn't regard it
as a reply to (my) previous message but an update. I'll take care
to avoid top-posting in any case, though.

regards,

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

Hello, please find the attached revised patches.

At Tue, 08 Dec 2015 18:31:10 +0900 (Tokyo Standard Time), Kyotaro HORIGUCHI <horiguchi.kyotaro@lab.ntt.co.jp> wrote in
<20151208.183110.229901672.horiguchi.kyotaro@lab.ntt.co.jp>
> > This patch fails to compile on OSX:
> > Undefined symbols for architecture x86_64:
> >   "_ExceptionalCondition", referenced from:
> >       _pg_regexec in regexec.o
> >       ...

I have fixed it.

> > So, to begin with, this may be better if replugged as a standalone
> > library, aka moving the regexp code into src/common for example or
> > similar. 
> 
> I agree to that. I'll consider doing so. (But my middle finger
> tip injury makes me further slower than usual..)

Done. They are moved into common/regex.

.../backend/utils/mb/wstrncase.o still remains in psql/Makefile
but moving it would makes it more odd so it is left as it is.

> > Also, per the comments on top of rcancelrequested,
> > rstacktoodeep and rcancelrequested, returning unconditionally 0 is not
> > a good idea for -DFRONTEND. Callbacks should be defined and made
> > available for callers.

Done. pg_regcomp now has additinal parameter, which can be NULL
on backend.

> cancel_pressed is usable for the purpose and I'll add
> cancel_callback feeature to separate it from both frontend and
> backend.
> 
> > -       {"EVENT TRIGGER", NULL, NULL},
...
> > -       {"MATERIALIZED VIEW", NULL, NULL},
> > +       {"MATERIALIZED VIEW", NULL, &Query_for_list_of_matviews},
> > This has value as a separate patch.
> 
> I carelessly merged it in the fourth (Merge mergable...)
> patch. I'll separate it.

This is not added in this patchset. I'll post it later.

> > The patch has many whitespaces, and unrelated diffs.
> 
> Mmm, thanks for pointing it out. I haven't see such lines differ
> only in whitespaces or found unrelated diffs so far but I'll
> check it out.

I think they are whitespaces in existing file and they Some lines in
header comments have trailing space and I can remove them not
only for files I moved but for all files. But it should be as
another patch. Is it worth doing?

I found a bug in matching for "DELETE" so fixed it.


regards,

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

On Mon, Dec 7, 2015 at 8:41 PM, Michael Paquier
<michael.paquier@gmail.com> wrote:
> On Tue, Nov 17, 2015 at 12:19 AM, Alvaro Herrera <alvherre@2ndquadrant.com>
> wrote:
>> Thomas Munro wrote:
>>> New version attached, merging recent changes.
>>
>> I wonder about the TailMatches and Matches macros --- wouldn't it be
>> better to have a single one, renaming TailMatches to Matches and
>> replacing the current Matches() with an initial token that corresponds
>> to anchoring to start of command?  Just wondering, not terribly attached
>> to the idea.
>
> +       /* TODO:TM -- begin temporary, not part of the patch! */
> +       Assert(!word_matches(NULL, ""));
> + [...]
> +       Assert(!word_matches("foo", ""));
> +       /* TODO:TM -- end temporary */
>
> Be sure to not forget to remove that later.

Thanks for looking at this Michael.  It's probably not much fun to
review!  Here is a new version with that bit removed.  More responses
inline below.

> -       else if (pg_strcasecmp(prev5_wd, "DEFAULT") == 0 &&
> -                        pg_strcasecmp(prev4_wd, "PRIVILEGES") == 0 &&
> -                        (pg_strcasecmp(prev3_wd, "FOR") == 0 ||
> -                         pg_strcasecmp(prev3_wd, "IN") == 0))
> -       {
> -               static const char *const
> list_ALTER_DEFAULT_PRIVILEGES_REST[] =
> -               {"GRANT", "REVOKE", NULL};
> -
> -               COMPLETE_WITH_LIST(list_ALTER_DEFAULT_PRIVILEGES_REST);
> -       }
> +       else if (TailMatches5("DEFAULT", "PRIVILEGES", "FOR", "ROLE",
> MatchAny) ||
> +                        TailMatches5("DEFAULT", "PRIVILEGES", "IN",
> "SCHEMA", MatchAny))
> +               CompleteWithList2("GRANT", "REVOKE");
> For this chunk I think that you need to check for ROLE|USER and not only
> ROLE.

Right, done.

> +       else if (TailMatches4("ALTER", "DOMAIN", MatchAny, "RENAME"))
>         {
>                 static const char *const list_ALTERDOMAIN[] =
>                 {"CONSTRAINT", "TO", NULL};
> I think you should remove COMPLETE_WITH_LIST here for consistency with the
> rest.

Right, done.

> -       else if (pg_strcasecmp(prev5_wd, "DOMAIN") == 0 &&
> -                        pg_strcasecmp(prev3_wd, "RENAME") == 0 &&
> -                        pg_strcasecmp(prev2_wd, "CONSTRAINT") == 0)
> +       else if (TailMatches3("RENAME", "CONSTRAINT", MatchAny))
>                 COMPLETE_WITH_CONST("TO");
> Perhaps you are missing DOMAIN here?

Right, done.

> -       else if (pg_strcasecmp(prev4_wd, "ALTER") == 0 &&
> -                        pg_strcasecmp(prev3_wd, "SEQUENCE") == 0 &&
> -                        pg_strcasecmp(prev_wd, "NO") == 0)
> -       {
> -               static const char *const list_ALTERSEQUENCE2[] =
> -               {"MINVALUE", "MAXVALUE", "CYCLE", NULL};
> -
> -               COMPLETE_WITH_LIST(list_ALTERSEQUENCE2);
> -       }
> +       else if (TailMatches4("ALTER", "SEQUEMCE", MatchAny, "NO"))
> +               CompleteWithList3("MINVALUE", "MAXVALUE", "CYCLE");
> Typo here: s/SEQUEMCE/SEQUENCE.

Oops, fixed.

> -       else if (pg_strcasecmp(prev5_wd, "TABLE") == 0 &&
> -                        pg_strcasecmp(prev3_wd, "RENAME") == 0 &&
> -                        (pg_strcasecmp(prev2_wd, "COLUMN") == 0 ||
> -                         pg_strcasecmp(prev2_wd, "CONSTRAINT") == 0) &&
> -                        pg_strcasecmp(prev_wd, "TO") != 0)
> +       else if (TailMatches6("ALTER", "TABLE", MatchAny, "RENAME",
> "COLUMN|CONSTRAINT", MatchAny) &&
> +                        !TailMatches1("TO"))
> This should use TailMatches5 without ALTER for consistency with the existing
> code?

Ok, done.

> -       else if (pg_strcasecmp(prev_wd, "CLUSTER") == 0 &&
> -                        pg_strcasecmp(prev2_wd, "WITHOUT") != 0)
> +       else if (TailMatches1("CLUSTER") && !TailMatches2("WITHOUT",
> "CLUSTER"))
> Here removing CLUSTER should be fine.

Ok.

> -       else if (pg_strcasecmp(prev2_wd, "CLUSTER") == 0 &&
> -                        pg_strcasecmp(prev_wd, "ON") != 0 &&
> -                        pg_strcasecmp(prev_wd, "VERBOSE") != 0)
> -       {
> +       else if (TailMatches2("CLUSTER", MatchAny) &&
> !TailMatches1("VERBOSE"))
> Handling of ON has been forgotten.

Right, fixed.

> -       else if (pg_strcasecmp(prev3_wd, "CREATE") == 0 &&
> -                        !(pg_strcasecmp(prev2_wd, "USER") == 0 &&
> pg_strcasecmp(prev_wd, "MAPPING") == 0) &&
> -                        (pg_strcasecmp(prev2_wd, "ROLE") == 0 ||
> -                         pg_strcasecmp(prev2_wd, "GROUP") == 0 ||
> pg_strcasecmp(prev2_wd, "USER") == 0))
> +       else if (TailMatches3("CREATE", "ROLE|GROUP|USER", MatchAny) &&
> +                        !TailMatches3("CREATE", "USER", "MAPPING"))
> !TailMatches2("USER", "MAPPING")?

Ok.

> -       else if (pg_strcasecmp(prev4_wd, "CREATE") == 0 &&
> -                        pg_strcasecmp(prev3_wd, "MATERIALIZED") == 0 &&
> -                        pg_strcasecmp(prev2_wd, "VIEW") == 0)
> +       else if (TailMatches3("CREATE", "MATERIALIZED", "VIEW"))
> Forgot a MatchAny here?

Right, fixed.

> -       else if (pg_strcasecmp(prev_wd, "DELETE") == 0 &&
> -                        !(pg_strcasecmp(prev2_wd, "ON") == 0 ||
> -                          pg_strcasecmp(prev2_wd, "GRANT") == 0 ||
> -                          pg_strcasecmp(prev2_wd, "BEFORE") == 0 ||
> -                          pg_strcasecmp(prev2_wd, "AFTER") == 0))
> +       else if (TailMatches1("DELETE") &&
> !TailMatches2("ON|GRANT|BEFORE|AFTER", "DELETE"))
>                 COMPLETE_WITH_CONST("FROM");
> In the second clause checking for DELETE is not necessary.

Ok.

> -       else if (pg_strcasecmp(prev_wd, "EXECUTE") == 0 &&
> -                        prev2_wd[0] == '\0')
> +       else if (Matches1("EXECUTE"))
> This looks not complete.

I think it's correct.  The existing code has prev2_wd[0] == '\0' as a
way of checking that EXECUTE is the first word.  Matches1("EXECUTE")
does the same.

> +       else if (TailMatches1("EXPLAIN"))
> +               CompleteWithList7("SELECT", "INSERT", "DELETE", "UPDATE",
> "DECLARE",
> +                                                       "ANALYZE",
> "VERBOSE");
> +       else if (TailMatches2("EXPLAIN", "ANALYZE|ANALYSE"))
> ANALYSE should be removed, former code only checked for ANALYZE => I heard
> about the grammar issues here :)

Right.  Removed.

> -               else if (pg_strcasecmp(prev4_wd, "GRANT") == 0)
> +               else if (TailMatches4("GRANT", MatchAny, MatchAny,
> MatchAny))
> +                       COMPLETE_WITH_CONST("TO");
> +               else
> +                       COMPLETE_WITH_CONST("FROM");
> +       }
> +
> +       /* Complete "GRANT/REVOKE * ON * *" with TO/FROM */
> +       else if (TailMatches5("GRANT|REVOKE", MatchAny, "ON", MatchAny,
> MatchAny))
> +       {
> +               if (TailMatches5("GRANT", MatchAny, MatchAny, MatchAny,
> MatchAny))
> Isn't the first check with TailMatches4 enough here?

I don't think so: the first handles GRANT * ON * where the final word
isn't one of the known keywords that will be followed by an
appropriate list of objects (in other words when it was a table name).
The TailMatches5 case is for supplying TO or FROM after you write eg
GRANT * ON TABLE *.

> -               if (pg_strcasecmp(prev6_wd, "GRANT") == 0)
> +               if (TailMatches6("GRANT", MatchAny, MatchAny, MatchAny,
> MatchAny, MatchAny))
> HeadMatches1 perhaps?

I agree that would probably be better but Alvaro suggested following
the existing logic in the first pass, which was mostly based on tails,
and then considering simpler head-based patterns in a future pass.

Thanks!

--
Thomas Munro
http://www.enterprisedb.com

On Wed, Dec 9, 2015 at 8:17 PM, Thomas Munro
<thomas.munro@enterprisedb.com> wrote:
> On Mon, Dec 7, 2015 at 8:41 PM, Michael Paquier
> <michael.paquier@gmail.com> wrote:
>> On Tue, Nov 17, 2015 at 12:19 AM, Alvaro Herrera <alvherre@2ndquadrant.com>
>> wrote:
>>> Thomas Munro wrote:
>>>> New version attached, merging recent changes.
>>>
>>> I wonder about the TailMatches and Matches macros --- wouldn't it be
>>> better to have a single one, renaming TailMatches to Matches and
>>> replacing the current Matches() with an initial token that corresponds
>>> to anchoring to start of command?  Just wondering, not terribly attached
>>> to the idea.
>>
>> +       /* TODO:TM -- begin temporary, not part of the patch! */
>> +       Assert(!word_matches(NULL, ""));
>> + [...]
>> +       Assert(!word_matches("foo", ""));
>> +       /* TODO:TM -- end temporary */
>>
>> Be sure to not forget to remove that later.
>
> Thanks for looking at this Michael.  It's probably not much fun to
> review!  Here is a new version with that bit removed.  More responses
> inline below.

I had a hard time not sleeping when reading it... That's very mechanical.

> I agree that would probably be better but Alvaro suggested following
> the existing logic in the first pass, which was mostly based on tails,
> and then considering simpler head-based patterns in a future pass.

Fine with me.

So what do we do now? There is your patch, which is already quite big,
but as well a second patch based on regexps, which is far bigger. And
at the end they provide a similar result:

Here is for example what the regexp patch does for some complex
checks, like ALTER TABLE RENAME:    /* ALTER TABLE xxx RENAME yyy */
-    else if (pg_strcasecmp(prev4_wd, "TABLE") == 0 &&
-             pg_strcasecmp(prev2_wd, "RENAME") == 0 &&
-             pg_strcasecmp(prev_wd, "CONSTRAINT") != 0 &&
-             pg_strcasecmp(prev_wd, "TO") != 0)
+    else if (MATCH("TABLE #id RENAME !CONSTRAINT|TO"))

And what Thomas's patch does:
+    else if (TailMatches5("ALTER", "TABLE", MatchAny, "RENAME", MatchAny) &&
+             !TailMatches1("CONSTRAINT|TO"))

The regexp patch makes the negative checks somewhat easier to read
(there are 19 positions in tab-complete.c doing that), still inventing
a new langage and having a heavy refactoring just tab completion of
psql seems a bit too much IMO, so my heart balances in favor of
Thomas' stuff. Thoughts from others?
-- 
Michael

On Wed, Dec 09, 2015 at 08:49:22PM +0900, Michael Paquier wrote:
> On Wed, Dec 9, 2015 at 8:17 PM, Thomas Munro
> <thomas.munro@enterprisedb.com> wrote:
> > On Mon, Dec 7, 2015 at 8:41 PM, Michael Paquier
> > <michael.paquier@gmail.com> wrote:
> >> On Tue, Nov 17, 2015 at 12:19 AM, Alvaro Herrera <alvherre@2ndquadrant.com>
> >> wrote:
> >>> Thomas Munro wrote:
> >>>> New version attached, merging recent changes.
> >>>
> >>> I wonder about the TailMatches and Matches macros --- wouldn't it be
> >>> better to have a single one, renaming TailMatches to Matches and
> >>> replacing the current Matches() with an initial token that corresponds
> >>> to anchoring to start of command?  Just wondering, not terribly attached
> >>> to the idea.
> >>
> >> +       /* TODO:TM -- begin temporary, not part of the patch! */
> >> +       Assert(!word_matches(NULL, ""));
> >> + [...]
> >> +       Assert(!word_matches("foo", ""));
> >> +       /* TODO:TM -- end temporary */
> >>
> >> Be sure to not forget to remove that later.
> >
> > Thanks for looking at this Michael.  It's probably not much fun to
> > review!  Here is a new version with that bit removed.  More responses
> > inline below.
> 
> I had a hard time not sleeping when reading it... That's very mechanical.
> 
> > I agree that would probably be better but Alvaro suggested following
> > the existing logic in the first pass, which was mostly based on tails,
> > and then considering simpler head-based patterns in a future pass.
> 
> Fine with me.
> 
> So what do we do now? There is your patch, which is already quite big,
> but as well a second patch based on regexps, which is far bigger. And
> at the end they provide a similar result:
> 
> Here is for example what the regexp patch does for some complex
> checks, like ALTER TABLE RENAME:
>      /* ALTER TABLE xxx RENAME yyy */
> -    else if (pg_strcasecmp(prev4_wd, "TABLE") == 0 &&
> -             pg_strcasecmp(prev2_wd, "RENAME") == 0 &&
> -             pg_strcasecmp(prev_wd, "CONSTRAINT") != 0 &&
> -             pg_strcasecmp(prev_wd, "TO") != 0)
> +    else if (MATCH("TABLE #id RENAME !CONSTRAINT|TO"))
> 
> And what Thomas's patch does:
> +    else if (TailMatches5("ALTER", "TABLE", MatchAny, "RENAME", MatchAny) &&
> +             !TailMatches1("CONSTRAINT|TO"))
> 
> The regexp patch makes the negative checks somewhat easier to read
> (there are 19 positions in tab-complete.c doing that), still inventing
> a new langage and having a heavy refactoring just tab completion of
> psql seems a bit too much IMO, so my heart balances in favor of
> Thomas' stuff. Thoughts from others?

Agreed that the "whole new language" aspect seems like way too big a
hammer, given what it actually does.

Cheers,
David.
-- 
David Fetter <david@fetter.org> http://fetter.org/
Phone: +1 415 235 3778  AIM: dfetter666  Yahoo!: dfetter
Skype: davidfetter      XMPP: david.fetter@gmail.com

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate

On Wed, Dec 9, 2015 at 2:27 PM, David Fetter <david@fetter.org> wrote:
> Agreed that the "whole new language" aspect seems like way too big a
> hammer, given what it actually does.

Which would be easier to update when things change?
Which would be possible to automatically generate from gram.y?

-- 
greg

On Wed, Dec 09, 2015 at 03:49:20PM +0000, Greg Stark wrote:
> On Wed, Dec 9, 2015 at 2:27 PM, David Fetter <david@fetter.org> wrote:
> > Agreed that the "whole new language" aspect seems like way too big a
> > hammer, given what it actually does.
> 
> Which would be easier to update when things change?

This question seems closer to being on point with the patch sets
proposed here.

> Which would be possible to automatically generate from gram.y?

This seems like it goes to a wholesale context-aware reworking of tab
completion rather than the myopic ("What has happened within the past N
tokens?", for slowly increasing N) versions of tab completions in both
the current code and in the two proposals here.

A context-aware tab completion wouldn't care how many columns you were
into a target list, or a FROM list, or whatever, as it would complete
based on the (possibly nested) context ("in a target list", e.g.)
rather than on inferences made from some slightly variable number of
previous tokens.

Cheers,
David.
-- 
David Fetter <david@fetter.org> http://fetter.org/
Phone: +1 415 235 3778  AIM: dfetter666  Yahoo!: dfetter
Skype: davidfetter      XMPP: david.fetter@gmail.com

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate

On Thu, Dec 10, 2015 at 12:49 AM, Greg Stark <stark@mit.edu> wrote:
> On Wed, Dec 9, 2015 at 2:27 PM, David Fetter <david@fetter.org> wrote:
>> Agreed that the "whole new language" aspect seems like way too big a
>> hammer, given what it actually does.
>
> Which would be easier to update when things change?

Regarding that both patches are equal compared to the current methods
with strcmp.

> Which would be possible to automatically generate from gram.y?

None of those patches take this approach.
-- 
Michael

Hello,

At Wed, 9 Dec 2015 10:31:06 -0800, David Fetter <david@fetter.org> wrote in <20151209183106.GC10778@fetter.org>
> On Wed, Dec 09, 2015 at 03:49:20PM +0000, Greg Stark wrote:
> > On Wed, Dec 9, 2015 at 2:27 PM, David Fetter <david@fetter.org> wrote:
> > > Agreed that the "whole new language" aspect seems like way too big a
> > > hammer, given what it actually does.
> > 
> > Which would be easier to update when things change?
> 
> This question seems closer to being on point with the patch sets
> proposed here.

I agree to some extent.

I'm unhappy with context matching using previous_words in two
points. Current code needs human-readable comments describing
almost all matchings. It is hard to maintain and some of them
actually are wrong. The hardness is largely alleviated by
Thomas's approach exept for complex ones. Another is that
previous_words method is not-enough adaptable for optional words
in syntax. For example, CREATE INDEX has a complex syntax and
current rather complex code does not cover it fully (or enough).

On the other hand, regexp is quite heavy-weight. Current code
does one completion in 1 milliseconds but regexps simplly
replaced with current matching code takes nearly 100ms on my
environment. But appropriate refactoring reduces it to under 10
ms.

If we need more powerful completion (which means it covers more
wide area of syntax including more optional words), Thomas's
approach would face difficulties of another level of
complexity. I'd like to overcome it.

> > Which would be possible to automatically generate from gram.y?
> 
> This seems like it goes to a wholesale context-aware reworking of tab
> completion rather than the myopic ("What has happened within the past N
> tokens?", for slowly increasing N) versions of tab completions in both
> the current code and in the two proposals here.
> 
> A context-aware tab completion wouldn't care how many columns you were
> into a target list, or a FROM list, or whatever, as it would complete
> based on the (possibly nested) context ("in a target list", e.g.)
> rather than on inferences made from some slightly variable number of
> previous tokens.

It's unknown to me how much the full-context-aware completion
makes me happy. But it would be another high-wall to overcome..

regards,

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

On Thu, Dec 10, 2015 at 5:38 PM, Kyotaro HORIGUCHI
<horiguchi.kyotaro@lab.ntt.co.jp> wrote:
> I'm unhappy with context matching using previous_words in two
> points. Current code needs human-readable comments describing
> almost all matchings. It is hard to maintain and some of them
> actually are wrong. The hardness is largely alleviated by
> Thomas's approach exept for complex ones. Another is that
> previous_words method is not-enough adaptable for optional words
> in syntax. For example, CREATE INDEX has a complex syntax and
> current rather complex code does not cover it fully (or enough).

Yep.

> On the other hand, regexp is quite heavy-weight. Current code
> does one completion in 1 milliseconds but regexps simplly
> replaced with current matching code takes nearly 100ms on my
> environment. But appropriate refactoring reduces it to under 10
> ms.

That's quite a difference in performance. A good responsiveness is
always nice for such things to make the user confortable.

> If we need more powerful completion (which means it covers more
> wide area of syntax including more optional words), Thomas's
> approach would face difficulties of another level of
> complexity. I'd like to overcome it.

That's a valid concern for sure because the patch of Thomas is not
much smart in emulating negative checks, still the main idea to not
rely anymore on some checks based on pg_strcmp or similar but have
something that is list-based, with a primitive sub-language in it is
very appealing.

As a next step, more committer and hacker input (people who have
worked on tab completion of psql) would be a nice next step. IMO, as
someone who has hacked tab-complete.c a couple of times I think that
Thomas' patch has merit, now it would make backpatch harder. Also, if
we prioritize a dynamically generated tab completion using gram.y, so
be it and let's reject both patches then...
-- 
Michael

On Fri, Dec 11, 2015 at 8:12 AM, Michael Paquier
<michael.paquier@gmail.com> wrote:
> Also, if
> we prioritize a dynamically generated tab completion using gram.y, so
> be it and let's reject both patches then...


Fwiw I poked at the bison output to see if it would be possible to do.
I think it's theoretically possible but a huge project and would
create dependencies on bison internals that we would be unlikelly to
accept. (Unless we can get new API methods added to bison which is not
entirely unreasonable). The problem is that bison is only a small part
of the problem.

You would need

a) A new protocol message to send the partial query to the server and
get back a list of completions
b) Machinery in bison to return both all terminals that could come
next as well as all possible terminals it could reduce to
c) Some kind of reverse lexer to determine for each terminal what the
current partial input would have to match to be accepted
d) Some way to deal with the partially parsed query to find out what
schemas, tables, column aliases, etc should be considered for possible
completion

The machinery to do (b) is actually there in bison for the error
reporting. It's currently hard coded to limit the output to 5 and
there's no API for it, just a function that returns an error string.
But it might be possible to get bison to add an API method for it. But
that's as far as I got. I have no idea what (c) and (d) would look
like.

So I don't think it makes sense to hold up improvements today hoping
for something like this. What might be more realistic is making sure
to design the minilanguage to be easily generated by perl scripts or
the like and then write something picking up easy patterns in gram.y
or possibly poking through the bison table to generate a table of
minilanguage matchers. My instinct is that would be easier to do with
a real minilanguage instead of a regular expression system.


-- 
greg

Greg Stark <stark@mit.edu> writes:
> Fwiw I poked at the bison output to see if it would be possible to do.
> I think it's theoretically possible but a huge project and would
> create dependencies on bison internals that we would be unlikelly to
> accept.

That's the impression I got when I looked at it briefly, too.  Without
some new APIs from bison it seems like it'd be way too messy/fragile.

> You would need
> a) A new protocol message to send the partial query to the server and
> get back a list of completions

As far as that goes, I'd imagined the functionality continuing to be
on the psql side.  If we make it wait for a protocol upgrade, that
makes it even more improbable that it will ever happen.  psql already
has its own copy of the lexer, so making it have something derived
from the grammar doesn't seem like a maintainability problem.

> b) Machinery in bison to return both all terminals that could come
> next as well as all possible terminals it could reduce to

Yeah, this is the hard part.

> d) Some way to deal with the partially parsed query to find out what
> schemas, tables, column aliases, etc should be considered for possible
> completion

I was imagining that some of that knowledge could be pushed back into the
grammar.  That is, instead of just using generic nonterminals like ColId,
we'd need to have TableId, SchemaId, etc and be careful to use the
appropriate one(s) in each production of the grammar.  Then, psql would
know which completion query to issue by noting which of these particular
nonterminals is a candidate for the next token right now.  However, that
moves the goalposts in terms of what we'd have to be able to get back from
the alternate bison machinery.

Also, it's not just a SMOP to modify the grammar like that: it's not
at all unlikely that attempting to introduce such a finer categorization
would lead to a broken grammar, ie shift/reduce or reduce/reduce
conflicts.  We couldn't be sure it would work till we've tried it.

> So I don't think it makes sense to hold up improvements today hoping
> for something like this.

Yeah, it's certainly a wishlist item rather than something that should
block shorter-term improvements.
        regards, tom lane

On Sat, Dec 12, 2015 at 12:00 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Greg Stark <stark@mit.edu> writes:
>> So I don't think it makes sense to hold up improvements today hoping
>> for something like this.
>
> Yeah, it's certainly a wishlist item rather than something that should
> block shorter-term improvements.

OK, hence it seems that the next move is to move on with Thomas' stuff.
-- 
Michael

On Wed, Dec 9, 2015 at 8:17 PM, Thomas Munro wrote:
> Thanks for looking at this Michael.  It's probably not much fun to
> review!  Here is a new version with that bit removed.  More responses
> inline below.

Could this patch be rebased? There are now conflicts with 8b469bd7 and
it does not apply cleanly.
-- 
Michael

On Sun, Dec 13, 2015 at 1:08 AM, Michael Paquier
<michael.paquier@gmail.com> wrote:
> On Wed, Dec 9, 2015 at 8:17 PM, Thomas Munro wrote:
>> Thanks for looking at this Michael.  It's probably not much fun to
>> review!  Here is a new version with that bit removed.  More responses
>> inline below.
>
> Could this patch be rebased? There are now conflicts with 8b469bd7 and
> it does not apply cleanly.

New version attached.

I've also add (very) primitive negative pattern support and used it in
5 places.  Improvement?  Examples:

        /* ALTER TABLE xxx RENAME yyy */
-       else if (TailMatches5("ALTER", "TABLE", MatchAny, "RENAME", MatchAny) &&
-                        !TailMatches1("CONSTRAINT|TO"))
+       else if (TailMatches5("ALTER", "TABLE", MatchAny, "RENAME",
"!CONSTRAINT|TO"))
                COMPLETE_WITH_CONST("TO");

        /* If we have CLUSTER <sth>, then add "USING" */
-       else if (TailMatches2("CLUSTER", MatchAny) &&
!TailMatches1("VERBOSE|ON"))
+       else if (TailMatches2("CLUSTER", "!VERBOSE|ON"))
                COMPLETE_WITH_CONST("USING");

--
Thomas Munro
http://www.enterprisedb.com

On Mon, Dec 14, 2015 at 8:10 AM, Thomas Munro
<thomas.munro@enterprisedb.com> wrote:
> I've also add (very) primitive negative pattern support and used it in
> 5 places.  Improvement?  Examples:
>
>         /* ALTER TABLE xxx RENAME yyy */
> -       else if (TailMatches5("ALTER", "TABLE", MatchAny, "RENAME", MatchAny) &&
> -                        !TailMatches1("CONSTRAINT|TO"))
> +       else if (TailMatches5("ALTER", "TABLE", MatchAny, "RENAME",
> "!CONSTRAINT|TO"))
>                 COMPLETE_WITH_CONST("TO");
>
>         /* If we have CLUSTER <sth>, then add "USING" */
> -       else if (TailMatches2("CLUSTER", MatchAny) &&
> !TailMatches1("VERBOSE|ON"))
> +       else if (TailMatches2("CLUSTER", "!VERBOSE|ON"))
>                 COMPLETE_WITH_CONST("USING");

+       /* Handle negated patterns. */
+       if (*pattern == '!')
+               return !word_matches(pattern + 1, word);

Yeah, I guess that's an improvement for those cases, and there is no
immediate need for a per-keyword NOT operator in our cases to allow
things of the type (foo1 OR NOT foo2). Still, in the case of this
patch !foo1|foo2 actually means (NOT foo1 AND NOT foo2). It does not
seem that much intuitive. Reading straight this expression it seems
that !foo1|foo2 means actually (NOT foo1 OR foo2) because the lack of
parenthesis. Thoughts?
-- 
Michael

Hello.

Ok, I withdraw the minilang solution and I'll go on Thomas's way,
which is still good to have.

At Mon, 14 Dec 2015 14:13:02 +0900, Michael Paquier <michael.paquier@gmail.com> wrote in
<CAB7nPqSggjPA8U1WV7ivW44xzboC8pci_Etmffr+ZEzxSX_ahA@mail.gmail.com>
> On Mon, Dec 14, 2015 at 8:10 AM, Thomas Munro
> <thomas.munro@enterprisedb.com> wrote:
> > I've also add (very) primitive negative pattern support and used it in
> > 5 places.  Improvement?  Examples:
> >
> >         /* ALTER TABLE xxx RENAME yyy */
> > -       else if (TailMatches5("ALTER", "TABLE", MatchAny, "RENAME", MatchAny) &&
> > -                        !TailMatches1("CONSTRAINT|TO"))
> > +       else if (TailMatches5("ALTER", "TABLE", MatchAny, "RENAME",
> > "!CONSTRAINT|TO"))
> >                 COMPLETE_WITH_CONST("TO");
> >
> >         /* If we have CLUSTER <sth>, then add "USING" */
> > -       else if (TailMatches2("CLUSTER", MatchAny) &&
> > !TailMatches1("VERBOSE|ON"))
> > +       else if (TailMatches2("CLUSTER", "!VERBOSE|ON"))
> >                 COMPLETE_WITH_CONST("USING");
> 
> +       /* Handle negated patterns. */
> +       if (*pattern == '!')
> +               return !word_matches(pattern + 1, word);
> 
> Yeah, I guess that's an improvement for those cases, and there is no
> immediate need for a per-keyword NOT operator in our cases to allow
> things of the type (foo1 OR NOT foo2). Still, in the case of this
> patch !foo1|foo2 actually means (NOT foo1 AND NOT foo2). It does not
> seem that much intuitive. Reading straight this expression it seems
> that !foo1|foo2 means actually (NOT foo1 OR foo2) because the lack of
> parenthesis. Thoughts?

I used just the same expression as Thomas in my patch since it
was enough intuitive in this context in my view. The expression
"(!FOO1)|FOO2" is a nonsence in the context of tab-completion and
won't be used in future. But it is true that "!(FOO|BAR|BAZ)" is
clearer than without the parenthesis.

We could use other characters as the operator, but it also might
make it a bit difficalt to read the meaning.

"~FOO|BAR|BAZ", "-FOO|BAR|BAZ"


"TailMatches2("CLUSTER", NEG "VERBOSE|ON")" is mere a syntax
sugar but reduces the uneasiness. But rather longer than adding
parenthesis.

As the result, I vote for "!(FOO|BAR|BAZ)", then "-FOO|BAR|BAZ"
for now.



Addition to that, I feel that successive "MatchAny"s are a bit
bothersome.

>  TailMatches6("COMMENT", "ON", MatchAny, MatchAny, MatchAny, MatchAny)) &&
>              !TailMatches1("IS")

Is MachAny<n> acceptable? On concern is the two n's
(TailMatches<n> and MatchAny<n>) looks a bit confising.

>  TailMatches4("COMMENT", "ON", MatchAny3, "!IS")

regards,

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

On Thu, Dec 17, 2015 at 3:06 PM, Kyotaro HORIGUCHI
<horiguchi.kyotaro@lab.ntt.co.jp> wrote:
> At Mon, 14 Dec 2015 14:13:02 +0900, Michael Paquier <michael.paquier@gmail.com> wrote in
<CAB7nPqSggjPA8U1WV7ivW44xzboC8pci_Etmffr+ZEzxSX_ahA@mail.gmail.com>
>> On Mon, Dec 14, 2015 at 8:10 AM, Thomas Munro <thomas.munro@enterprisedb.com> wrote:
>> Yeah, I guess that's an improvement for those cases, and there is no
>> immediate need for a per-keyword NOT operator in our cases to allow
>> things of the type (foo1 OR NOT foo2). Still, in the case of this
>> patch !foo1|foo2 actually means (NOT foo1 AND NOT foo2). It does not
>> seem that much intuitive. Reading straight this expression it seems
>> that !foo1|foo2 means actually (NOT foo1 OR foo2) because the lack of
>> parenthesis. Thoughts?
>
> I used just the same expression as Thomas in my patch since it
> was enough intuitive in this context in my view. The expression
> "(!FOO1)|FOO2" is a nonsence in the context of tab-completion and
> won't be used in future. But it is true that "!(FOO|BAR|BAZ)" is
> clearer than without the parenthesis.

Yeah that's my whole point. If we decide to support that I think that
we had better go all the way through it, with stuff like:
- & for AND operator
- Support of parenthesis to prioritize expressions
- ! for NOT operator
- | for OR OPERATOR
But honestly at this stage this would just benefit 5 places (citing
Thomas' quote), hence let's just go to the most simple approach which
is the one without all this fancy operator stuff... That will be less
bug prone, and still benefits more than 95% of the tab completion code
path. At least I think that's the most realistic thing to do if we
want to get something for this commit fest. If someone wants those
operators, well I guess that he could add them afterwards.. Thomas,
what do you think?

> Addition to that, I feel that successive "MatchAny"s are a bit
> bothersome.
>
>>  TailMatches6("COMMENT", "ON", MatchAny, MatchAny, MatchAny, MatchAny)) &&
>>              !TailMatches1("IS")
>
> Is MachAny<n> acceptable? On concern is the two n's
> (TailMatches<n> and MatchAny<n>) looks a bit confising.
>
>>  TailMatches4("COMMENT", "ON", MatchAny3, "!IS")

Ah, OK, so you would want to be able to have an inner list, MatchAnyN
meaning actually a list of MatchAny items repeated N times. I am not
sure if that's worth it.. I would just keep it simple, and we are just
discussing about a couple of places only that would benefit from that.
-- 
Michael

On Thu, Dec 17, 2015 at 7:24 PM, Michael Paquier
<michael.paquier@gmail.com> wrote:
> On Thu, Dec 17, 2015 at 3:06 PM, Kyotaro HORIGUCHI
> <horiguchi.kyotaro@lab.ntt.co.jp> wrote:
>> At Mon, 14 Dec 2015 14:13:02 +0900, Michael Paquier <michael.paquier@gmail.com> wrote in
<CAB7nPqSggjPA8U1WV7ivW44xzboC8pci_Etmffr+ZEzxSX_ahA@mail.gmail.com>
>>> On Mon, Dec 14, 2015 at 8:10 AM, Thomas Munro <thomas.munro@enterprisedb.com> wrote:
>>> Yeah, I guess that's an improvement for those cases, and there is no
>>> immediate need for a per-keyword NOT operator in our cases to allow
>>> things of the type (foo1 OR NOT foo2). Still, in the case of this
>>> patch !foo1|foo2 actually means (NOT foo1 AND NOT foo2). It does not
>>> seem that much intuitive. Reading straight this expression it seems
>>> that !foo1|foo2 means actually (NOT foo1 OR foo2) because the lack of
>>> parenthesis. Thoughts?
>>
>> I used just the same expression as Thomas in my patch since it
>> was enough intuitive in this context in my view. The expression
>> "(!FOO1)|FOO2" is a nonsence in the context of tab-completion and
>> won't be used in future. But it is true that "!(FOO|BAR|BAZ)" is
>> clearer than without the parenthesis.
>
> Yeah that's my whole point. If we decide to support that I think that
> we had better go all the way through it, with stuff like:
> - & for AND operator
> - Support of parenthesis to prioritize expressions
> - ! for NOT operator
> - | for OR OPERATOR
> But honestly at this stage this would just benefit 5 places (citing
> Thomas' quote), hence let's just go to the most simple approach which
> is the one without all this fancy operator stuff... That will be less
> bug prone, and still benefits more than 95% of the tab completion code
> path. At least I think that's the most realistic thing to do if we
> want to get something for this commit fest. If someone wants those
> operators, well I guess that he could add them afterwards.. Thomas,
> what do you think?

I agree with both of you that using "!" without parentheses is not
ideal.  I also don't think it's worth trying to make a clever language
here: in future it will be a worthy and difficult project to do
something far cleverer based on the productions of the real grammar as
several people have said.  (Presumably with some extra annotations to
enable/disable productions and mark out points where database object
names are looked up?).  In the meantime, I think we should just do the
simplest thing that will replace the repetitive strcasecmp-based code
with something readable/writable, and avoid the
fully-general-pattern-language rabbit hole.

Kyotaro's suggestion of using a macro NEG x to avoid complicating the
string constants seems good to me.  But perhaps like this?

  TailMatches4("COMMENT", "ON", MatchAny, MatchAnyExcept("IS"))

See attached patch which does it that way.

>> Addition to that, I feel that successive "MatchAny"s are a bit
>> bothersome.
>>
>>>  TailMatches6("COMMENT", "ON", MatchAny, MatchAny, MatchAny, MatchAny)) &&
>>>              !TailMatches1("IS")
>>
>> Is MachAny<n> acceptable? On concern is the two n's
>> (TailMatches<n> and MatchAny<n>) looks a bit confising.
>>
>>>  TailMatches4("COMMENT", "ON", MatchAny3, "!IS")
>
> Ah, OK, so you would want to be able to have an inner list, MatchAnyN
> meaning actually a list of MatchAny items repeated N times. I am not
> sure if that's worth it.. I would just keep it simple, and we are just
> discussing about a couple of places only that would benefit from that.

+1 for simple.

--
Thomas Munro
http://www.enterprisedb.com

On Thu, Dec 17, 2015 at 6:04 PM, Thomas Munro
<thomas.munro@enterprisedb.com> wrote:
> On Thu, Dec 17, 2015 at 7:24 PM, Michael Paquier
> <michael.paquier@gmail.com> wrote:
> Kyotaro's suggestion of using a macro NEG x to avoid complicating the
> string constants seems good to me.  But perhaps like this?
>
>   TailMatches4("COMMENT", "ON", MatchAny, MatchAnyExcept("IS"))
>
> See attached patch which does it that way.

Fine for me.

>>> Addition to that, I feel that successive "MatchAny"s are a bit
>>> bothersome.
>>>
>>>>  TailMatches6("COMMENT", "ON", MatchAny, MatchAny, MatchAny, MatchAny)) &&
>>>>              !TailMatches1("IS")
>>>
>>> Is MachAny<n> acceptable? On concern is the two n's
>>> (TailMatches<n> and MatchAny<n>) looks a bit confising.
>>>
>>>>  TailMatches4("COMMENT", "ON", MatchAny3, "!IS")
>>
>> Ah, OK, so you would want to be able to have an inner list, MatchAnyN
>> meaning actually a list of MatchAny items repeated N times. I am not
>> sure if that's worth it.. I would just keep it simple, and we are just
>> discussing about a couple of places only that would benefit from that.
>
> +1 for simple.

+#define CompleteWithList10(s1, s2, s3, s4, s5, s6, s7, s8, s9, s10)    \
+do { \
+       static const char *const list[] = { s1, s2, s3, s4, s5, s6,
s7, s8, s9, s10, NULL }; \
+       completion_charpp = list; \
+       completion_case_sensitive = false; \
+       matches = completion_matches(text, complete_from_list); \
+} while (0)
Actually we are not using this one. I am fine if this is kept, just
worth noting.

OK, I am marking that as ready for committer. Let's see what happens next.
Regards,
-- 
Michael

Michael Paquier <michael.paquier@gmail.com> writes:
> OK, I am marking that as ready for committer. Let's see what happens next.

I'll pick this up, as penance for not having done much in this commitfest.
I think it's important to get it pushed quickly so that Thomas doesn't
have to keep tracking unrelated changes in tab-complete.c.
        regards, tom lane

Thomas Munro <thomas.munro@enterprisedb.com> writes:
> [ tab-complete-macrology-v11.patch.gz ]

A couple of stylistic reactions after looking through the patch for the
first time in a long time:

1. It seems inconsistent that all the new macros are named in CamelCase
style, whereas there is still plenty of usage of the existing macros like
COMPLETE_WITH_LIST.  It looks pretty jarring IMO.  I think we should
either rename the new macros back to all-upper-case style, or rename the
existing macros in CamelCase style.

I slightly favor the latter option; we're already pretty much breaking any
hope of tab-complete fixes applying backwards over this patch, so changing
the code even more doesn't seem like a problem.  Either way, it's a quick
search-and-replace.  Thoughts?

2. Why does MatchAnyExcept use "'" as the inversion flag, rather than
say "!" or "~" ?  Seems pretty random.
        regards, tom lane

On Sat, Dec 19, 2015 at 5:42 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Thomas Munro <thomas.munro@enterprisedb.com> writes:
>> [ tab-complete-macrology-v11.patch.gz ]
>
> A couple of stylistic reactions after looking through the patch for the
> first time in a long time:
>
> 1. It seems inconsistent that all the new macros are named in CamelCase
> style, whereas there is still plenty of usage of the existing macros like
> COMPLETE_WITH_LIST.  It looks pretty jarring IMO.  I think we should
> either rename the new macros back to all-upper-case style, or rename the
> existing macros in CamelCase style.
>
> I slightly favor the latter option; we're already pretty much breaking any
> hope of tab-complete fixes applying backwards over this patch, so changing
> the code even more doesn't seem like a problem.  Either way, it's a quick
> search-and-replace.  Thoughts?

Both would be fine, honestly. Now if we look at the current code there
are already a lot of macros IN_UPPER_CASE, so it would make more sense
on the contrary to have MATCHES_N and MATCHES_EXCEPT?

> 2. Why does MatchAnyExcept use "'" as the inversion flag, rather than
> say "!" or "~" ?  Seems pretty random.

Actually, "'" is not that much a good idea, no? There could be single
quotes in queries so there is a risk of interfering with the
completion... What do you think would be good candidates? "?", "!",
"#" or "&"?
-- 
Michael

Michael Paquier <michael.paquier@gmail.com> writes:
> On Sat, Dec 19, 2015 at 5:42 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> 2. Why does MatchAnyExcept use "'" as the inversion flag, rather than
>> say "!" or "~" ?  Seems pretty random.

> Actually, "'" is not that much a good idea, no? There could be single
> quotes in queries so there is a risk of interfering with the
> completion... What do you think would be good candidates? "?", "!",
> "#" or "&"?

We don't care whether the character appears in queries; it only matters
that it not be something we'd need to use in patterns.  My inclination
is to use "!".
        regards, tom lane

Michael Paquier <michael.paquier@gmail.com> writes:
> On Sat, Dec 19, 2015 at 5:42 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> 1. It seems inconsistent that all the new macros are named in CamelCase
>> style, whereas there is still plenty of usage of the existing macros like
>> COMPLETE_WITH_LIST.  It looks pretty jarring IMO.  I think we should
>> either rename the new macros back to all-upper-case style, or rename the
>> existing macros in CamelCase style.
>> 
>> I slightly favor the latter option; we're already pretty much breaking any
>> hope of tab-complete fixes applying backwards over this patch, so changing
>> the code even more doesn't seem like a problem.  Either way, it's a quick
>> search-and-replace.  Thoughts?

> Both would be fine, honestly. Now if we look at the current code there
> are already a lot of macros IN_UPPER_CASE, so it would make more sense
> on the contrary to have MATCHES_N and MATCHES_EXCEPT?

After some contemplation I decided that what was bugging me was mainly
the inconsistency of having some completion actions in camelcase and
immediately adjacent actions in all-upper-case.  Making the test macros
camelcase isn't such a problem as long as they all look similar, and
I think it's more readable anyway.  So I changed CompleteWithListN to
COMPLETE_WITH_LISTN and otherwise left the naming alone.

I've committed this now with a number of changes, many of them just
stylistic.  Notable is that I rewrote word_matches to rely on
pg_strncasecmp rather than using toupper/tolower directly, so as to avoid
any possible change in semantics.  (The code as submitted was flat wrong
anyway, since it wasn't being careful about passing only unsigned chars to
those functions.)  I also got rid of its inconsistent treatment of empty
strings, in favor of not ever calling word_matches() on nonexistent words
in the first place.  That just requires testing previous_words_count in
the TailMatches macros.  I think it'd now be possible for 
get_previous_words to skip generating empty strings for word positions
before the start of line, but have not experimented.

I found a couple more small errors in the converted match rules too,
but I have to admit my eyes started to glaze over while looking through
them.  It's possible there are some remaining errors there.  On the
other hand, it's at least as likely that we've gotten rid of some bugs.

There's a good deal more that could be done here:

1. I think it would be a good idea to convert the matching rules for
backslash commands too.  To do that, we'd need to provide a case-sensitive
equivalent to word_match and the matching macros.  I think we'd also have
to extend word_match to allow a trailing wildcard character, maybe "*".

2. I believe that a very large fraction of the TailMatches() rules really
ought to be Matches(), ie, they should not consider matches that don't
start at the start of the line.  And there's another bunch that could
be Matches() if the author hadn't been unaccountably lazy about checking
all words of the expected command.  If we converted as much as we could
that way, it would make psql_completion faster because many inapplicable
rules could be discarded after a single integer comparison on
previous_words_count, and it would greatly reduce the risk of inapplicable
matches.  We can't do that for rules meant to apply to DML statements,
since they can be buried in WITH, EXPLAIN, etc ... but an awful lot of
the DDL rules could be changed.

3. The HeadMatches macros are pretty iffy because they can only look back
nine words.  I'm tempted to redesign get_previous_words so it just
tokenizes the whole line rather than having an arbitrary limitation.
(For that matter, it's long overdue for it to be able to deal with
multiline input...)

I might go look at #3, but I can't currently summon the energy to tackle
#1 or #2 --- any volunteers?
        regards, tom lane

On Sun, Dec 20, 2015 at 6:24 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> 1. I think it would be a good idea to convert the matching rules for
> backslash commands too.  To do that, we'd need to provide a case-sensitive
> equivalent to word_match and the matching macros.  I think we'd also have
> to extend word_match to allow a trailing wildcard character, maybe "*".
>
> 2. I believe that a very large fraction of the TailMatches() rules really
> ought to be Matches(), ie, they should not consider matches that don't
> start at the start of the line.  And there's another bunch that could
> be Matches() if the author hadn't been unaccountably lazy about checking
> all words of the expected command.  If we converted as much as we could
> that way, it would make psql_completion faster because many inapplicable
> rules could be discarded after a single integer comparison on
> previous_words_count, and it would greatly reduce the risk of inapplicable
> matches.  We can't do that for rules meant to apply to DML statements,
> since they can be buried in WITH, EXPLAIN, etc ... but an awful lot of
> the DDL rules could be changed.
>
> 3. The HeadMatches macros are pretty iffy because they can only look back
> nine words.  I'm tempted to redesign get_previous_words so it just
> tokenizes the whole line rather than having an arbitrary limitation.
> (For that matter, it's long overdue for it to be able to deal with
> multiline input...)
>
> I might go look at #3, but I can't currently summon the energy to tackle
> #1 or #2 --- any volunteers?

I could have a look at both of them and submit patch for next CF, both
things do not seem that much complicated.
-- 
Michael

I wrote:
> 3. The HeadMatches macros are pretty iffy because they can only look back
> nine words.  I'm tempted to redesign get_previous_words so it just
> tokenizes the whole line rather than having an arbitrary limitation.
> (For that matter, it's long overdue for it to be able to deal with
> multiline input...)

I poked into this and found that it's really not hard, if you don't mind
still another global variable associated with tab-completion.  See
attached patch.

The main objection to this change might be speed, but I experimented with
the longest query in information_schema.sql (CREATE VIEW usage_privileges,
162 lines) and found that pressing tab at the end of that seemed to take
well under a millisecond on my workstation.  So I think it's probably
insignificant, especially compared to any of the code paths that send a
query to the server for tab completion.

            regards, tom lane

diff --git a/src/bin/psql/input.c b/src/bin/psql/input.c
index 2bc065a..ccd9a3e 100644
*** a/src/bin/psql/input.c
--- b/src/bin/psql/input.c
*************** static void finishInput(void);
*** 53,64 ****
   * gets_interactive()
   *
   * Gets a line of interactive input, using readline if desired.
   * The result is a malloc'd string.
   *
   * Caller *must* have set up sigint_interrupt_jmp before calling.
   */
  char *
! gets_interactive(const char *prompt)
  {
  #ifdef USE_READLINE
      if (useReadline)
--- 53,69 ----
   * gets_interactive()
   *
   * Gets a line of interactive input, using readline if desired.
+  *
+  * prompt: the prompt string to be used
+  * query_buf: buffer containing lines already read in the current command
+  * (query_buf is not modified here, but may be consulted for tab completion)
+  *
   * The result is a malloc'd string.
   *
   * Caller *must* have set up sigint_interrupt_jmp before calling.
   */
  char *
! gets_interactive(const char *prompt, PQExpBuffer query_buf)
  {
  #ifdef USE_READLINE
      if (useReadline)
*************** gets_interactive(const char *prompt)
*** 76,81 ****
--- 81,89 ----
          rl_reset_screen_size();
  #endif

+         /* Make current query_buf available to tab completion callback */
+         tab_completion_query_buf = query_buf;
+
          /* Enable SIGINT to longjmp to sigint_interrupt_jmp */
          sigint_interrupt_enabled = true;

*************** gets_interactive(const char *prompt)
*** 85,90 ****
--- 93,101 ----
          /* Disable SIGINT again */
          sigint_interrupt_enabled = false;

+         /* Pure neatnik-ism */
+         tab_completion_query_buf = NULL;
+
          return result;
      }
  #endif
diff --git a/src/bin/psql/input.h b/src/bin/psql/input.h
index abd7012..c9d30b9 100644
*** a/src/bin/psql/input.h
--- b/src/bin/psql/input.h
***************
*** 38,44 ****
  #include "pqexpbuffer.h"


! char       *gets_interactive(const char *prompt);
  char       *gets_fromFile(FILE *source);

  void        initializeInput(int flags);
--- 38,44 ----
  #include "pqexpbuffer.h"


! char       *gets_interactive(const char *prompt, PQExpBuffer query_buf);
  char       *gets_fromFile(FILE *source);

  void        initializeInput(int flags);
diff --git a/src/bin/psql/mainloop.c b/src/bin/psql/mainloop.c
index b6cef94..cb86457 100644
*** a/src/bin/psql/mainloop.c
--- b/src/bin/psql/mainloop.c
*************** MainLoop(FILE *source)
*** 133,139 ****
              /* May need to reset prompt, eg after \r command */
              if (query_buf->len == 0)
                  prompt_status = PROMPT_READY;
!             line = gets_interactive(get_prompt(prompt_status));
          }
          else
          {
--- 133,139 ----
              /* May need to reset prompt, eg after \r command */
              if (query_buf->len == 0)
                  prompt_status = PROMPT_READY;
!             line = gets_interactive(get_prompt(prompt_status), query_buf);
          }
          else
          {
diff --git a/src/bin/psql/tab-complete.c b/src/bin/psql/tab-complete.c
index 731df2a..fec7c38 100644
*** a/src/bin/psql/tab-complete.c
--- b/src/bin/psql/tab-complete.c
*************** extern char *filename_completion_functio
*** 70,75 ****
--- 70,82 ----
  #define WORD_BREAKS        "\t\n@$><=;|&{() "

  /*
+  * Since readline doesn't let us pass any state through to the tab completion
+  * callback, we have to use this global variable to let get_previous_words()
+  * get at the previous lines of the current command.  Ick.
+  */
+ PQExpBuffer tab_completion_query_buf = NULL;
+
+ /*
   * This struct is used to define "schema queries", which are custom-built
   * to obtain possibly-schema-qualified names of database objects.  There is
   * enough similarity in the structure that we don't want to repeat it each
*************** static char *pg_strdup_keyword_case(cons
*** 923,929 ****
  static char *escape_string(const char *text);
  static PGresult *exec_query(const char *query);

! static int    get_previous_words(int point, char **previous_words, int nwords);

  static char *get_guctype(const char *varname);

--- 930,936 ----
  static char *escape_string(const char *text);
  static PGresult *exec_query(const char *query);

! static char **get_previous_words(int point, char **buffer, int *nwords);

  static char *get_guctype(const char *varname);

*************** psql_completion(const char *text, int st
*** 1027,1039 ****
      /* This is the variable we'll return. */
      char      **matches = NULL;

!     /* This array will contain some scannage of the input line. */
!     char       *previous_words[9];

      /* The number of words found on the input line. */
      int            previous_words_count;

!     /* For compactness, we use these macros to reference previous_words[]. */
  #define prev_wd   (previous_words[0])
  #define prev2_wd  (previous_words[1])
  #define prev3_wd  (previous_words[2])
--- 1034,1055 ----
      /* This is the variable we'll return. */
      char      **matches = NULL;

!     /* Workspace for parsed words. */
!     char       *words_buffer;
!
!     /* This array will contain pointers to parsed words. */
!     char      **previous_words;

      /* The number of words found on the input line. */
      int            previous_words_count;

!     /*
!      * For compactness, we use these macros to reference previous_words[].
!      * Caution: do not access a previous_words[] entry without having checked
!      * previous_words_count to be sure it's valid.  In most cases below, that
!      * check is implicit in a TailMatches() or similar macro, but in some
!      * places we have to check it explicitly.
!      */
  #define prev_wd   (previous_words[0])
  #define prev2_wd  (previous_words[1])
  #define prev3_wd  (previous_words[2])
*************** psql_completion(const char *text, int st
*** 1133,1141 ****

      /*
       * Macros for matching N words at the start of the line, regardless of
!      * what is after them.  These are pretty broken because they can only look
!      * back lengthof(previous_words) words, but we'll worry about improving
!      * their implementation some other day.
       */
  #define HeadMatches1(p1) \
      (previous_words_count >= 1 && \
--- 1149,1155 ----

      /*
       * Macros for matching N words at the start of the line, regardless of
!      * what is after them.
       */
  #define HeadMatches1(p1) \
      (previous_words_count >= 1 && \
*************** psql_completion(const char *text, int st
*** 1194,1206 ****
      completion_info_charp2 = NULL;

      /*
!      * Scan the input line before our current position for the last few words.
       * According to those we'll make some smart decisions on what the user is
       * probably intending to type.
       */
!     previous_words_count = get_previous_words(start,
!                                               previous_words,
!                                               lengthof(previous_words));

      /* If current word is a backslash command, offer completions for that */
      if (text[0] == '\\')
--- 1208,1220 ----
      completion_info_charp2 = NULL;

      /*
!      * Scan the input line to extract the words before our current position.
       * According to those we'll make some smart decisions on what the user is
       * probably intending to type.
       */
!     previous_words = get_previous_words(start,
!                                         &words_buffer,
!                                         &previous_words_count);

      /* If current word is a backslash command, offer completions for that */
      if (text[0] == '\\')
*************** psql_completion(const char *text, int st
*** 1692,1698 ****

          COMPLETE_WITH_LIST(list_TABLEOPTIONS);
      }
!     else if (TailMatches4("REPLICA", "IDENTITY", "USING", "INDEX"))
      {
          completion_info_charp = prev5_wd;
          COMPLETE_WITH_QUERY(Query_for_index_of_table);
--- 1706,1712 ----

          COMPLETE_WITH_LIST(list_TABLEOPTIONS);
      }
!     else if (TailMatches5(MatchAny, "REPLICA", "IDENTITY", "USING", "INDEX"))
      {
          completion_info_charp = prev5_wd;
          COMPLETE_WITH_QUERY(Query_for_index_of_table);
*************** psql_completion(const char *text, int st
*** 2806,2812 ****
          if (!recognized_connection_string(text))
              COMPLETE_WITH_QUERY(Query_for_list_of_databases);
      }
!     else if (strcmp(prev2_wd, "\\connect") == 0 || strcmp(prev2_wd, "\\c") == 0)
      {
          if (!recognized_connection_string(prev_wd))
              COMPLETE_WITH_QUERY(Query_for_list_of_roles);
--- 2820,2828 ----
          if (!recognized_connection_string(text))
              COMPLETE_WITH_QUERY(Query_for_list_of_databases);
      }
!     else if (previous_words_count >= 2 &&
!              (strcmp(prev2_wd, "\\connect") == 0 ||
!               strcmp(prev2_wd, "\\c") == 0))
      {
          if (!recognized_connection_string(prev_wd))
              COMPLETE_WITH_QUERY(Query_for_list_of_roles);
*************** psql_completion(const char *text, int st
*** 2891,2897 ****

          COMPLETE_WITH_LIST_CS(my_list);
      }
!     else if (strcmp(prev2_wd, "\\pset") == 0)
      {
          if (strcmp(prev_wd, "format") == 0)
          {
--- 2907,2914 ----

          COMPLETE_WITH_LIST_CS(my_list);
      }
!     else if (previous_words_count >= 2 &&
!              strcmp(prev2_wd, "\\pset") == 0)
      {
          if (strcmp(prev_wd, "format") == 0)
          {
*************** psql_completion(const char *text, int st
*** 2927,2933 ****
      {
          matches = complete_from_variables(text, "", "", false);
      }
!     else if (strcmp(prev2_wd, "\\set") == 0)
      {
          static const char *const boolean_value_list[] =
          {"on", "off", NULL};
--- 2944,2951 ----
      {
          matches = complete_from_variables(text, "", "", false);
      }
!     else if (previous_words_count >= 2 &&
!              strcmp(prev2_wd, "\\set") == 0)
      {
          static const char *const boolean_value_list[] =
          {"on", "off", NULL};
*************** psql_completion(const char *text, int st
*** 3048,3059 ****
      }

      /* free storage */
!     {
!         int            i;
!
!         for (i = 0; i < lengthof(previous_words); i++)
!             free(previous_words[i]);
!     }

      /* Return our Grand List O' Matches */
      return matches;
--- 3066,3073 ----
      }

      /* free storage */
!     free(previous_words);
!     free(words_buffer);

      /* Return our Grand List O' Matches */
      return matches;
*************** exec_query(const char *query)
*** 3632,3661 ****


  /*
!  * Return the nwords word(s) before point.  Words are returned right to left,
!  * that is, previous_words[0] gets the last word before point.
!  * If we run out of words, remaining array elements are set to empty strings.
!  * Each array element is filled with a malloc'd string.
!  * Returns the number of words that were actually found (up to nwords).
   */
! static int
! get_previous_words(int point, char **previous_words, int nwords)
  {
!     const char *buf = rl_line_buffer;    /* alias */
      int            words_found = 0;
      int            i;

!     /* first we look for a non-word char before the current point */
      for (i = point - 1; i >= 0; i--)
          if (strchr(WORD_BREAKS, buf[i]))
              break;
      point = i;

!     while (nwords-- > 0)
      {
          int            start,
                      end;
!         char       *s;

          /* now find the first non-space which then constitutes the end */
          end = -1;
--- 3646,3725 ----


  /*
!  * Parse all the word(s) before point.
!  *
!  * Returns a malloc'd array of character pointers that point into the malloc'd
!  * data array returned to *buffer; caller must free() both of these when done.
!  * *nwords receives the number of words found, ie, the valid length of the
!  * return array.
!  *
!  * Words are returned right to left, that is, previous_words[0] gets the last
!  * word before point, previous_words[1] the next-to-last, etc.
   */
! static char **
! get_previous_words(int point, char **buffer, int *nwords)
  {
!     char      **previous_words;
!     char       *buf;
!     int            buflen;
      int            words_found = 0;
      int            i;

!     /*
!      * Construct a writable buffer including both preceding and current lines
!      * of the query, up to "point" which is where the currently completable
!      * word begins.  Because our definition of "word" is such that a non-word
!      * character must end each word, we can use this buffer to return the word
!      * data as-is, by placing a '\0' after each word.
!      */
!     buflen = point + 1;
!     if (tab_completion_query_buf)
!         buflen += tab_completion_query_buf->len + 1;
!     *buffer = buf = pg_malloc(buflen);
!     i = 0;
!     if (tab_completion_query_buf)
!     {
!         memcpy(buf, tab_completion_query_buf->data,
!                tab_completion_query_buf->len);
!         i += tab_completion_query_buf->len;
!         buf[i++] = '\n';
!     }
!     memcpy(buf + i, rl_line_buffer, point);
!     i += point;
!     buf[i] = '\0';
!
!     /* Readjust point to reference appropriate offset in buf */
!     point = i;
!
!     /*
!      * Allocate array of word start points.  There can be at most length/2 + 1
!      * words in the buffer.
!      */
!     previous_words = (char **) pg_malloc((point / 2 + 1) * sizeof(char *));
!
!     /*
!      * First we look for a non-word char before the current point.  (This is
!      * probably useless, if readline is on the same page as we are about what
!      * is a word, but if so it's cheap.)
!      */
      for (i = point - 1; i >= 0; i--)
+     {
          if (strchr(WORD_BREAKS, buf[i]))
              break;
+     }
      point = i;

!     /*
!      * Now parse words, working backwards, until we hit start of line.  The
!      * backwards scan has some interesting but intentional properties
!      * concerning parenthesis handling.
!      */
!     while (point >= 0)
      {
          int            start,
                      end;
!         bool        inquotes = false;
!         int            parentheses = 0;

          /* now find the first non-space which then constitutes the end */
          end = -1;
*************** get_previous_words(int point, char **pre
*** 3667,3725 ****
                  break;
              }
          }

          /*
!          * If no end found we return an empty string, because there is no word
!          * before the point
           */
!         if (end < 0)
!         {
!             point = end;
!             s = pg_strdup("");
!         }
!         else
          {
!             /*
!              * Otherwise we now look for the start. The start is either the
!              * last character before any word-break character going backwards
!              * from the end, or it's simply character 0. We also handle open
!              * quotes and parentheses.
!              */
!             bool        inquotes = false;
!             int            parentheses = 0;
!
!             for (start = end; start > 0; start--)
              {
!                 if (buf[start] == '"')
!                     inquotes = !inquotes;
!                 if (!inquotes)
                  {
!                     if (buf[start] == ')')
!                         parentheses++;
!                     else if (buf[start] == '(')
!                     {
!                         if (--parentheses <= 0)
!                             break;
!                     }
!                     else if (parentheses == 0 &&
!                              strchr(WORD_BREAKS, buf[start - 1]))
                          break;
                  }
              }
-
-             point = start - 1;
-
-             /* make a copy of chars from start to end inclusive */
-             s = pg_malloc(end - start + 2);
-             strlcpy(s, &buf[start], end - start + 2);
-
-             words_found++;
          }

!         *previous_words++ = s;
      }

!     return words_found;
  }

  /*
--- 3731,3776 ----
                  break;
              }
          }
+         /* if no end found, we're done */
+         if (end < 0)
+             break;

          /*
!          * Otherwise we now look for the start.  The start is either the last
!          * character before any word-break character going backwards from the
!          * end, or it's simply character 0.  We also handle open quotes and
!          * parentheses.
           */
!         for (start = end; start > 0; start--)
          {
!             if (buf[start] == '"')
!                 inquotes = !inquotes;
!             if (!inquotes)
              {
!                 if (buf[start] == ')')
!                     parentheses++;
!                 else if (buf[start] == '(')
                  {
!                     if (--parentheses <= 0)
                          break;
                  }
+                 else if (parentheses == 0 &&
+                          strchr(WORD_BREAKS, buf[start - 1]))
+                     break;
              }
          }

!         /* Return the word located at start to end inclusive */
!         previous_words[words_found] = &buf[start];
!         buf[end + 1] = '\0';
!         words_found++;
!
!         /* Continue searching */
!         point = start - 1;
      }

!     *nwords = words_found;
!     return previous_words;
  }

  /*
diff --git a/src/bin/psql/tab-complete.h b/src/bin/psql/tab-complete.h
index 9dcd7e7..0e9a430 100644
*** a/src/bin/psql/tab-complete.h
--- b/src/bin/psql/tab-complete.h
***************
*** 8,15 ****
  #ifndef TAB_COMPLETE_H
  #define TAB_COMPLETE_H

! #include "postgres_fe.h"

! void        initialize_readline(void);

! #endif
--- 8,17 ----
  #ifndef TAB_COMPLETE_H
  #define TAB_COMPLETE_H

! #include "pqexpbuffer.h"

! extern PQExpBuffer tab_completion_query_buf;

! extern void initialize_readline(void);
!
! #endif   /* TAB_COMPLETE_H */

On Sun, Dec 20, 2015 at 10:24 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> I've committed this now with a number of changes, many of them just
> stylistic.

Thanks!  And thanks also to Michael, Kyotaro, Alvaro and Jeff.  +1 for
the suggested further improvements, which I will help out with where I
can.

-- 
Thomas Munro
http://www.enterprisedb.com

On Sun, Dec 20, 2015 at 8:08 AM, Michael Paquier
<michael.paquier@gmail.com> wrote:
> On Sun, Dec 20, 2015 at 6:24 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> 1. I think it would be a good idea to convert the matching rules for
>> backslash commands too.  To do that, we'd need to provide a case-sensitive
>> equivalent to word_match and the matching macros.  I think we'd also have
>> to extend word_match to allow a trailing wildcard character, maybe "*".

I am not really sure I follow much the use of the wildcard, do you
mean to be able to work with the [S] extensions of the backslash
commands which are not completed now? I am attaching a patch that adds
support for a case-sensitive comparison facility without this wildcard
system, simplifying the backslash commands.

>> 2. I believe that a very large fraction of the TailMatches() rules really
>> ought to be Matches(), ie, they should not consider matches that don't
>> start at the start of the line.  And there's another bunch that could
>> be Matches() if the author hadn't been unaccountably lazy about checking
>> all words of the expected command.  If we converted as much as we could
>> that way, it would make psql_completion faster because many inapplicable
>> rules could be discarded after a single integer comparison on
>> previous_words_count, and it would greatly reduce the risk of inapplicable
>> matches.  We can't do that for rules meant to apply to DML statements,
>> since they can be buried in WITH, EXPLAIN, etc ... but an awful lot of
>> the DDL rules could be changed.

Yep, clearly. We may gain a bit of performance by matching directly
with an equal number of words using Matches instead of a lower bound
with TailMatches. I have looked at this thing and hacked a patch as
attached.

>> 3. The HeadMatches macros are pretty iffy because they can only look back
>> nine words.  I'm tempted to redesign get_previous_words so it just
>> tokenizes the whole line rather than having an arbitrary limitation.
>> (For that matter, it's long overdue for it to be able to deal with
>> multiline input...)
>>
>> I might go look at #3, but I can't currently summon the energy to tackle
>> #1 or #2 --- any volunteers?

#3 has been already done in the mean time...

> I could have a look at both of them and submit patch for next CF, both
> things do not seem that much complicated.

Those things are as well added to the next CF.
--
Michael

Michael Paquier <michael.paquier@gmail.com> writes:
>> On Sun, Dec 20, 2015 at 6:24 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>> 1. I think it would be a good idea to convert the matching rules for
>>> backslash commands too.  To do that, we'd need to provide a case-sensitive
>>> equivalent to word_match and the matching macros.  I think we'd also have
>>> to extend word_match to allow a trailing wildcard character, maybe "*".

> I am not really sure I follow much the use of the wildcard, do you
> mean to be able to work with the [S] extensions of the backslash
> commands which are not completed now?

But they are completed:

regression=# \dfS str<TAB>
string_agg          string_agg_transfn  strip
string_agg_finalfn  string_to_array     strpos

This is because of the use of strncmp instead of plain strcmp
in most of the backslash matching rules, eg the above case is
covered by
else if (strncmp(prev_wd, "\\df", strlen("\\df")) == 0)

I was envisioning that we'd want to convert this to something like
else if (TailMatchesCS1("\\df*"))
        regards, tom lane

On Wed, Dec 30, 2015 at 3:14 AM, Michael Paquier
<michael.paquier@gmail.com> wrote:
> On Sun, Dec 20, 2015 at 8:08 AM, Michael Paquier
> <michael.paquier@gmail.com> wrote:
>> On Sun, Dec 20, 2015 at 6:24 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>> 2. I believe that a very large fraction of the TailMatches() rules really
>>> ought to be Matches(), ie, they should not consider matches that don't
>>> start at the start of the line.  And there's another bunch that could
>>> be Matches() if the author hadn't been unaccountably lazy about checking
>>> all words of the expected command.  If we converted as much as we could
>>> that way, it would make psql_completion faster because many inapplicable
>>> rules could be discarded after a single integer comparison on
>>> previous_words_count, and it would greatly reduce the risk of inapplicable
>>> matches.  We can't do that for rules meant to apply to DML statements,
>>> since they can be buried in WITH, EXPLAIN, etc ... but an awful lot of
>>> the DDL rules could be changed.
>
> Yep, clearly. We may gain a bit of performance by matching directly
> with an equal number of words using Matches instead of a lower bound
> with TailMatches. I have looked at this thing and hacked a patch as
> attached.

I see that you changed INSERT and DELETE (but not UPDATE) to use
MatchesN rather than TailMatchesN.  Shouldn't these stay with
TailMatchesN for the reason Tom gave above?

-- 
Thomas Munro
http://www.enterprisedb.com

On Wed, Dec 30, 2015 at 1:21 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> This is because of the use of strncmp instead of plain strcmp
> in most of the backslash matching rules, eg the above case is
> covered by
>
>         else if (strncmp(prev_wd, "\\df", strlen("\\df")) == 0)

Ah, OK. The length of the name and not the pattern is used in
word_matches, but we had better use something based on the pattern
shape. And the current logic for backslash commands uses the length of
the pattern, and not the word for its checks.

> I was envisioning that we'd want to convert this to something like
>
>         else if (TailMatchesCS1("\\df*"))

That's a better macro name...
-- 
Michael

On Wed, Dec 30, 2015 at 6:26 AM, Thomas Munro
<thomas.munro@enterprisedb.com> wrote:
> On Wed, Dec 30, 2015 at 3:14 AM, Michael Paquier
> <michael.paquier@gmail.com> wrote:
>> On Sun, Dec 20, 2015 at 8:08 AM, Michael Paquier
>> <michael.paquier@gmail.com> wrote:
>>> On Sun, Dec 20, 2015 at 6:24 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>>> 2. I believe that a very large fraction of the TailMatches() rules really
>>>> ought to be Matches(), ie, they should not consider matches that don't
>>>> start at the start of the line.  And there's another bunch that could
>>>> be Matches() if the author hadn't been unaccountably lazy about checking
>>>> all words of the expected command.  If we converted as much as we could
>>>> that way, it would make psql_completion faster because many inapplicable
>>>> rules could be discarded after a single integer comparison on
>>>> previous_words_count, and it would greatly reduce the risk of inapplicable
>>>> matches.  We can't do that for rules meant to apply to DML statements,
>>>> since they can be buried in WITH, EXPLAIN, etc ... but an awful lot of
>>>> the DDL rules could be changed.
>>
>> Yep, clearly. We may gain a bit of performance by matching directly
>> with an equal number of words using Matches instead of a lower bound
>> with TailMatches. I have looked at this thing and hacked a patch as
>> attached.
>
> I see that you changed INSERT and DELETE (but not UPDATE) to use
> MatchesN rather than TailMatchesN.  Shouldn't these stay with
> TailMatchesN for the reason Tom gave above?

Er, yeah. They had better be TailMatches, or even COPY DML stuff will be broken.
-- 
Michael

On Wed, Dec 30, 2015 at 9:14 AM, Michael Paquier wrote:
> On Wed, Dec 30, 2015 at 6:26 AM, Thomas Munro wrote:
>> I see that you changed INSERT and DELETE (but not UPDATE) to use
>> MatchesN rather than TailMatchesN.  Shouldn't these stay with
>> TailMatchesN for the reason Tom gave above?
>
> Er, yeah. They had better be TailMatches, or even COPY DML stuff will be broken.

OK, here are new patches.
- 0001 switches a bunch of TailMatches to Matches. Do we want to care
about the case where a schema is created following by a bunch of
objects? I mean stuff like "CREATE SCHEMA hoge CREATE TABLE ..." where
the current completion would work fine. The performance gains seem
worth it compared to the number of people actually using it, the point
has just not been raised yet.
- 0002 that implements the new tab completion for backslash commands,
with the wildcard "*" as suggested by Tom.

I fixed in 0001 the stuff with DML queries, and also found one bug for
another query while re-reading the code.
Regards,
--
Michael

Michael Paquier wrote:

> OK, here are new patches.
> - 0001 switches a bunch of TailMatches to Matches. Do we want to care
> about the case where a schema is created following by a bunch of
> objects? I mean stuff like "CREATE SCHEMA hoge CREATE TABLE ..." where
> the current completion would work fine. The performance gains seem
> worth it compared to the number of people actually using it, the point
> has just not been raised yet.

I'd rather have the completion work for that case than get a few
microseconds speedup.  As far as I recall, it's only four commands that
must retain the old coding.

-- 
Álvaro Herrera                http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On Wed, Dec 30, 2015 at 11:21 PM, Alvaro Herrera
<alvherre@2ndquadrant.com> wrote:
> Michael Paquier wrote:
>
>> OK, here are new patches.
>> - 0001 switches a bunch of TailMatches to Matches. Do we want to care
>> about the case where a schema is created following by a bunch of
>> objects? I mean stuff like "CREATE SCHEMA hoge CREATE TABLE ..." where
>> the current completion would work fine. The performance gains seem
>> worth it compared to the number of people actually using it, the point
>> has just not been raised yet.
>
> I'd rather have the completion work for that case than get a few
> microseconds speedup.  As far as I recall, it's only four commands that
> must retain the old coding.

Fine for me this way.
-- 
Michael

On Thu, Dec 31, 2015 at 9:13 AM, Michael Paquier
<michael.paquier@gmail.com> wrote:
> On Wed, Dec 30, 2015 at 11:21 PM, Alvaro Herrera
> <alvherre@2ndquadrant.com> wrote:
>> Michael Paquier wrote:
>>
>>> OK, here are new patches.
>>> - 0001 switches a bunch of TailMatches to Matches. Do we want to care
>>> about the case where a schema is created following by a bunch of
>>> objects? I mean stuff like "CREATE SCHEMA hoge CREATE TABLE ..." where
>>> the current completion would work fine. The performance gains seem
>>> worth it compared to the number of people actually using it, the point
>>> has just not been raised yet.
>>
>> I'd rather have the completion work for that case than get a few
>> microseconds speedup.  As far as I recall, it's only four commands that
>> must retain the old coding.
>
> Fine for me this way.

So, here are the commands that still remain with TailMatches to cover
this case, per gram.y:
- CREATE TABLE
- CREATE INDEX
- CREATE VIEW
- GRANT
- CREATE TRIGGER
- CREATE SEQUENCE
New patches are attached.
Regards,
--
Michael

Michael Paquier <michael.paquier@gmail.com> writes:
> So, here are the commands that still remain with TailMatches to cover
> this case, per gram.y:
> - CREATE TABLE
> - CREATE INDEX
> - CREATE VIEW
> - GRANT
> - CREATE TRIGGER
> - CREATE SEQUENCE
> New patches are attached.

I've reviewed and committed the first of these patches.  I found a few
mistakes, mostly where you'd converted TailMatches to Matches without
adding the leading words that the original author had left out of the
pattern.  But man, this is mind-numbingly tedious work :-(.  I probably
made a few more mistakes myself.  Still, the code is enormously more
readable now than when we started, and almost certainly more reliable.

I'm too burned out to look at the second patch tonight, but hopefully
will get to it tomorrow.
        regards, tom lane

On Tue, Jan 5, 2016 at 10:13 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Michael Paquier <michael.paquier@gmail.com> writes:
>> So, here are the commands that still remain with TailMatches to cover
>> this case, per gram.y:
>> - CREATE TABLE
>> - CREATE INDEX
>> - CREATE VIEW
>> - GRANT
>> - CREATE TRIGGER
>> - CREATE SEQUENCE
>> New patches are attached.
>
> I've reviewed and committed the first of these patches.  I found a few
> mistakes, mostly where you'd converted TailMatches to Matches without
> adding the leading words that the original author had left out of the
> pattern.  But man, this is mind-numbingly tedious work :-(.  I probably
> made a few more mistakes myself.  Still, the code is enormously more
> readable now than when we started, and almost certainly more reliable.

Thanks. My best advice is to avoid doing such after 10PM, that's a
good way to finish with a headache. I did it.

> I'm too burned out to look at the second patch tonight, but hopefully
> will get to it tomorrow.

I see that you are on fire these days, still bodies need some rest.
This second one still applies cleanly, but it is far less urgent, the
other psql-tab patches do not depend on it.
-- 
Michael

I wrote:
> Michael Paquier <michael.paquier@gmail.com> writes:
>> So, here are the commands that still remain with TailMatches to cover
>> this case, per gram.y:
>> - CREATE TABLE
>> - CREATE INDEX
>> - CREATE VIEW
>> - GRANT
>> - CREATE TRIGGER
>> - CREATE SEQUENCE
>> New patches are attached.

> I've reviewed and committed the first of these patches.

I've pushed the second patch now.  I made a few adjustments --- notably,
I didn't like the way you'd implemented '*' wildcards, because they
wouldn't have behaved very sanely in combination with '|'.  The case
doesn't come up in the current set of patterns, but we'll likely want it
sometime.
        regards, tom lane

On Wed, Jan 6, 2016 at 2:03 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> I wrote:
>> Michael Paquier <michael.paquier@gmail.com> writes:
>>> So, here are the commands that still remain with TailMatches to cover
>>> this case, per gram.y:
>>> - CREATE TABLE
>>> - CREATE INDEX
>>> - CREATE VIEW
>>> - GRANT
>>> - CREATE TRIGGER
>>> - CREATE SEQUENCE
>>> New patches are attached.
>
>> I've reviewed and committed the first of these patches.
>
> I've pushed the second patch now.  I made a few adjustments --- notably,
> I didn't like the way you'd implemented '*' wildcards, because they
> wouldn't have behaved very sanely in combination with '|'.  The case
> doesn't come up in the current set of patterns, but we'll likely want it
> sometime.

Thanks! Let's consider this project as done then. That was a long way to it...
-- 
Michael

On 01/06/2016 01:13 AM, Michael Paquier wrote:
> On Wed, Jan 6, 2016 at 2:03 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> I've pushed the second patch now.  I made a few adjustments --- notably,
>> I didn't like the way you'd implemented '*' wildcards, because they
>> wouldn't have behaved very sanely in combination with '|'.  The case
>> doesn't come up in the current set of patterns, but we'll likely want it
>> sometime.
>
> Thanks! Let's consider this project as done then. That was a long way to it...

Thanks to everyone involved in cleaning this up, it is much easier to 
add tab completions now.

Andreas