Thread: psql tab completion bug and possible fix

Recently I've been seeing regular but very occasional errors like the
following while using psql:

test=> BEGIN ;
BEGIN
test=> UPDATE language SET name_native = 'Français' WHERE lang_id='fr';
ERROR:  current transaction is aborted, commands ignored until end of
transaction block

where the UPDATE statement itself is entirely correct and is executed
correctly when a new transaction is started. Unfortunately I was never able
to reproduce the error and thought it might be some kind of beta flakiness,
until it turned up in a 7.3 installation too.

The culprit is the following section of psql's tab-complete.c , around line
1248:

/* WHERE */
    /* Simple case of the word before the where being the table name */
    else if (strcasecmp(prev_wd, "WHERE") == 0)
        COMPLETE_WITH_ATTR(prev2_wd);

which is correct for SELECT statements. Where the line contains an UPDATE
statement however, and tab is pressed after WHERE, the word before WHERE is
passed to the backend via a sprintf-generated query with the %s between single
quotes, i.e. in the above case
  AND c.relname='%s'
is translated to
  AND c.relname=''Français''

which is causing a silent error and the transaction failure.

I don't see a simple solution to cater for UPDATE syntax in this context
(you'd need to keep track of whether the statement begins with SELECT
or UPDATE), though it might be a good todo item.

A quick (but not dirty) fix for this and other current or future potential
corner cases would be to ensure any statements executed by the tab completion
functions are quoted correctly, so even if the statement does not produce any
results for tab completion, at least it cannot cause mysterious transaction
errors (and associated doubts about PostgreSQL's stability ;-).

A patch for this using PQescapeString (is there another preferred method?) is
attached as a possible solution.


Ian Barwick
barwick@gmx.net

Ian Barwick <barwick@gmx.net> writes:
> A patch for this using PQescapeString (is there another preferred method?) is
> attached as a possible solution.

Surely all those replacements of \\ with \\\\ are wrong.  I agree that
it's insane not to be escaping the user input, though ...

            regards, tom lane

On Tuesday 14 October 2003 23:38, Tom Lane wrote:
> Ian Barwick <barwick@gmx.net> writes:
> > A patch for this using PQescapeString (is there another preferred
> > method?) is attached as a possible solution.
>
> Surely all those replacements of \\ with \\\\ are wrong.

The slash in the slash command gets escaped too...:

#include <stdio.h>
#include "libpq-fe.h"

main() {
  char *s, *r;

  s = "\\d";

  printf("%s\n", s);

  r = (char *) malloc(strlen(s) * 2);
  PQescapeString(r, s, strlen(s));

  printf("%s\n", r);

  if(strcmp(r, "\\\\d") == 0) {
    printf("match\n");
  }

  free(s);
  free(r);
}

Without \\\\ tab expansion for slash commands doesn't work.

There may of course be better ways of solving this problem.


Ian Barwick
barwick@gmx.net

Ian Barwick <barwick@gmx.net> writes:
> On Tuesday 14 October 2003 23:38, Tom Lane wrote:
>> Surely all those replacements of \\ with \\\\ are wrong.

> The slash in the slash command gets escaped too...:

Not the way I did it.  You were doing the escaping in the wrong place
IMHO --- the string passed to _complete_from_query() mustn't be escaped
already, because it is not only used to send a command to the backend,
but also to compare against the strings returned by the backend, which
won't be escaped.  So the escaping needs to be done internally to
_complete_from_query(), and that eliminates the side-effects elsewhere.

            regards, tom lane

On Wednesday 15 October 2003 22:45, Tom Lane wrote:
> Ian Barwick <barwick@gmx.net> writes:
> > On Tuesday 14 October 2003 23:38, Tom Lane wrote:
> >> Surely all those replacements of \\ with \\\\ are wrong.
> >
> > The slash in the slash command gets escaped too...:
>
> Not the way I did it.  You were doing the escaping in the wrong place
> IMHO --- the string passed to _complete_from_query() mustn't be escaped
> already, because it is not only used to send a command to the backend,
> but also to compare against the strings returned by the backend, which
> won't be escaped.  So the escaping needs to be done internally to
> _complete_from_query(), and that eliminates the side-effects elsewhere.

Aha, sorry, I hadn't seen your patch. It works excellently for me, so
I shall gripe no longer ;-).

Many thanks

Ian Barwick
barwick@gmx.net