[PATCH] support tab-completion for single quote input with equal sign

On Thursday, July 22, 2021 1:05 PM, tanghy.fnst@fujitsu.com <tanghy.fnst@fujitsu.com> wrote

I found a problem when using tab-completion as follows:

CREATE SUBSCRIPTION my_subscription
CONNECTION 'host=localhost port=5432 dbname=postgres' [TAB]

The word 'PUBLICATION' couldn't be auto completed as expected.

Added above patch in commit fest as follows:

https://commitfest.postgresql.org/34/3267/

Regards,
Tang

On Fri, 2021-07-23 at 05:34 +0000, tanghy.fnst@fujitsu.com wrote:

On Thursday, July 22, 2021 1:05 PM, tanghy.fnst@fujitsu.com <tanghy.fnst@fujitsu.com> wrote

I found a problem when using tab-completion as follows:

CREATE SUBSCRIPTION my_subscription
CONNECTION 'host=localhost port=5432 dbname=postgres' [TAB]

The word 'PUBLICATION' couldn't be auto completed as expected.

Hello,

I applied your patch against HEAD (and did a clean build for good
measure) but couldn't get the tab-completion you described -- on my
machine, `PUBLICATION` still fails to complete. Tab completion is
working in general, for example with the `SUBSCRIPTION` and
`CONNECTION` keywords.

Is there additional setup that I need to do?

--Jacob

On Friday, September 3, 2021 2:14 AM, Jacob Champion <pchampion@vmware.com> wrote

I applied your patch against HEAD (and did a clean build for good
measure) but couldn't get the tab-completion you described -- on my
machine, `PUBLICATION` still fails to complete. Tab completion is
working in general, for example with the `SUBSCRIPTION` and
`CONNECTION` keywords.

Is there additional setup that I need to do?

Thanks for your check.

I applied the 0001-patch to the HEAD(c95ede41) by now and it worked as I expected.
Did you leave a space between "dbname=postgres'" and "[TAB]"?

In 0002-patch I added a tap test for the scenario of single quoted input with equal sign.
The test result is 'All tests successful. ' on my machine.
You can run the tap tests as follows:
1. Apply the attached patch
2. build the src with option '--enable-tap-tests' (./configure --enable-tap-tests)
3. Move to src subdirectory 'src/bin/psql'
4. Run 'make check'

I'd appreciate it if you can share your test results with me.

Regards,
Tang

Hi Tang,

On Fri, 2021-09-03 at 04:32 +0000, tanghy.fnst@fujitsu.com wrote:

I'd appreciate it if you can share your test results with me.

Sure! Here's my output (after a `make clean && make`):

cd . && TESTDIR='/home/pchampion/workspace/postgres/src/bin/psql' PATH="/home/pchampion/workspace/postgres/tmp_install/usr/local/pgsql-master/bin:$PATH" LD_LIBRARY_PATH="/home/pchampion/workspace/postgres/tmp_install/usr/local/pgsql-master/lib" PGPORT='65432' PG_REGRESS='/home/pchampion/workspace/postgres/src/bin/psql/../../../src/test/regress/pg_regress' /usr/bin/prove -I ../../../src/test/perl/ -I . t/*.pl
t/010_tab_completion.pl .. 17/?
# Failed test 'tab-completion after single quoted text input with equal sign'
# at t/010_tab_completion.pl line 198.
# Actual output was "CREATE SUBSCRIPTION my_sub CONNECTION 'host=localhost port=5432 dbname=postgres' \a"
# Did not match "(?^:PUBLICATION)"
# Looks like you failed 1 test of 23.
t/010_tab_completion.pl .. Dubious, test returned 1 (wstat 256, 0x100)
Failed 1/23 subtests
t/020_cancel.pl .......... ok

Test Summary Report
-------------------
t/010_tab_completion.pl (Wstat: 256 Tests: 23 Failed: 1)
Failed test: 17
Non-zero exit status: 1
Files=2, Tests=25, 8 wallclock secs ( 0.02 usr 0.01 sys + 1.48 cusr 0.37 csys = 1.88 CPU)
Result: FAIL
make: *** [Makefile:87: check] Error 1

Thanks,
--Jacob

"tanghy.fnst@fujitsu.com" <tanghy.fnst@fujitsu.com> writes:

[ v2-0001-support-tab-completion-for-single-quote-input-wit.patch ]

Surely this patch is completely wrong? It needs more thought about
the interaction with the existing logic for double quotes, ie single
quote inside double quotes is not special, nor the reverse; nor should
parentheses inside quotes be counted. It also needs to be aware of
backslashes in escape-style strings.

I kind of doubt that it's actually possible to parse string literals
correctly when working backward, as this function does. For starters,
you won't know whether the string starts with "E", so you won't know
whether backslashes are special. We've got away with backwards
parsing so far because the syntax rules for double-quoted strings are
so much simpler. But if you want to handle single quotes, I think
you'll have to start by rearranging the code to parse forward. That's
likely to be fairly ticklish, see the comment about

* backwards scan has some interesting but intentional properties
* concerning parenthesis handling.

I wish that whoever wrote that (which I think was me :-() had been
more explicit. But I think that the point is that we look for a place
that's at the same parenthesis nesting level as the completion point,
not necessarily one that's globally outside of any parens. That will
be messy to handle if we want to convert to scanning forwards from
the start of the string.

I kind of wonder if it isn't time to enlist the help of psqlscan.l
instead of doubling down on the idea that tab-complete.c should have
its own half-baked SQL lexer.

regards, tom lane

I wrote:

Surely this patch is completely wrong? It needs more thought about
the interaction with the existing logic for double quotes, ie single
quote inside double quotes is not special, nor the reverse; nor should
parentheses inside quotes be counted. It also needs to be aware of
backslashes in escape-style strings.

Actually ... those are just implementation details, and now that
I've thought about it a little more, I question the entire concept
of making single-quoted strings be single words in tab-complete's
view. I think it's quite intentional that we don't do that;
if we did, it'd forever foreclose the possibility of tab-completing
*within* strings. You don't have to look any further than CREATE
SUBSCRIPTION itself to see possible applications of that: someone
could wish that

CREATE SUBSCRIPTION my_sub CONNECTION 'db<TAB>

would complete with "name=", or that <TAB> right after the quote
would offer a list of connection keywords.

(More generally, I'm afraid that people are already relying on this
behavior in other contexts, and thus that the proposed patch could
break more use-cases than it fixes.)

So now I think that this approach should be rejected, and that the
right thing is to fix the CREATE SUBSCRIPTION completion rules
to allow more than one "word" between CONNECTION and PUBLICATION.

Another idea that might be useful is to treat the opening and
closing quotes themselves as separate "words", which'd give
the CREATE SUBSCRIPTION rules a bit more to go on about when to
offer PUBLICATION.

regards, tom lane

Jacob Champion <pchampion@vmware.com> writes:

t/010_tab_completion.pl .. 17/?
# Failed test 'tab-completion after single quoted text input with equal sign'
# at t/010_tab_completion.pl line 198.
# Actual output was "CREATE SUBSCRIPTION my_sub CONNECTION 'host=localhost port=5432 dbname=postgres' \a"
# Did not match "(?^:PUBLICATION)"
# Looks like you failed 1 test of 23.

Independently of the concerns I raised, I'm wondering how come you
are getting different results. Which readline or libedit version
are you using, on what platform?

regards, tom lane

On Sat, 2021-09-04 at 11:32 -0400, Tom Lane wrote:

Jacob Champion <pchampion@vmware.com> writes:

t/010_tab_completion.pl .. 17/?
# Failed test 'tab-completion after single quoted text input with equal sign'
# at t/010_tab_completion.pl line 198.
# Actual output was "CREATE SUBSCRIPTION my_sub CONNECTION 'host=localhost port=5432 dbname=postgres' \a"
# Did not match "(?^:PUBLICATION)"
# Looks like you failed 1 test of 23.

Independently of the concerns I raised, I'm wondering how come you
are getting different results. Which readline or libedit version
are you using, on what platform?

Now you have me worried...

- Ubuntu 20.04
- libedit, version 3.1-20191231-1

--Jacob

Jacob Champion <pchampion@vmware.com> writes:

On Sat, 2021-09-04 at 11:32 -0400, Tom Lane wrote:

Independently of the concerns I raised, I'm wondering how come you
are getting different results. Which readline or libedit version
are you using, on what platform?

Now you have me worried...
- Ubuntu 20.04
- libedit, version 3.1-20191231-1

We've had troubles with libedit before :-(, and that particular release
is known to be a bit buggy [1]http://gnats.netbsd.org/cgi-bin/query-pr-single.pl?number=54510.

I can reproduce a problem using HEAD (no patch needed) and Fedora 32's
libedit-3.1-32.20191231cvs.fc32.x86_64: if I type

create subscription s connection foo <TAB>

then it happily completes "PUBLICATION", but if I type

create subscription s connection 'foo' <TAB>

I just get beeps.

I do *not* see this misbehavior on a nearby FreeBSD 13.0 machine
with libedit 3.1.20210216,1 (which isn't even the latest version).
So it's a fixed-some-time-ago bug. Given the symptoms, I wonder
if it isn't closely related to the original complaint at [1]http://gnats.netbsd.org/cgi-bin/query-pr-single.pl?number=54510.

Anyway, the bottom line from our standpoint is going to be that
we can't put a test case like this one into the TAP test. I recall
that getting 010_tab_completion.pl to pass everywhere was a huge
headache at the outset, so this conclusion doesn't surprise me
much.

regards, tom lane

[1]: http://gnats.netbsd.org/cgi-bin/query-pr-single.pl?number=54510

On Wed, 2021-09-08 at 17:08 -0400, Tom Lane wrote:

Jacob Champion <pchampion@vmware.com> writes:

On Sat, 2021-09-04 at 11:32 -0400, Tom Lane wrote:

Independently of the concerns I raised, I'm wondering how come you
are getting different results. Which readline or libedit version
are you using, on what platform?

Now you have me worried...
- Ubuntu 20.04
- libedit, version 3.1-20191231-1

We've had troubles with libedit before :-(, and that particular release
is known to be a bit buggy [1].

That's... unfortunate. Thanks for the info; I wonder what other tab-
completion bugs I've just accepted as "standard behavior".

--Jacob

On Saturday, September 4, 2021 11:58 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Actually ... those are just implementation details, and now that
I've thought about it a little more, I question the entire concept
of making single-quoted strings be single words in tab-complete's
view. I think it's quite intentional that we don't do that;
if we did, it'd forever foreclose the possibility of tab-completing
*within* strings. You don't have to look any further than CREATE
SUBSCRIPTION itself to see possible applications of that: someone
could wish that

CREATE SUBSCRIPTION my_sub CONNECTION 'db<TAB>

would complete with "name=", or that <TAB> right after the quote
would offer a list of connection keywords.

(More generally, I'm afraid that people are already relying on this
behavior in other contexts, and thus that the proposed patch could
break more use-cases than it fixes.)

Agreed. Thanks for your comments.

So now I think that this approach should be rejected, and that the
right thing is to fix the CREATE SUBSCRIPTION completion rules
to allow more than one "word" between CONNECTION and PUBLICATION.

Another idea that might be useful is to treat the opening and
closing quotes themselves as separate "words", which'd give
the CREATE SUBSCRIPTION rules a bit more to go on about when to
offer PUBLICATION.

Treat the opening and closing quotes themselves as separate "words" may affect some current tap completion.
So I tried to fix the CREATE SUBSCRIPTION completion rules in the V3 patch.
The basic idea is to check the head words of the input text as "CREATE SUBSCRIPTION subname CONNECTION anystring",
then check to see if anystring ends with single quote. If all check passed, PUBLICATION will be auto-completed.

Tap tests(including the one added in V3) has been passed.

Regards,
Tang

At Sat, 04 Sep 2021 10:18:24 -0400, Tom Lane <tgl@sss.pgh.pa.us> wrote in

I kind of wonder if it isn't time to enlist the help of psqlscan.l
instead of doubling down on the idea that tab-complete.c should have
its own half-baked SQL lexer.

So, I played with this idea and came up with the attached WIP. The
test added by the original patch succeeds with it not tweakig the
"Matches" part in psql_completion.

While I checked this I found several dubious parts in TAP test.

=== 010_tab_completion.pl
# COPY requires quoting
# note: broken versions of libedit want to backslash the closing quote;
# not much we can do about that
check_completion(
- "COPY foo FROM tmp_check/some\t",
+ "COPY foo FROM \'tmp_check/some\t",

The original command syntax is just wrong and this patch make
completion code treat the command line correctly (breaks the
"filename" into "tmp_check" "/" "some") and the test item fails.

check_completion(
-	"COPY foo FROM tmp_check/af\t",
-	qr|'tmp_check/afile|,
+	"COPY foo FROM \'tmp_check/af\t",
+	qr|'tmp_check/af\a?ile|,	# \a is BEL

This test fails for the same reason, but after fixing it the result
contains \a (BEL) in the output on my CentOS8. I'm not sure what is
happening here..

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center

On Fri, Sep 17, 2021 at 02:45:57AM +0900, Kyotaro Horiguchi wrote:

This test fails for the same reason, but after fixing it the result
contains \a (BEL) in the output on my CentOS8. I'm not sure what is
happening here..

The patch is still failing under the CF bot, and this last update was
two months ago. I am marking it as returned with feedback.
--
Michael

At Fri, 3 Dec 2021 15:16:55 +0900, Michael Paquier <michael@paquier.xyz> wrote in

On Fri, Sep 17, 2021 at 02:45:57AM +0900, Kyotaro Horiguchi wrote:

This test fails for the same reason, but after fixing it the result
contains \a (BEL) in the output on my CentOS8. I'm not sure what is
happening here..

The patch is still failing under the CF bot, and this last update was
two months ago. I am marking it as returned with feedback.

I thought the last *WIP* patch as just a proposal of another direction
after the first attempt was failed.

I'll start another thread named as 'let's psql-completion share the
psql command line lexer' or such, after rebasing and some polishing.

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center

On Thursday, July 22, 2021 1:05 PM, tanghy(dot)fnst(at)fujitsu(dot)com
<tanghy(dot)fnst(at)fujitsu(dot)com> wrote

I found a problem when using tab-completion as follows:

CREATE SUBSCRIPTION my_subscription
CONNECTION 'host=localhost port=5432 dbname=postgres' [TAB]

The word 'PUBLICATION' couldn't be auto completed as expected.

I too wondered about this behavior.

v3-0001-support-tab-completion-for-CONNECTION-string-with.patch

I applied the patch and succeeded in the above case, but failed in the
below case.

=# CREATE SUBSCRIPTION s1 CONNECTION 'a=' PUBLICATION p1 <tab>

Before applying the patch, 'WITH (' was completed, but now it completes
nothing since it matches the below condition:

18 + else if ((HeadMatches("CREATE", "SUBSCRIPTION", MatchAny,
"CONNECTION", MatchAny)))
19 + {

I updated the patch going along with the v3 direction.

What do you think?

--
Regards,

--
Atsushi Torikoshi
NTT DATA CORPORATION

torikoshia <torikoshia@oss.nttdata.com> writes:

I updated the patch going along with the v3 direction.

I think this adds about as many failure modes as it removes,
if not more.

* The connection string doesn't necessarily end with "'"; it could
be a dollar-quoted string.

* If it is a dollar-quoted string, there could be "'" marks internal
to it, allowing PUBLICATION to be falsely offered when we're really
still within the connection string.

* The following WITH options could contain "'", allowing PUBLICATION
to be falsely offered within that clause.

I've spent some effort previously on getting tab-completion to deal
sanely with single-quoted strings, but everything I've tried has
crashed and burned :-(, mainly because it's not clear when to take
the whole literal as one "word" and when not. A relevant example
here is that somebody might wish that we could tab-complete within
the connection string, e.g. that

CREATE SUBSCRIPTION sub CONNECTION 'db<TAB>

would complete with "name=". We have the info available from libpq
to do that, if only we could figure out when to apply it. I think
we need some pretty fundamental design work to figure out what we
want to do in this area, and that in the meantime putting band-aids
on specific cases is probably not very productive.

regards, tom lane

I wrote:

I've spent some effort previously on getting tab-completion to deal
sanely with single-quoted strings, but everything I've tried has
crashed and burned :-(, mainly because it's not clear when to take
the whole literal as one "word" and when not.

After a little further thought, a new idea occurred to me: maybe
we could push some of the problem down into the Matches functions.
Consider inventing a couple of new match primitives:

* MatchLiteral matches one or more parse tokens that form a single
complete, valid SQL literal string (either single-quoted or dollar
style). Use it like

else if (Matches("CREATE", "SUBSCRIPTION", MatchAny, "CONNECTION", MatchLiteral))
COMPLETE_WITH("PUBLICATION");

I think there's no question that most Matches calls that might subsume
a quoted literal would prefer to treat the literal as a single word,
and this'd let them do that correctly.

* MatchLiteralBegin matches the opening of a literal string (either '
or $...$). Handwaving freely, we might do

else if (Matches("CREATE", "SUBSCRIPTION", MatchAny, "CONNECTION", MatchLiteralBegin))
COMPLETE_WITH(List_of_connection_keywords);

This part of the idea still needs some thought, because it remains
unclear how we might offer completion for connection keywords
after the first one.

Implementing these primitives might be a little tricky too.
If memory serves, readline and libedit have different behaviors
around quote marks. But at least it seems like a framework
that could solve a number of problems, if we can make it go.

regards, tom lane

On 2023-01-11 11:28, Tom Lane wrote:

I wrote:

I've spent some effort previously on getting tab-completion to deal
sanely with single-quoted strings, but everything I've tried has
crashed and burned :-(, mainly because it's not clear when to take
the whole literal as one "word" and when not.

After a little further thought, a new idea occurred to me: maybe
we could push some of the problem down into the Matches functions.
Consider inventing a couple of new match primitives:

Thanks for the idea!
I'm going to try it.

--
Atsushi Torikoshi
NTT DATA CORPORATION