EXECUTE tab completion

Hi,

Magnus's patch for adding tab completion of views to the TABLE statement
reminded me of a minor annoyance of mine -- that EXECUTE always
completes with "PROCEDURE" as if it would have been part of CREATE
TRIGGER ... EXECUTE even when it is the first word of the line.

Attached is a simple patch which adds completion of prepared statement
names to the EXECUTE statement.

What could perhaps be added is that if you press tab again after
completing the prepared statement name you might want to see a single
"(" appear. Did not add that though since "EXECUTE foo();" is not valid
syntax (while "EXECUTE foo(1);" is) and I did not feel the extra lines
of code to add a query to check if the number of expected parameters is
greater than 0 were worth it.

--
Andreas Karlsson

On Mon, Sep 26, 2011 at 5:03 PM, Andreas Karlsson <andreas(at)proxel(dot)se> wrote:
> Magnus's patch for adding tab completion of views to the TABLE statement
> reminded me of a minor annoyance of mine -- that EXECUTE always completes
> with "PROCEDURE" as if it would have been part of CREATE TRIGGER ... EXECUTE
> even when it is the first word of the line.

+1

> Attached is a simple patch which adds completion of prepared statement names
> to the EXECUTE statement.
>
> What could perhaps be added is that if you press tab again after completing
> the prepared statement name you might want to see a single "(" appear. Did
> not add that though since "EXECUTE foo();" is not valid syntax (while
> "EXECUTE foo(1);" is) and I did not feel the extra lines of code to add a
> query to check if the number of expected parameters is greater than 0 were
> worth it.

Yeah, that doesn't seem worth the trouble. The patch looks fine to me;
it doesn't break the existing EXECUTE completion intended for CREATE
TRIGGER and seems to work OK on a few examples I tried.

I guess the only quibble I can see is that the two comment lines might
be better written together, to mimic the neighboring comment styles,
as in:
/* EXECUTE */
/* must not match CREATE TRIGGER ... EXECUTE PROCEDURE */
else if ...

Incidentally, I was wondering what the heck was up with a clause like this:
else if (pg_strcasecmp(prev_wd, "EXECUTE") == 0 &&
pg_strcasecmp(prev2_wd, "EXECUTE") == 0)

though that looks to be some strange quirk of previous_word()'s behavior.

Josh

Josh Kupershmidt <schmiddy(at)gmail(dot)com> writes:
> Incidentally, I was wondering what the heck was up with a clause like this:
> else if (pg_strcasecmp(prev_wd, "EXECUTE") == 0 &&
> pg_strcasecmp(prev2_wd, "EXECUTE") == 0)

Hmm, maybe || was meant not && ? It seems pretty unlikely that the
above test would ever trigger on valid SQL input.

regards, tom lane

On Wed, Oct 19, 2011 at 10:40 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Josh Kupershmidt <schmiddy(at)gmail(dot)com> writes:
>> Incidentally, I was wondering what the heck was up with a clause like this:
>>     else if (pg_strcasecmp(prev_wd, "EXECUTE") == 0 &&
>>              pg_strcasecmp(prev2_wd, "EXECUTE") == 0)
>
> Hmm, maybe || was meant not && ?  It seems pretty unlikely that the
> above test would ever trigger on valid SQL input.

Well, changing '&&' to '||' breaks the stated comment of the patch, namely:
/* must not match CREATE TRIGGER ... EXECUTE PROCEDURE */

I assume this is an accepted quirk of previous_word() since we have
this existing similar code:

/* DROP, but watch out for DROP embedded in other commands */
/* complete with something you can drop */
else if (pg_strcasecmp(prev_wd, "DROP") == 0 &&
pg_strcasecmp(prev2_wd, "DROP") == 0)

and the patch does seem to auto-complete a beginning EXECUTE
correctly. We could probably use a comment somewhere explaining this
quirk.

Josh

Excerpts from Josh Kupershmidt's message of mié oct 19 23:50:58 -0300 2011:
> On Wed, Oct 19, 2011 at 10:40 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> > Josh Kupershmidt <schmiddy(at)gmail(dot)com> writes:
> >> Incidentally, I was wondering what the heck was up with a clause like this:
> >>     else if (pg_strcasecmp(prev_wd, "EXECUTE") == 0 &&
> >>              pg_strcasecmp(prev2_wd, "EXECUTE") == 0)
> >
> > Hmm, maybe || was meant not && ?  It seems pretty unlikely that the
> > above test would ever trigger on valid SQL input.
>
> Well, changing '&&' to '||' breaks the stated comment of the patch, namely:
> /* must not match CREATE TRIGGER ... EXECUTE PROCEDURE */
>
> I assume this is an accepted quirk of previous_word() since we have
> this existing similar code:
>
> /* DROP, but watch out for DROP embedded in other commands */
> /* complete with something you can drop */
> else if (pg_strcasecmp(prev_wd, "DROP") == 0 &&
> pg_strcasecmp(prev2_wd, "DROP") == 0)

Maybe both are wrong, though the DROP case seems to work so maybe it's
just dead code. This was introduced in commit
90725929465474648de133d216b873bdb69fe357:

***************
*** 674,685 **** psql_completion(char *text, int start, int end)
else if (pg_strcasecmp(prev_wd, "CREATE") == 0)
matches = completion_matches(text, create_command_generator);

! /* DROP, except ALTER (TABLE|DOMAIN|GROUP) sth DROP */
/* complete with something you can drop */
else if (pg_strcasecmp(prev_wd, "DROP") == 0 &&
! pg_strcasecmp(prev3_wd, "TABLE") != 0 &&
! pg_strcasecmp(prev3_wd, "DOMAIN") != 0 &&
! pg_strcasecmp(prev3_wd, "GROUP") != 0)
matches = completion_matches(text, drop_command_generator);

/* ALTER */
--- 674,683 ----
else if (pg_strcasecmp(prev_wd, "CREATE") == 0)
matches = completion_matches(text, create_command_generator);

! /* DROP, but watch out for DROP embedded in other commands */
/* complete with something you can drop */
else if (pg_strcasecmp(prev_wd, "DROP") == 0 &&
! pg_strcasecmp(prev2_wd, "DROP") == 0)
matches = completion_matches(text, drop_command_generator);

/* ALTER */

--
Álvaro Herrera <alvherre(at)commandprompt(dot)com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

Alvaro Herrera <alvherre(at)commandprompt(dot)com> writes:
> Excerpts from Josh Kupershmidt's message of mi oct 19 23:50:58 -0300 2011:
>> I assume this is an accepted quirk of previous_word() since we have
>> this existing similar code:

> Maybe both are wrong, though the DROP case seems to work so maybe it's
> just dead code.

I looked at the code more closely and I now see what Josh saw and why
this code is like this. If you take a desultory look at previous_word()
you will come away with the impression that it returns NULL when asked
for a word before the first word on the line. But in reality, it
returns NULL only if there is nothing before the current point.
Otherwise, you get duplicates of the first word on the line. For
example in "DROP TABLE <tab>" we'll get
prev_wd = TABLE
prev2_wd = DROP
prev3_wd = DROP
prev4_wd = DROP
prev5_wd = DROP
prev6_wd = DROP
I think this is just a plain old coding bug: the stop-test assumes that
end is reset to -1 each time through the outer loop, but it isn't.

However, we can't just fix the bug, because if previous_word() actually
starts to return NULLs like its author seems to have intended, the
strcasecmp calls that use its output will dump core.

What I suggest is that we redefine previous_word() as returning an empty
string, not NULL, anytime there is no such preceding word. This is
better than the current behavior because (a) it's less surprising and
(b) it's not ambiguous. Right now the caller can't tell the difference
between "DROP" and "DROP DROP DROP".

With that change, the correct test at line 795 would become

else if (pg_strcasecmp(prev_wd, "DROP") == 0 &&
prev2_wd[0] == '\0')

(or any other way you care to write a test for empty string). It looks
like there is only one place in the file that is actually expecting a
null result in prev_wd, so there wouldn't be much collateral damage
to fix.

regards, tom lane

I wrote:
> What I suggest is that we redefine previous_word() as returning an empty
> string, not NULL, anytime there is no such preceding word. This is
> better than the current behavior because (a) it's less surprising and
> (b) it's not ambiguous. Right now the caller can't tell the difference
> between "DROP" and "DROP DROP DROP".

> With that change, the correct test at line 795 would become

> else if (pg_strcasecmp(prev_wd, "DROP") == 0 &&
> prev2_wd[0] == '\0')

I've committed this --- please adjust the EXECUTE patch to match.

regards, tom lane

On 10/20/2011 09:53 PM, Tom Lane wrote:
>> With that change, the correct test at line 795 would become
>
>> else if (pg_strcasecmp(prev_wd, "DROP") == 0&&
>> prev2_wd[0] == '\0')
>
> I've committed this --- please adjust the EXECUTE patch to match.

Thanks for cleaning up the code to some sanity, I should have done so
myself when I noticed the problem.

A new version is attached.

Best regards,
Andreas

--
Andreas Karlsson

On Thu, Oct 20, 2011 at 5:16 PM, Andreas Karlsson <andreas(at)proxel(dot)se> wrote:
> A new version is attached.

Looks fine. Marking ready for committer (CF 2011-11).

Josh

Andreas Karlsson <andreas(at)proxel(dot)se> writes:
> Thanks for cleaning up the code to some sanity, I should have done so
> myself when I noticed the problem.

> A new version is attached.

Committed with minor adjustments --- I didn't see any need to make this
wait for the next commitfest.

regards, tom lane

On Mon, Oct 24, 2011 at 01:26, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Andreas Karlsson <andreas(at)proxel(dot)se> writes:
>> Thanks for cleaning up the code to some sanity, I should have done so
>> myself when I noticed the problem.
>
>> A new version is attached.
>
> Committed with minor adjustments --- I didn't see any need to make this
> wait for the next commitfest.

Thanks - I was planning to pick that one up along with my TABLE patch,
but was too busy with pgconf.eu over the past couple of weeks..

--
 Magnus Hagander
 Me: http://www.hagander.net/
 Work: http://www.redpill-linpro.com/