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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Sat, Sep 5, 2015 at 1:40 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

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.

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:

        /* ALTER TABLE,INDEX,MATERIALIZED VIEW xxx ALL IN TABLESPACE xxx */
-       else if (pg_strcasecmp(prev4_wd, "ALL") == 0 &&
-                        pg_strcasecmp(prev3_wd, "IN") == 0 &&
-                        pg_strcasecmp(prev2_wd, "TABLESPACE") == 0)
-       {
-               static const char *const list_ALTERALLINTSPC[] =
-               {"SET TABLESPACE", "OWNED BY", NULL};
-
-               COMPLETE_WITH_LIST(list_ALTERALLINTSPC);
-       }
+       else if (TAIL_MATCHES4("ALL", "IN", "TABLESPACE", ANY))
+               COMPLETE_WITH_LIST2("SET TABLESPACE", "OWNED BY");

... versus:

 /* EXECUTE, but not EXECUTE embedded in other commands */
-       else if (pg_strcasecmp(prev_wd, "EXECUTE") == 0 &&
-                        prev2_wd[0] == '\0')
+       else if (MATCHES1("EXECUTE"))
                COMPLETE_WITH_QUERY(Query_for_list_of_prepared_statements);

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

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Tue, Sep 8, 2015 at 1:56 AM, Alvaro Herrera <alvherre@2ndquadrant.com>
wrote:

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 ...

+1

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

MatchAny would make more sense to me.

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.

Ok, here's a rebased version that uses the style you suggested. It also
adds HeadMatchesN macros, so we can do this:

         * Complete "GRANT/REVOKE ... TO/FROM" with username, PUBLIC,
         * CURRENT_USER, or SESSION_USER.
         */
-       else if (((pg_strcasecmp(prev9_wd, "GRANT") == 0 ||
-                          pg_strcasecmp(prev8_wd, "GRANT") == 0 ||
-                          pg_strcasecmp(prev7_wd, "GRANT") == 0 ||
-                          pg_strcasecmp(prev6_wd, "GRANT") == 0 ||
-                          pg_strcasecmp(prev5_wd, "GRANT") == 0) &&
-                         pg_strcasecmp(prev_wd, "TO") == 0) ||
-                        ((pg_strcasecmp(prev9_wd, "REVOKE") == 0 ||
-                          pg_strcasecmp(prev8_wd, "REVOKE") == 0 ||
-                          pg_strcasecmp(prev7_wd, "REVOKE") == 0 ||
-                          pg_strcasecmp(prev6_wd, "REVOKE") == 0 ||
-                          pg_strcasecmp(prev5_wd, "REVOKE") == 0) &&
-                         pg_strcasecmp(prev_wd, "FROM") == 0))
+       else if ((HeadMatches1("GRANT") && TailMatches1("TO")) ||
+                        (HeadMatches1("REVOKE") && TailMatches1("FROM")))
                COMPLETE_WITH_QUERY(Query_for_list_of_grant_roles);

So to recap:

MatchesN(...) -- matches the whole expression (up to lookback buffer size)
HeadMatchesN(...) -- matches the start of the expression (ditto)
TailMatchesN(...) -- matches the end of the expression
MatchAny -- placeholder

It would be nice to get rid of those numbers in the macro names, and I
understand that we can't use variadic macros. Should we use varargs
functions instead of macros? Then we could lose the numbers, but we'd need
to introduce global variables to keep the notation short and sweet (or pass
in the previous_words and previous_words_count, which would be ugly
boilerplate worse than the numbers).

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

Hi

Here is a new version merging recent changes.

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

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.

Cheers,

Jeff

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Thu, Oct 22, 2015 at 7:05 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

I think it's already true that the previous lines of the query buffer
are stashed somewhere that psql knows about,

​Just skimming but:​

​"""
​\p or \print
Print the current query buffer to the standard output.
"""
​http://www.postgresql.org/docs/9.4/interactive/app-psql.html

David J.
​

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 the command line
editing capability.

- When people do compile with libreadline, the only software affected
by libreadine's license is the binary to which it is linked, 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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

* 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